search

FasterXML / jackson-modules-java8

Set of support modules for Java 8 datatypes (Optionals, date/time) and features (parameter names)
Apache License 2.0
399 stars 116 forks source link

Change InstantSerializerBase to format dates in the same way as DateTimeSerializerBase #255

Closed rotilho closed 1 year ago

rotilho commented 1 year ago

This fix an issue I have with jsonSchema where the schema generated for Dates differ from the Java Time ones.

I believe this also may affect other applications that relies on jackson formats.

cowtowncoder commented 1 year ago

Ok, first of all: thank you for contribution!

Second, a couple of requests:

  1. Please add actual description (seems like this would be for Swagger generation or something)?
  2. Any chance of a unit test? (I don't remember if schema generation aspects have existing tests -- if none then that's ok but ideally changes would be checked by a test)
rotilho commented 1 year ago

@cowtowncoder thanks for reviewing it, I just added a description.

Regarding the tests I couldn't find any similar tests or way where schema can be validate. It's my first time debugging the Jackson internals so I may just missing something obvious.

If you have an example somewhere I will be happy to add the unit tests.

cowtowncoder commented 1 year ago

Ok thank you for adding description!

One thing I was wondering was whether there might be backwards-compatibility concerns, but it looks like you are only adding a new property, not changing. So maybe that is fine. But definitely needs to go in 2.15 not 2.14.x patch.

As to tests, there is existing test class:

datetime/src/test/java/com/fasterxml/jackson/datatype/jsr310/misc/DateTimeSchemasTest.java

which I hope helps. It doesn't have to be anything big, just verify the fix in a minimal way (or not minimal if you prefer :) ).

rotilho commented 1 year ago

It makes sense releasing it just in 2.15.

Thanks for the example, I just push a change in the existing tests for ZonedDateTime to cover the schema change

cowtowncoder commented 1 year ago

Excellent, thank you!

One last thing: if I haven't yet asked for and received (apologies if I have -- this only needs to be done once for any and all contributions) Contributor License Agreement (CLA), I'd need it. It's here:

https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf

and is usually easiest to print, fill & sign, scan/photo, email to info at fasterxml dot com. Once I have that (and like I said it's needed just once and I'll keep a copy for future PRs), will be happy to merge this in!

Thank you once again for the contribution, looking forward to merging it!