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

Deserialize ArrayItems and JsonValueFormat correctly #81

Merged
merged 11 commits into from Oct 7, 2015

Conversation

georgewfraser
Copy link
Contributor

No description provided.

@Override
public Items deserialize(JsonParser parser,
DeserializationContext context) throws IOException, JsonProcessingException {
ObjectCodec mapper = parser.getCodec();
Copy link
Member

Choose a reason for hiding this comment

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

I assume it is legal to use array, or a single item here?
This works, I can probably simplify it after merge a bit.

@cowtowncoder
Copy link
Member

I think I'd like to know bit more about what exactly is being fixed and why. I can see that handling of JsonValueFormat is changed, but not sure why so: default Enum serializer would use upper-case name(), new code would expect to use lower-case desc. Is that something JSON Schema specification mandates?

If the change is needed, there are also ways to change definition of JsonValueFormat, which is bit simpler: for example, adding @JsonValue to toString() method. This seems preferable, as it should support both serialization and deserialization.

Handling of item array makes sense, but it would be good to have unit test(s) to show how things work better now.

@georgewfraser
Copy link
Contributor Author

JsonValueFormat should be serializing and deserializing using the lower-snake-case values. These values are part of the JSON schema spec. If you run this example https://gist.github.com/georgewfraser/196c84c5072a800e9cb9 through a JSON schema validator, for example http://jsonschemalint.com/draft4/#, you will see that "date-time" is a real format that gets validated, but DATE_TIME is meaningless.

I added some tests, and fixed some existing tests.

@cowtowncoder
Copy link
Member

And just to make sure, lower-snake-case is what JSON Schema v3 defines? (since that's what module supports). I know v4 has changed many things, and this has caused lots of confusion.

@georgewfraser
Copy link
Contributor Author

Yes, all versions use lower-snake-case.

cowtowncoder added a commit that referenced this pull request Oct 7, 2015
Deserialize ArrayItems and JsonValueFormat correctly
@cowtowncoder cowtowncoder merged commit ccb1061 into FasterXML:master Oct 7, 2015
@cowtowncoder
Copy link
Member

Ok thanks. Just wanted to verify.

@cowtowncoder
Copy link
Member

Ok, merged, will do some minor cleanup.

One thing I did not, wrt JsonFormatValues, as per:

http://json-schema.org/latest/json-schema-validation.html#anchor104

that existing list does seem to fully align with that; and (for example), whereas we have "host-name", spec seems to have "hostname". Similarly with "ip-address" vs "ipv4". This could be some difference between v3 of json schema vs later verisons.

I guess I won't be changing that quite yet, but thought I'd note it.
Also, use of enum for format values does seem bit wrong, wish I had known to bring it up when functionality was first merged (whole JSON Schema handling is contribution, I did not write it).

@georgewfraser
Copy link
Contributor Author

What is your plan for moving to v4? I'm fixing some v3/v4 differences on https://github.com/fivetran/jackson-module-jsonSchema

On Oct 7, 2015, at 10:58 AM, Tatu Saloranta notifications@github.com wrote:

Ok, merged, will do some minor cleanup.

One thing I did not, wrt JsonFormatValues, as per:

http://json-schema.org/latest/json-schema-validation.html#anchor104

that existing list does seem to fully align with that; and (for example), whereas we have "host-name", spec seems to have "hostname". Similarly with "ip-address" vs "ipv4". This could be some difference between v3 of json schema vs later verisons.

I guess I won't be changing that quite yet, but thought I'd note it.
Also, use of enum for format values does seem bit wrong, wish I had known to bring it up when functionality was first merged (whole JSON Schema handling is contribution, I did not write it).


Reply to this email directly or view it on GitHub.

cowtowncoder added a commit that referenced this pull request Oct 7, 2015
… proper ser/deser for JsonValueFormat in databind, for 2.7
@cowtowncoder
Copy link
Member

No solid plan. Some discussion on dev list (and with some developers). I think it is easiest to do via new module, instead of trying to add modality; similar to how Hibernate module (v3, v4, v5) works.
One open question there would be whether to do Maven sub-projects/modules, or separate git repos.
I am guessing separate repos would be better eventually, considering that v3 should eventually be deprecated.

It's probably best to discuss this further on jackson-dev list; not all/many developers read issue updates on regular basis.

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.

None yet

2 participants