Skip to content

StdDelegatingDeserializer ignores nullValue of _delegateDeserializer. #3741

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

Closed
k163377 opened this issue Jan 18, 2023 · 2 comments
Closed

StdDelegatingDeserializer ignores nullValue of _delegateDeserializer. #3741

k163377 opened this issue Jan 18, 2023 · 2 comments
Milestone

Comments

@k163377
Copy link
Contributor

k163377 commented Jan 18, 2023

Describe the bug
StdDelegatingDeserializer does not properly override methods such as getNullValue.
This prevents Converter from working when the value is null.

Version information
2.14.1

To Reproduce
I think I can read it from the source code, but I will write it if necessary.
https://github.com/FasterXML/jackson-databind/blob/ae3ca887c589e075ed13ea0665ac9bf22c0517a9/src/main/java/com/fasterxml/jackson/databind/deser/std/StdDelegatingDeserializer.java

Expected behavior
override a method such as getNullValue and call Converter if the result of such a method call to _delegateDeserializer is not null.

Additional context
There was a situation where the value class deserialization in jackson-module-kogera needed to be enabled.
https://github.com/ProjectMapK/jackson-module-kogera

@cowtowncoder
Copy link
Member

@k163377 yes, I think you are right wrt delegation, methods are missing. And I suppose calling of Converter makes sense as well. There are a few other methods missing too so it'd make sense to address those as well.

I think this would qualify to be in 2.14(.2).

I can probably work on this, time permitting, to get it in 2.14.2. Or if you want to do PR, happy to review. Methods from JsonSerializer that seem to be missing:

  • isCachable
  • getKnownPropertyNames
  • unwrappingDeserializer (may need to re-construct delegating one itself)
  • replaceDelegatee (not sure this is actually used anywhere but...)
  • getNullValue / getNullAccessPattern
  • getAbsentValue
  • getEmptyValue / getEmptyAccessPattern
  • getObjectIdReader
  • findBackReference

So I think all of those should delegate. Although one way to reduce risk would be to only delegate getXxxValue()/getXxxAccessPattern() calls for 2.14(.2), and the rest in 2.15.
Although I am not sure why delegation should break anything it might still be safest.

Ideally we'd also have unit tests for these which adds to work.

@cowtowncoder
Copy link
Member

cowtowncoder commented Jan 22, 2023

Actually, I think overrides may or may not make sense so need to check on case by case basis: pure delegating only via DelegatingDeserializer: StdDelegatingDeserializer should really be called [Std]ConvertingDeserializer instead due to conversion step. This conversion step also means that some delegation may either not make sense or cannot be handled in general manner.

EDIT: and in fact 3.0 does that.

@cowtowncoder cowtowncoder added 2.14 and removed to-evaluate Issue that has been received but not yet evaluated labels Jan 22, 2023
@cowtowncoder cowtowncoder added this to the 2.14.2 milestone Jan 22, 2023
cowtowncoder added a commit that referenced this issue Jan 22, 2023
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

No branches or pull requests

2 participants