Support @JsonFormat annotation for date-time fields #643

shravanspurohit · 2016-11-07T03:24:21Z

Adds @jsonformat annotation to specified property. An example of the jsonschema is:

{
    "type" : "object",
    "properties" : {
        "stringAsJavaDate" : {
            "type" : "string",
            "format" : "date-time",
            "customDateTimePattern" : "yyyy-MM-dd'T'HH:mm:ss.SSSZ",
            "timezone" : "PST"
        },
        "stringAsJodaDateTime" : {
            "javaType" : "org.joda.time.DateTime",
            "format" : "date-time",
            "customDateTimePattern" : "yyyy-MM-dd'T'HH:mm:ss.SSSZ",
            "timezone" : "PST"
        }
    }
}

This PR is not ready to be merged. I created this to get feedback. I am still trying to get the integration tests to work.

gaba-xyz · 2016-11-07T08:59:48Z

I think it looks good.

javaTypeseems like a redundant property as there is a setting for formatting dates as JodaTime DateTime already.

Also as @joelittlejohn mentioned earlier, if we want to conform better to the latest JSON schema spec, then any property with format: "date-time" should be a RFC3339 date-time formatted string. So the addition of a customDateTimePattern will mean that you will be able to define schemas that violate the date formatting invariant.

For me personally at least I think the generated POJOs should validate against any JSON schema validator that follows the official JSON schema validation specs when serialised.

shravanspurohit · 2016-11-09T02:56:28Z

@OverlyExcessive are you suggesting that I need to validate the pattern we receive in customDateTimePattern to ensure it corresponds to RFC3339 format?

gaba-xyz · 2016-11-10T08:18:54Z

@shravanspurohit I'm suggesting that we get rid of the customDateTimePattern property as it is not a part of the official JSON Schema spec, if the date-time format is used then according to the latest JSON schema spec the provided value must be a RFC3339 formatted date-time.

shravanspurohit · 2016-11-11T06:11:36Z

@OverlyExcessive I am a bit concerned with this. Here is why.

Firstly, this will break a lot of things for a lot of people. Right now, I can extend the ObjectMapper class and set the date format to anything I want.

public class CustomObjectMapper extends ObjectMapper {
    public CustomObjectMapper(){
        setDateFormat(new SimpleDateFormat("yyyy-MM-dd")); // Set any format here
        configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false);
    }
}

And I can register this class as a json provider on my server. The date format you set here would be the global configuration which applies to all date-time fields.
With the change you are proposing, when we use date-time in the schema, if we by default include the @JsonFormat annotation on the field and use the RFC3339 date-time format, this configuration will take precedence and the the format set in the CustomObjectMapper would not be honored and there by breaking existing things.

Secondly, people are using @jsonformat annotation to use custom format. I personally turned to this because I had to change all my (Date) fields to include date and time except the dob field where I wanted just the date. Since I was working with legacy code, changing the date format in the CustomObjectMapper to yyyy-MM-dd'T'HH:mm:ss.SSSZ and using @JsonFormat(pattern='yyyy-MM-dd') for my dob field was basically a 2 line code change.
I do agree that we want to follow json schema 4 specs but if you look at Jackson's documentation http://wiki.fasterxml.com/JacksonFAQDateHandling you will see that they introduced @jsonformat specifically to allow us to use whatever format we want.

What I would like to do is figure out a way in which jsonschema2pojo can use the full extent of capabilities Jackson provides.

shravanspurohit · 2016-11-17T01:23:08Z

What are your thoughts on this, @joelittlejohn ?

joelittlejohn · 2016-11-17T11:32:31Z

There isn't an easy answer here, but here's what I'm thinking right now:

I'd like this tool to (by default) do the right thing and create valid documents according to the rules laid out by the JSON Schema validation spec. Just as an aside, I'd also like this tool to support and encourage using the best and ubiquitous date format with no effort. We have a legacy here though and a backwards-compatibility problem - at the very least we should give people a way to keep the old behaviour.

I think the best route forward would be:

By default, add the JsonFormat annotation when "format":"date-time" appears. This should use yyyy-MM-dd'T'HH:mm:ss.SSSZ.
It should be possible to disable this by setting <formatDatetimes>false</formatDatetimes> in the plugin configuration.
customDateFormat schema rule should be supported, and can be used if you really want to override the format and create data that isn't strictly valid according to the spec.
customTimeZone schema rule should be supported, and can be used if want to override the timezone in which the instant is rendered.

shravanspurohit · 2016-11-18T04:56:08Z

I like this solution. So this is what I understood. Correct me if I am wrong.

If formatDateTimes is set to true we will add the @JsonFormat property with pattern as yyyy-MM-dd'T'HH:mm:ss.SSSZ and use the customDateFormat if provided.

If formatDateTimes is set to false we will NOT add the @JsonFormat property with pattern as yyyy-MM-dd'T'HH:mm:ss.SSSZ by default. However, if the customDateFormat is provided, we will add the @JsonFormat to that field.

Basically, the setting formatDateTimes controls whether we add @JsonFormat with pattern yyyy-MM-dd'T'HH:mm:ss.SSSZ as default or not.

Regarding the implementation, I have 2 options.

In Jackson2Annotator class, I include a variable named GenerationConfig and initialize this when the annotator is instantiated.
In Jackson2Annotator class, for method jsonFormat, I pass in the GenerationConfig as a parameter since this isnt being used anywhere else. The caveat being we might have other annotations in future which might require the GenerationConfig too. But agile says dont plan for a future unknown :-)

gaba-xyz · 2016-12-07T10:57:57Z

@shravanspurohit For me I would say either way is fine. Would be nice if we could move this PR along, let me know if you need any assistance.

joelittlejohn · 2016-12-07T13:52:29Z

@shravanspurohit I've made a change recently to ensure that Annotators receive the generation config, and that any custom annotator that has a constructor that takes a generation config will also received it.

joelittlejohn:master -> shravanspurohit:master

Todo: Integration tests

Integration tests implemented Fixed groovy exception. Updated comments. Updated ant documentation

shravanspurohit · 2016-12-11T06:27:46Z

@joelittlejohn @OverlyExcessive Its done!

joelittlejohn

I've tried to do a detailed review here. There's a few things to fix. Thanks!

joelittlejohn · 2016-12-11T22:06:47Z

jsonschema2pojo-ant/src/main/java/org/jsonschema2pojo/ant/Jsonschema2PojoTask.java

+     * Sets the 'formatDateTime' property of this class
+     *
+     * @param formatDateTime
+     *            Whether the fields of type `date-type` have the `@JsonFormat` annotation 


I think date-type should be date-time. Also, I think the backticks should be <code></code> (?).

joelittlejohn · 2016-12-11T22:08:32Z

jsonschema2pojo-ant/src/main/java/org/jsonschema2pojo/ant/Jsonschema2PojoTask.java

@@ -140,6 +140,8 @@
    private String timeType = null;

    private String dateType = null;
+
+    private boolean formatDateTime = false;


Could you rename this config option to formatDateTimes? (plural) This fits better with the conventions in this project and also describes this feature better I think.

joelittlejohn · 2016-12-11T22:09:33Z

jsonschema2pojo-ant/src/site/Jsonschema2PojoTask.html

@@ -301,6 +301,12 @@
        </td>
        <td align="center" valign="top">No</td>
      </tr>
+      <tr>
+        <td valign="top">formatDateTime</td>
+        <td valign="top">Whether the fields of type <code>date-type</code> have the <code>@JsonFormat</code> annotation with pattern set to the default value of <code>yyyy-MM-dd'T'HH:mm:ss.SSS</code> and timezone set to default value of <code>UTC</code>


date-type should be date-time I think

joelittlejohn · 2016-12-11T22:13:02Z

jsonschema2pojo-core/src/main/java/org/jsonschema2pojo/Annotator.java

+     * @param propertyNode
+     *            the schema node defining this property
+     */
+    void jsonFormat(JFieldVar field, JDefinedClass clazz, String propertyName, JsonNode node);


The implementation of this method doesn't seem to use clazz at all, could you remove this from the signature?

This method needs a new name I think, jsonFormat doesn't describe what this does, it's too general. I think a good name for this method (that fits with the other methods in this interface) would be: dateField.

The implementation of this method doesn't seem to use propertyName at all, could you remove this from the signature?

joelittlejohn · 2016-12-11T22:15:44Z

jsonschema2pojo-cli/src/main/java/org/jsonschema2pojo/cli/Arguments.java

@@ -161,7 +161,10 @@

    @Parameter(names = { "-ida", "--include-dynamic-accessors" }, description = "Include dynamic getter, setter, and builder support on generated types.")
    private boolean includeDynamicAccessors = false;
-
+
+    @Parameter(names = { "-fdt", "--format-date-time" }, description = "Whether the fields of type `date-type` have the `@JsonFormat` annotation with pattern set to the default value of `yyyy-MM-dd'T'HH:mm:ss.SSS` and timezone set to default value of `UTC`")


s/date-type/date-time/

joelittlejohn · 2016-12-11T22:26:04Z

jsonschema2pojo-core/src/main/java/org/jsonschema2pojo/Jackson2Annotator.java

+        boolean formatDateTime = generationConfig.isFormatDateTime();
+        String iso8601DateTimeFormat = "yyyy-MM-dd'T'HH:mm:ss.SSS";
+        String customDateTimePattern = node.has("customDateTimePattern") == true ? node.get("customDateTimePattern").asText() : null;
+        String timezone = node.has("customTimezone") == true ? node.get("customTimezone").asText() : "UTC";


No need for == true

joelittlejohn · 2016-12-11T22:33:04Z

jsonschema2pojo-core/src/main/java/org/jsonschema2pojo/rules/PropertyRule.java

@@ -134,6 +137,13 @@ private void propertyAnnotations(String nodeName, JsonNode node, Schema schema,
            ruleFactory.getNotRequiredRule().apply(nodeName, node.get("required"), generatedJavaConstruct, schema);
        }
    }
+
+    private void formatAnnotation(JFieldVar field, JDefinedClass clazz, String propertyName, JsonNode node) {
+    	String format = node.has("format") == true ? node.get("format").asText() : null;


I'd simplify this to

String format = node.path("format").asText();

joelittlejohn · 2016-12-11T22:35:56Z

...-integration-tests/src/test/java/org/jsonschema2pojo/integration/CustomDateTimeFormatIT.java

+            assertEquals(format, annotation.pattern());
+            // Assert that the timezones match
+            assertEquals(timezone, annotation.timezone());
+        }


This project uses the following style for if:

if (...) { } else { }

Could you follow this? Thanks

joelittlejohn · 2016-12-11T22:43:36Z

...-integration-tests/src/test/java/org/jsonschema2pojo/integration/CustomDateTimeFormatIT.java

+import com.fasterxml.jackson.databind.node.ObjectNode;
+
+@RunWith(Parameterized.class)
+public class CustomDateTimeFormatIT {


I appreciate that you're trying to minimize duplication here, but this test is really hard to follow and maintain. The test itself has configurable behaviour and assertions, and it's simply very hard to see that examples are being tested.

Could you refactor this so that it not a parameterized test? These tests are good when theres is a large body of test data, but they're not good for readability and maintainability when they're being used to create a single test with many different switchable code paths.

Each example should have a clear, descriptive method (test) name. If there's duplication then you can add some helper methods to the test.

Yeah, I felt the same but I decided to optimize it for performance. I'll make it more readable instead.

joelittlejohn · 2016-12-11T22:44:01Z

jsonschema2pojo-maven-plugin/src/main/java/org/jsonschema2pojo/maven/Jsonschema2PojoMojo.java

@@ -542,6 +542,17 @@
     * @readonly
     */
    private MavenProject project;
+
+    /**
+     * Whether the fields of type `date-type` have the `@JsonFormat` annotation 


s/date-type/date-time/

shravanspurohit · 2016-12-13T03:37:54Z

@joelittlejohn Done.

shravanspurohit mentioned this pull request Nov 7, 2016

Add @JsonFormat annotation if date-pattern specified #380

Closed

shravanspurohit and others added 3 commits December 10, 2016 18:43

Merge pull request #1 from joelittlejohn/master

127f834

joelittlejohn:master -> shravanspurohit:master

Implemented feature.

bac916a

Todo: Integration tests

Fixed jdk 7 compile issue

870fe7b

shravanspurohit force-pushed the custom_date_format branch 2 times, most recently from bcc9f1e to 9fea42a Compare December 11, 2016 06:05

Added support for config option

2e18f83

Integration tests implemented Fixed groovy exception. Updated comments. Updated ant documentation

shravanspurohit force-pushed the custom_date_format branch from 9fea42a to 2e18f83 Compare December 11, 2016 06:19

joelittlejohn requested changes Dec 11, 2016

View reviewed changes

Addressed code review comments

e37b4ce

joelittlejohn approved these changes Dec 13, 2016

View reviewed changes

joelittlejohn merged commit 9f5ca13 into joelittlejohn:master Dec 13, 2016

joelittlejohn added this to the 0.4.29 milestone Dec 13, 2016

joelittlejohn changed the title ~~Support for @JsonFormat annotation~~ Support @JsonFormat annotation for date-time fields Dec 13, 2016

unkish mentioned this pull request Dec 15, 2021

Property with type string and format date-time should output ISO8601 Date instead of Timestamp #247

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support @JsonFormat annotation for date-time fields #643

Support @JsonFormat annotation for date-time fields #643

shravanspurohit commented Nov 7, 2016

gaba-xyz commented Nov 7, 2016 •

edited

Loading

shravanspurohit commented Nov 9, 2016

gaba-xyz commented Nov 10, 2016

shravanspurohit commented Nov 11, 2016

shravanspurohit commented Nov 17, 2016

joelittlejohn commented Nov 17, 2016

shravanspurohit commented Nov 18, 2016

gaba-xyz commented Dec 7, 2016

joelittlejohn commented Dec 7, 2016

shravanspurohit commented Dec 11, 2016

joelittlejohn left a comment

joelittlejohn Dec 11, 2016

joelittlejohn Dec 11, 2016

joelittlejohn Dec 11, 2016

joelittlejohn Dec 11, 2016

joelittlejohn Dec 11, 2016

joelittlejohn Dec 11, 2016

joelittlejohn Dec 11, 2016

joelittlejohn Dec 11, 2016

joelittlejohn Dec 11, 2016

joelittlejohn Dec 11, 2016

shravanspurohit Dec 13, 2016

joelittlejohn Dec 11, 2016

shravanspurohit Dec 13, 2016

joelittlejohn Dec 11, 2016

shravanspurohit commented Dec 13, 2016

Support @JsonFormat annotation for date-time fields #643

Support @JsonFormat annotation for date-time fields #643

Conversation

shravanspurohit commented Nov 7, 2016

gaba-xyz commented Nov 7, 2016 • edited Loading

shravanspurohit commented Nov 9, 2016

gaba-xyz commented Nov 10, 2016

shravanspurohit commented Nov 11, 2016

shravanspurohit commented Nov 17, 2016

joelittlejohn commented Nov 17, 2016

shravanspurohit commented Nov 18, 2016

gaba-xyz commented Dec 7, 2016

joelittlejohn commented Dec 7, 2016

shravanspurohit commented Dec 11, 2016

joelittlejohn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shravanspurohit commented Dec 13, 2016

gaba-xyz commented Nov 7, 2016 •

edited

Loading