Skip to content

Use a stricter date-time attribute formatter(rfc3339) #235

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

Merged
merged 2 commits into from
Jul 10, 2017

Conversation

tvdinh
Copy link
Contributor

@tvdinh tvdinh commented Jun 11, 2017

Current date-time attribute formatter does not exactly comply with RFC3339 (required by Swagger specs). This PR provides a stricter one.

Note: using java.time lib, available in Java 8 only.

@huggsboson
Copy link
Member

thanks for the patch set! I'm happy to drop compatibility for 1.6 but hesitant to drop 1.7 without info on %-age of people are using 1.7 vs 1.8. Would it be hard to make this compatible with joda or some other lib?

@tvdinh
Copy link
Contributor Author

tvdinh commented Jun 12, 2017

Should not be that hard. I'll look it to it. Thanks for the feedback.

@tvdinh
Copy link
Contributor Author

tvdinh commented Jun 13, 2017

Tried using joda-time. It looks straight-forward except for the Offset. RFC3339 strictly requires ":" between hours and minutes (HH:mm). Eventhough the javadoc of

org.joda.time.format.DateTimeFormatterBuilder#appendTimeZoneOffset(String, boolean, init, int)

clearly states if the boolean is set to true, it will include the ":"

     * @param showSeparators  if true, prints ':' separator before minute and
     * second field and prints '.' separator before fraction field.

The formatter still eventually considers this valid:

2012-12-02T13:05:00+0100

This is the only failing test case left.

Will have to take another look.

@tvdinh
Copy link
Contributor Author

tvdinh commented Jun 14, 2017

Used joda-time lib instead of java.time to make it java 7 compatible.

@huggsboson
Copy link
Member

sorry for the delay!

@huggsboson
Copy link
Member

I'm super happy about this patchset as it brings the library closer to conformance. The thing that makes me sad is that we may have current users who are depending on the old behavior and I'm torn on breaking them with no path forward other than freezing their version. Still debating the right move in my head.

@huggsboson
Copy link
Member

The current behavior is probably actually more disruptive than the fixed behavior for clients. If a user needs the old behavior they should come up with a way to inject a Dictionary so they can use the old behavior.

@huggsboson huggsboson merged commit 728b3da into java-json-tools:master Jul 10, 2017
@tvdinh
Copy link
Contributor Author

tvdinh commented Jul 10, 2017 via email

@huggsboson
Copy link
Member

@tvdinh I'm gonna have to change the default back to the old DateTimeAttribute as it is breaking adoption by other users that were relying on the bad spec. At some point I'll do a major version bump and let them fix their stuff but I'm not there at the moment.

You should be able to use your more specific validator by following Example 8 and configuring your Attribute (still in the JAR) for the Library in the ValidationCfg.

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.

2 participants