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

Add support for date/time formats #98

Merged
merged 8 commits into from
Oct 5, 2023
Merged

Add support for date/time formats #98

merged 8 commits into from
Oct 5, 2023

Conversation

durandj
Copy link
Contributor

@durandj durandj commented Jul 16, 2023

DRAFT PR

This is a draft of the final product. So far I've implemented 2 of the 3 date/time formats and wanted to confirm that my approach was acceptable before doing the third and before updating the docs.

I also want to add some helper functions for my custom type I had to add which you can learn more about below.

Summary

This PR adds support for the 3 date/time formats that are supported by JSON schema: date, time, and date-time.

All 3 formats build off of the time.Time type but in a couple different ways. For date-time we can just use the native type because the provided behavior for JSON is exactly what is desired. When marshaling to JSON the value is converted to an ISO-8601 timestamp. When unmarshaling from a JSON string it expects an ISO-8601 timestamp.

For date and time though things had to be changed up a bit. With date we expect to only be given a date string without any time component and the time format is the same but in reverse. If you tried to unmarshal \"2023-01-02\" though you would get an error since it's missing a time component.

There were 5 possible approaches that I came up with to resolve this issue:

  1. Create a shim type for each string that is marked as a date or a time which knows how to properly marshal/unmarshal the value. I rejected this approach because it could create a lot of extra bloat in the client code if there are a lot of generated types. It is however the most straightforward approach.
  2. Modify the UnmarshalJSON and MarshalJSON methods that are generated to be aware of if there's a date or time field and if so write to a map and then iterate over each field and do custom logic for each field. This gives the most flexibility but is overly complicated and basically re-implements what Go does so I rejected this approach.
  3. Create a shim of the entire generated code which treats all date and time fields as strings and then generates a final type which has time.Time as the field value. There would then need to be extra logic to initially use the shim type and then convert to the final type and do special handling for dates and times. This again produces a lot of boilerplate and requires create two copies of the data which seems wasteful so this was rejected.
  4. Don't do anything special and just leave date and time values as strings but this is annoying as a consumer of this tool since I have to do the conversion myself so this was rejected.
  5. Create a thin wrapper around the time.Time type which knows how to handle the missing component (the time for date and the date for time). This type can be used basically the same as a time.Time instance (because that's what it is) but it will work with JSON in a cleaner way. I'm not super thrilled that this adds a new type that must be interacted with in client code but there are some examples of this in the Go standard library already ( https://pkg.go.dev/database/sql#NullBool )

On top of adding tests for these changes into this repository I also tested them against my own project where I wanted to use this tool.

@omissis
Copy link
Owner

omissis commented Jul 16, 2023

Hey @durandj , thanks for the contribution! I will look at it asap :)

@codecov
Copy link

codecov bot commented Jul 16, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (8e9be7c) 0.00% compared to head (0fae179) 77.99%.

Additional details and impacted files
@@            Coverage Diff            @@
##           main      #98       +/-   ##
=========================================
+ Coverage      0   77.99%   +77.99%     
=========================================
  Files         0       17       +17     
  Lines         0     1718     +1718     
=========================================
+ Hits          0     1340     +1340     
- Misses        0      293      +293     
- Partials      0       85       +85     
Files Coverage Δ
pkg/codegen/utils.go 92.30% <100.00%> (ø)
types/date.go 100.00% <100.00%> (ø)
types/time.go 100.00% <100.00%> (ø)

... and 14 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@omissis
Copy link
Owner

omissis commented Jul 21, 2023

hey @durandj ! I've been looking at your proposal it looks solid to me, so +1 for going down that path. I've also took a moment to look around this issue on the Go repository and pondered if we could use google's civil package, but it's probably better to provide our types instead of requiring a third party library.

@omissis omissis marked this pull request as draft July 21, 2023 21:14
@omissis omissis self-requested a review July 21, 2023 21:15
@omissis omissis added the enhancement New feature or request label Jul 21, 2023
@durandj
Copy link
Contributor Author

durandj commented Jul 21, 2023

Huh interesting, hadn't seen the civil package before. Yeah I'm thinking it's better to stick to the one required dependency for consumers versus opting them into another without consent.

Cool, if you're happy with how this looks so far then I'll work on getting the time format completed as well and then get the docs updated as well.

Thanks for the preliminary review!

@omissis
Copy link
Owner

omissis commented Sep 30, 2023

hi @durandj ! did you have any time to work on this PR? do you need some help? I'd love to merge it in a future release :)

@durandj durandj force-pushed the main branch 2 times, most recently from bf3174c to 8a91c0f Compare October 4, 2023 01:30
@durandj
Copy link
Contributor Author

durandj commented Oct 4, 2023

Hey @omissis, sorry for the delay. Things have been busy so I haven't had much time to work on this. Things should be all set for review now though.

@durandj durandj changed the title [DRAFT] Add support for date/time formats Add support for date/time formats Oct 4, 2023
@omissis
Copy link
Owner

omissis commented Oct 4, 2023

hey hey! no problem at all, no worries :) I'll review in the next few days, thanks again! 💪

@omissis omissis marked this pull request as ready for review October 4, 2023 08:21
durandj and others added 8 commits October 6, 2023 00:15
The `date-time` format is used to specify that a JSON string should
conform to the ISO-8601 standard for defining timestamp strings. Here we
add the ability to generate a `time.Time` field when a date-time
formatted string is found.

https://json-schema.org/draft/2020-12/json-schema-validation.html#name-dates-times-and-duration

No extra validation logic is needed as Golang already handles time.Time
values properly when unmarshaling from JSON.
The `date` format is used to specify that a JSON string should conform
to the ISO-8601 standard for defining a date _without_ a time as a
string. In this implementation a `time.Time` value is generated given
this type of formatted string.

https://json-schema.org/draft/2020-12/json-schema-validation.html#name-dates-times-and-duration

Some extra work was required here to make this work since the
`time.Time.UnmarshalJSON` implementation expects to only be given a full
timestamps with both a date AND a time. If you gave it just "2023-01-02"
you would get an error. To get around this a new `SerializableDate` type
was added which knows to to properly marshal/unmarshal as a pure date.
This breaks a bit from how the tool has worked before because this would
now make it expected as an import in client code.

The `SerializableDate` type transparently extends the `time.Time` type
and just changes the JSON related methods.
The `time` format is used to specify that a JSON string should conform
to the ISO-8601 standard for defining a time _without_ a date as a
string. In this implementation a `time.Time` value is generated given
this type of formatted string.

https://json-schema.org/draft/2020-12/json-schema-validation.html#name-dates-times-and-duration

Some extra work was required here to make this work since the
`time.Time.UnmarshalJSON` implementation expects to only be given a full
timestamps with both a date AND a time. If you gave it just "2023-01-02"
you would get an error. To get around this a new `SerializableTime` type
was added which knows to to properly marshal/unmarshal as a pure time.
This breaks a bit from how the tool has worked before because this would
now make it expected as an import in client code.

The `SerializableTime` type transparently extends the `time.Time` type
and just changes the JSON related methods.
The `date` format is used to specify that a JSON string should conform
to the ISO-8601 standard for defining a date _without_ a time as a
string. In this implementation a `time.Time` value is generated given
this type of formatted string.

https://json-schema.org/draft/2020-12/json-schema-validation.html#name-dates-times-and-duration

Some extra work was required here to make this work since the
`time.Time.UnmarshalJSON` implementation expects to only be given a full
timestamps with both a date AND a time. If you gave it just "2023-01-02"
you would get an error. To get around this a new `SerializableDate` type
was added which knows to to properly marshal/unmarshal as a pure date.
This breaks a bit from how the tool has worked before because this would
now make it expected as an import in client code.

The `SerializableDate` type transparently extends the `time.Time` type
and just changes the JSON related methods.
@omissis omissis merged commit e7be9e2 into omissis:main Oct 5, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants