Mismatch between time-format in spec and tests

[ybroeker]

Spec defines HH:mm:ss as default-format for times, but tests expect HH:mm:ss.sss, eg in https://github.com/eclipse/microprofile-graphql/blob/master/server/tck/src/main/resources/tests/basicScalar/output.json#L47

[phillip-kruger]

It should be the default ISO Format:
https://docs.oracle.com/javase/8/docs/api/java/time/format/DateTimeFormatter.html#ISO_LOCAL_TIME
So I think both is actually valid ?

[ybroeker]

HH:mm:ss would always drop the nanoseconds-parts, the ISO_LOCAL_TIME-formatter would include those, if they're available, so both produce slightly different results.

Reason behind this issue is the time-formatting in smallrye-graphql. Time is formatted with HH:mm:ss, eg 11:46:34 - which should be correct according to spec (or did I misunderstood this part?).
But the tck fails, because it expects eg 11:46:34.263.

[phillip-kruger]

@andymc12 what do you think ?

[phillip-kruger]

@ybroeker I think you are right, the spec text is correct, but the TCK is wrong. I think we should fix the TCK.
Smallrye current version use a hand coded default format (that is why it's passing the TCK), however the new one we are working on is using the default of DateTimeFormatter, and that is the same as the spec.

[phillip-kruger]

Ok, so I think the reason is that the Test specifically do
LocalTime localTime = LocalTime.parse("11:46:34.263");
So the TCK is correct. But it is confusing, so I suggest that we change the TCK to:
LocalTime localTime = LocalTime.parse("11:46:34");

The spec is "correct" but should maybe change to reference the ISO link in my comment above

[ybroeker]

Smallrye current version use a hand coded default format (that is why it's passing the TCK), however the new one we are working on is using the default of DateTimeFormatter, and that is the same as the spec.

Which versions do you mean by current and new? Maybe I misunderstood, but didn't 1.0.0 use DateTimeFormatter.ISO_TIME (and passed the tck) and current master uses "HH:mm:ss" as pattern (and would fail the tck)?

Ok, so I think the reason is that the Test specifically do
LocalTime localTime = LocalTime.parse("11:46:34.263");
So the TCK is correct. But it is confusing, so I suggest that we change the TCK to:
LocalTime localTime = LocalTime.parse("11:46:34");

The spec is "correct" but should maybe change to reference the ISO link in my comment above

Wouldn't this just hide this "edge case" and add confusion?

A date with the format HH:mm:ss could be parsed with DateTimeFormatter.ISO_LOCAL_TIME, but the opposite way doesn't work (only, if the nanoseconds are randomly 0), which could introduce subtle bugs in client-code.

Also, the generated description in the schema wouldn't match the format of the values.

A type with two time-fields, one with @DateFormat("HH:mm:ss")and one without, would generate this schema:

type Dates {
  "HH:mm:ss"
  time1: Time
  "HH:mm:ss"
  time2: Time
}

And return this values, even if both values are the exact same timestamp:

{
  "time1": "11:46:34.263",
  "time2": "11:46:34"
}

(and, to add confusion, time1 would sometimes randomly be "11:46:34").

But I honestly don't know how best to deal with it either.

[phillip-kruger]

Yes, sorry I was wrong, the current version is using the handcoded default formatting, although the correct way would be not to do any formatting if the developer did not add a formatting annotation.
(I am fixing that now in the master SmallRye)
This is the same in JsonB, so I think we should stick with that.

Any ISO time is valid. That include: HH:mm, HH:mm:ss , HH:mm:ss.SSS.

My suggestion is that we remove the description when no formatting annotation is present, as you pointed out the format is just one of the correct ones and could be confusing. I think we can say that when using LocalTime (or the others) ISO is valid.

But, that does mean that we should, when adding formatting, change the type to String (in my opinion) and then add the format in the description. Or we need to be clever and figure out if the format in the formatted annotation is still a valid ISO, then keep it Time, but once the format is not anymore change to String. That can just be confusing though, so I am leaning toward just make it String when formatting.

Let's discuss in the meeting.

[ybroeker]

I like the idea to distinguish between scalars (in ISO) and formatted date.

This would also be consistent with formatting of numbers and would make #169 much easier.

[ybroeker]

Because I can't attend the meeting a quick update here:
I would do it consistently with number formatting.
Without @DateFormat as Date-field (or appropriate scalar) formatted as ISO-date and without description (but maybe description at the scalar-type?). And with @DateFormat as string-field and with format in the field-description.

[andymc12]

In the meeting today, we discusses that this should probably have the similar behavior as #209 (which is specific to Number type). @phillip-kruger will look to see if it is possible (without breaking 1.0 behavior) to always use the Date, Time, and DateTime for ISO dates, times, and date/times, respectively - and then when any formatting is applied, it becomes a different type (String, FormattedDate, StringifiedDate).