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

Fix deser of null values for collection/array type deserializers #407

Merged
merged 2 commits into from
Feb 26, 2014

Conversation

wpalmeri
Copy link

  • All of these deserializers should ask the encapsulated valueDeserializer for its nullValue when VALUE_NULL is encountered, rather than assuming a return value of null.
  • This is a bug that affects the ser/deser of Collection[Option[_]] in scala, as None values are serialized to null and deserialized back to null rather than None, which is correctly overridden in OptionDeserializer
  • Note I am a first time committer and happy to sign any release. Thanks!

…izers

* All of these deserializers should ask the encapsulated valueDeserializer for its nullValue when `VALUE_NULL` is encountered, rather than assuming a return value of `null`.
* This is a bug that affects the ser/deser of `Collection[Option[_]]` in scala, as `None` values are serialized to `null` and deserialized back to `null` rather than `None`, which is correctly overriden in `OptionDeserializer`
* Note I am a first time committer and happy to sign any release. Thanks!
@cowtowncoder
Copy link
Member

Awesome! Yes, we do need the usual CLA, see:

https://github.com/FasterXML/jackson

(and 'contributor-agreeement.pdf` listed)

Other than that, it'd be great if you could add a simple unit test; but change itself is simple enough that I don't have concern about it. Just nice to have something against regression.

@cowtowncoder
Copy link
Member

Forgot to mention: since there's no way (that I know of) to attach docs here, just email scanned document to 'info' at fasterxml.com, and we'll keep a copy. Let me know if you have questions.

unit tests showing custom null deserialization of values in arrays and maps
@wpalmeri
Copy link
Author

@cowtowncoder Just pushed some unit tests. I'll let you know when I send the the release, still getting company approval. Thanks!

@cowtowncoder
Copy link
Member

Perfect! Also note that wrt CLA, you can choose to do it just as yourself; or as employee of a company. Both work for us, and both are used (we have corporate CLAs that simplify contributions by bigger companies). So you can choose based on how it relates to your work (part of work or outside).

@wpalmeri
Copy link
Author

Just sent the signed CLA. Sorry for the delay!

wpalmeri pushed a commit to wpalmeri/jackson-module-scala that referenced this pull request Feb 25, 2014
This will ensure that the custom OptionDeserializer is always used when deserializing Options.
Right now when `List[Option[_]]` is deserialized, the Option class is treated as a "simple" type in the constructed `JavaType` and thus a generic JSON Deserializer is used rather than `OptionDeserializer`.
Note that the new test will NOT pass until a jackson-databind dependency with FasterXML/jackson-databind#407 is used
cowtowncoder added a commit that referenced this pull request Feb 26, 2014
Fix deser of null values for collection/array type deserializers
@cowtowncoder cowtowncoder merged commit 5ad9622 into FasterXML:master Feb 26, 2014
cowtowncoder added a commit that referenced this pull request Feb 26, 2014
@christophercurrie
Copy link
Member

It doesn't look like this patch made it into 2.3.2, correct?

@cowtowncoder
Copy link
Member

@christophercurrie It should have made it in 2.3.2.

@christophercurrie
Copy link
Member

I might be missing something, because I don't see the relevant commits on the databind 2.3 branch, and the test for FasterXML/jackson-module-scala#132 does not pass when I merge it into the 2.3 branch there.

I'll hold off releasing 2.3.2 of the Scala module in case I've missed something critical.

@cowtowncoder
Copy link
Member

No, you are right. I did not consider the part of backporting; I do merge my own fixes, but for pull requests usually ask submitter. But not here.

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