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

Support java.time.Duration and java.time.Period with examples and tests #369

Merged
merged 3 commits into from
Jun 16, 2020

Conversation

lucamolteni
Copy link
Contributor

Currently java.time.Period and java.time.Duration fields are shown in this way

openapi-period

Needless to say, this is not the correct way to provide a Duration or a Period in a REST endpoint.

This patches supports these two datatypes with this output

Screen Shot 2020-06-16 at 11 45 51

I read this shouldn't be the "canonical" way to support custom formats and that they're discussing the way to do in the specification, but as smallrye-open-api supports most of java.time classes I think it'd make sense to support all of them.

phillip-kruger
phillip-kruger previously approved these changes Jun 16, 2020
Copy link
Member

@phillip-kruger phillip-kruger left a comment

Choose a reason for hiding this comment

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

LGTM. @MikeEdgar, @EricWittmann w.d.y.t ?

@MikeEdgar
Copy link
Member

I like this, but my thoughts are that we should use standard-based naming rather than Java-based. For example, a Java Period is an ISO 8601 duration (I think - I've only looked at it today :-) ). It seems that "period" is something different that includes a start date plus the duration, so mixing terms in the "format" field might cause confusion. It might also be helpful to include links to external docs like is done for some of the other java.time formats.

In any case, I think it should be clear to the API consumer that Period is date-based, and Duration is time-based.

@MikeEdgar
Copy link
Member

I'm referring to the "Durations" and "Period" section in Appendix A of RFC 3339.

@lucamolteni
Copy link
Contributor Author

lucamolteni commented Jun 16, 2020

@MikeEdgar according to javadoc https://docs.oracle.com/javase/8/docs/api/java/time/Period.html

This class models a quantity or amount of time in terms of years, months and days. See Duration for the time-based equivalent to this class.

So yes they're the same format, but one supports Y M D and the other S etc...

Do you think it'll be better to have only one constant here?

https://github.com/smallrye/smallrye-open-api/pull/369/files#diff-9aadc53ecbf4fd8e1431e09d062da247R789

In that case I think we can't provide two different examples though...

Let me know how I can change this.

@MikeEdgar
Copy link
Member

Yes, my thought is that they should both be duration in the format. Perhaps they could have an externalDocumentation link to that RFC page with a description indicating which components are supported. I.e. a Duration is a duration that supports dur-day and dur-time components; whereas Period is a duration that supports year, month, week, and day components. The challenge is stating that and not being too verbose :-)

@lucamolteni
Copy link
Contributor Author

@MikeEdgar I pushed another commit, now we use always duration, there's a link of both the Javadoc and the RFC, and an example that is parsed from both classes.

I think this might be ok for now, as it's still better than the current implementation that prints the internals of the Duration and Period classes

Copy link
Member

@phillip-kruger phillip-kruger left a comment

Choose a reason for hiding this comment

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

LGTM. @MikeEdgar ?

Copy link
Member

@MikeEdgar MikeEdgar left a comment

Choose a reason for hiding this comment

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

Looks good - thank you @lucamolteni

@phillip-kruger phillip-kruger merged commit 9aaad76 into smallrye:master Jun 16, 2020
@phillip-kruger phillip-kruger added this to the 2.0.3 milestone Jul 15, 2020
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 this pull request may close these issues.

3 participants