Skip to content

Add two more options for returning a DateTime - Instant and Date #26

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Add two more options for returning a DateTime - Instant and Date #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
38e8d5d
Add two more options for returning a DateTime - Instant and Date
fotoetienne Jul 3, 2020
c8b7ca4
Add serialization for LocalDateTime
fotoetienne Jul 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/main/java/graphql/scalars/datetime/DateTimeScalar.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,10 @@
import graphql.schema.CoercingSerializeException;
import graphql.schema.GraphQLScalarType;

import java.time.DateTimeException;
import java.time.OffsetDateTime;
import java.time.ZonedDateTime;
import java.time.*;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are zero wildcard imports in this repository. I recommend to match the repository's code style.

import java.time.format.DateTimeFormatter;
import java.time.format.DateTimeParseException;
import java.util.Date;
import java.util.function.Function;

import static graphql.scalars.util.Kit.typeName;
Expand All @@ -32,6 +31,12 @@ public String serialize(Object input) throws CoercingSerializeException {
offsetDateTime = (OffsetDateTime) input;
} else if (input instanceof ZonedDateTime) {
offsetDateTime = ((ZonedDateTime) input).toOffsetDateTime();
} else if (input instanceof Instant) {
offsetDateTime = ((Instant) input).atOffset(ZoneOffset.UTC);
} else if (input instanceof LocalDateTime) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it makes sense to ever convert a LocalDateTime to a single moment in time without extra context. The reason for using the LocalDateTime type in Java is to semantically indicate that the time does not represent a universal moment in time but rather a conceptual calendar date and time.

It cannot represent an instant on the time-line without additional information such as an offset or time-zone.

That's why these methods exist:

  • LocalDateTime.atOffset(zoneOffset)
  • LocalDateTime.atZone(zoneId)

Passing a LocalDateTime to this serialize method probably indicates there are bugs around due to misunderstanding dates. Otherwise, if the caller truly means for a LocalDateTime to represent a UTC date/time, then the caller can call .atOffset(ZoneOffset.UTC) first.

offsetDateTime = ((LocalDateTime) input).atOffset(ZoneOffset.UTC);
} else if (input instanceof Date) {
offsetDateTime = ((Date) input).toInstant().atOffset(ZoneOffset.UTC);
} else if (input instanceof String) {
offsetDateTime = parseOffsetDateTime(input.toString(), CoercingSerializeException::new);
} else {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about the parseValue method below? That one still only has cases for OffsetDateTime, ZonedDateTime, and String.

Expand Down
8 changes: 6 additions & 2 deletions src/test/groovy/graphql/scalars/datetime/DateTimeScalarTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import graphql.schema.CoercingParseValueException
import graphql.schema.CoercingSerializeException
import spock.lang.Specification
import spock.lang.Unroll
import java.util.Date
import java.time.Instant

import static graphql.scalars.util.TestKit.mkLocalDT
import static graphql.scalars.util.TestKit.mkOffsetDT
Expand Down Expand Up @@ -68,6 +70,9 @@ class DateTimeScalarTest extends Specification {
"1937-01-01T12:00:27.87+00:20" | "1937-01-01T12:00:27.87+00:20"
mkOffsetDT(year: 1980, hour: 3) | "1980-08-08T03:10:09+10:00"
mkZonedDT(year: 1980, hour: 3) | "1980-08-08T03:10:09+10:00"
mkLocalDT(year: 1980, hour: 3) | "1980-08-08T03:10:09Z"
new Instant(334588209, 0) | "1980-08-08T13:10:09Z"
new Date(334588209000) | "1980-08-08T13:10:09Z"
}

def "datetime serialisation bad inputs"() {
Expand All @@ -79,8 +84,7 @@ class DateTimeScalarTest extends Specification {
where:
input | expectedValue
"1985-04-12" | CoercingSerializeException
mkLocalDT(year: 1980, hour: 3) | CoercingSerializeException
666 || CoercingSerializeException
666 | CoercingSerializeException
}

}