Skip to content
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.

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

DateTime string formats / ISO 8601 #16

Open
jwbrew opened this issue Mar 19, 2021 · 8 comments · Fixed by #20
Open

DateTime string formats / ISO 8601 #16

jwbrew opened this issue Mar 19, 2021 · 8 comments · Fixed by #20

Comments

@jwbrew
Copy link

jwbrew commented Mar 19, 2021

I'm using Servant & OpenAPI to generate a schema for code generation via openapi-generator, targeting Dart as a client language.

The current implmentations of declareNamedSchema for UTCTime, and toParamSchema for UCTime and ZonedTime generate a schema of type: string with a custom format as a specific representation of each type. This breaks codegen in the Dart client, as it defines the required type as a String where a DateTime would be more appropriate. This seems to be down to the format being a custom representation, rather than one of openapi3's built-in format values.

Given the custom formats as currently returned (yyyy-mm-ddThh:MM:ssZ and yyyy-mm-ddThh:MM:ss+hhMM) are (as far as I can see) ISO8601 - and therefore openapi3 date-time compliant, I would like to open a PR that changes the implementation declareNamedSchema for UTCTime, and toParamSchema for UTCTime and ZonedTime to return format: "date-time".

Is this a reasonable thing to do?

As ever with matters involving dates and times the devil is hiding in every detail and I expect there's a subtle detail I've missed here. Hopefully not however, as these changes help avoid wrapping a whole load of types to generate the right schema!

@maksbotan
Copy link
Collaborator

Hi @jwbrew,

My guess is that UTCTime uses this format to specifically indicate that the time zone is always Z. If we switch to format: date-time, the reader of the spec will not know this detail. What do you think?

As for ToParamSchema instance for ZonedTime, I suppose this is a mistake, I will fix that.

maksbotan added a commit that referenced this issue Apr 17, 2021
Previously ToParamSchema ZonedTime instance used custom format instead
of OpenAPI default "date-time".

Partially fixes #16.
maksbotan added a commit that referenced this issue Apr 17, 2021
Previously ToParamSchema ZonedTime instance used custom format instead
of OpenAPI default "date-time".

Partially fixes #16.
@maksbotan maksbotan reopened this Apr 17, 2021
@maksbotan
Copy link
Collaborator

I've changed ToParamSchema ZonedTime instance for now. Let me know what you think about my comment above.

@jwbrew
Copy link
Author

jwbrew commented Apr 19, 2021

My guess is that UTCTime uses this format to specifically indicate that the time zone is always Z. If we switch to format: date-time, the reader of the spec will not know this detail. What do you think?

Yup I think that makes complete sense. IIRC I remember thinking something similar some time after posting the original comment. Thanks for making the change to ZonedTime, it's going to make things a whole lot more straightforward now!

@maksbotan
Copy link
Collaborator

You are welcome!

I'll keep this issue open for some time, to think about it once again later.

@dhess
Copy link

dhess commented Oct 20, 2022

We've just run across this and I wonder whether @maksbotan you've had a chance to think about this some more.

I'm not convinced that the benefit of the client knowing that the timezone is always UTC for UTCTime is worth the tradeoff of not recognizing the format string. In our case, at least, this just creates a big hassle: our generator doesn't recognize UTCTimes as a date, so we have two choices: do some client-side overrides to say, "no, these fields are in fact Date and not string", or server-side wrangling of internal UTCTime values to ZonedTime before we export them via the OpenAPI API.

@maksbotan
Copy link
Collaborator

Well, that makes sense. After all, always-Z time is certainly a subtype of ISO8601 time. Can you make a PR for that change?

BTW, did you consider using UNIX timestamps instead of strings? IMO that representation is far more convenient.

@dhess
Copy link

dhess commented Oct 26, 2022

Assuming the PR is as simple as I'm imagining, I'll try to find the time soon, yes.

I didn't consider UNIX timestamps. We want to store YYYY-MM-DD and HH:MM:SS uniformly on the server side as UTC and then use the client's browser to render those dates+times as appropriate for the client's locale. There's a straightforward way (or should be, per my argument above) to go from UTCTime in Haskell to date-time in OpenAPI, and then to Date in JavaScript, so what could be more convenient than that for our purposes? :)

I'm all ears though if you think there's a better/simpler way to achieve this with UNIX timestamps.

@maksbotan
Copy link
Collaborator

I think it's a matter of personal preference. For me timestamps are better, but it may not suit you.

We always expose datetime in our API as an integer — UNIX timestamp (or timestamp*1000 for millisecond precision), which by definition is always in UTC. It's then the client's (e.g. JS application in browser) responsibility to display it in any format and in any timezone. IMO there is no possibility to parse such date wrongly. And, for me, number (UTCTime) -> string (haskell) -> string (client) -> number (Date in js) roundtrip is weird — what do we need the intermediate string for?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants