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 missing value of EXTERNAL_PROPERTY type using custom NullValueProvider #3533

Closed
fxshlein opened this issue Jul 3, 2022 · 6 comments
Milestone

Comments

@fxshlein
Copy link
Contributor

fxshlein commented Jul 3, 2022

Is your feature request related to a problem? Please describe.
I am currently trying to deserialize a structure with a generic field that has an external type property, where the type property is always present, but the actual property may be absent, in which case it should just use a custom NullValueProvider.

The json can either look like this:

{
  "type": "inner-implementation",
  "value": {
    "foo": "bar"
  }
}

Or like this, in which case value should use the NullValueProvider:

{
  "type": "inner-implementation"
}

The outer type looks like this:

public class Wrapper<I extends Inner> {
    @JsonTypeInfo(use = Id.NAME, include = As.EXTERNAL_PROPERTY)
    private I value;

    public Wrapper(@JsonProperty(value = "value", required = true) I value) {
        this.value = value;
    }
}

And the inner types all implement this interface:

@JsonSubTypes({
        @Type(InnerImplementation.class),
})
public interface Inner {}

With an implementation of the inner type looking like this:

@JsonTypeName("inner-implementation")
@JsonDeserialize(using = InnerImplementationDeserializer.class)
public class InnerImplementation implements Inner {
    @Nullable
    private String foo;

    public InnerImplementation(@Nullable String foo) {
        this.foo = foo;
    }
}

Each implementation of Inner also has a deserializer which implements the getNullValue method:

class InnerImplementationDeserializer extends StdNodeBasedDeserializer<InnerImplementation> {
    protected InnerImplementationDeserializer() {
        super(TypeFactory.defaultInstance().constructType(InnerImplementation.class));
    }

    @Override
    public InnerImplementation convert(JsonNode root, DeserializationContext ctxt) throws IOException {
        JsonNode foo = root.get("foo");

        return new InnerImplementation(foo != null ? foo.textValue() : null);
    }

    @Override
    public InnerImplementation getNullValue(DeserializationContext ctxt) throws JsonMappingException {
        return new InnerImplementation(null);
    }
}

I have also disabled FAIL_ON_MISSING_EXTERNAL_TYPE_ID_PROPERTY.

The first snippet deserializes without problems, and when I remove required = true from the value property, the second snippet deserializes with Wrapper#value being null. With required = true however, a com.fasterxml.jackson.databind.exc.MismatchedInputException is thrown.

The exception originates from this part of the jackson databind codebase.

Describe the solution you'd like
In the linked snippet of jackson code, it would in my opinion be appropriate to use the custom NullValueProviders if FAIL_ON_MISSING_EXTERNAL_TYPE_ID_PROPERTY is disabled.

Usage example
The feature would work like shown above, however depending on how reasonable this approach generally is, the option FAIL_ON_MISSING_EXTERNAL_TYPE_ID_PROPERTY could be disabled by default.

Additional context
The underlying WRAPPER_ARRAY property used in the background by EXTERNAL_PROPERTY was already changed to support this behaviour: #2467

@fxshlein fxshlein added the to-evaluate Issue that has been received but not yet evaluated label Jul 3, 2022
@fxshlein
Copy link
Contributor Author

fxshlein commented Jul 3, 2022

I gave this a go here, with this my use case works fine: fxshlein@a965ec7

I have no idea what other magic this perhaps breaks or where a test for this would belong however, so any ideas there would be greatly appreciated.

@cowtowncoder
Copy link
Member

Hmmh. I was about to suggest that NullValueProvider is not used in the "absent" case, but I guess in this case it would be reasonable to use NVP if possible.

I don't think I have time in near future to look deeper into this, but I'd be happy to help with PR if someone has time to figure out if handling can be improved.
Fix could be simple, or very difficult, it unfortunately depends on how current handling is implemented. The important part is just that the deserializer for given type needs to be retrieved first, to be able to see if it provides an alternative null value (with NullValueProvider).

@fxshlein
Copy link
Contributor Author

fxshlein commented Jul 5, 2022

Fix could be simple, or very difficult, it unfortunately depends on how current handling is implemented.

Given that the ExternalTypeHandler just "fakes" an array structure, and the deserializer for EXTERNAL_PROPERTY then just extends the one for WRAPPER_ARRAY and inherits all its behavior, including #2467 (which is basically the same issue as this one, just for arrays), this is already supported by the deserializer. In the absent case, the ExternalTypeHandler just needs to not write the second field of the array, like I already tested here: fxshlein@a965ec7

@cowtowncoder
Copy link
Member

@fxshlein Ok that sounds good. Do you think you could do a PR for fix, and we get to see that unit test suite passes etc?
This would be against 2.14 branch and could make it in 2.14.0 -- I am bit hesitant to change behavior in this are for patches.

@fxshlein fxshlein mentioned this issue Aug 1, 2022
@fxshlein
Copy link
Contributor Author

fxshlein commented Aug 1, 2022

Sorry took a while to get back - created the PR!

cowtowncoder pushed a commit that referenced this issue Aug 3, 2022
@cowtowncoder cowtowncoder added 2.14 and removed to-evaluate Issue that has been received but not yet evaluated labels Aug 3, 2022
@cowtowncoder cowtowncoder added this to the 2.14.0 milestone Aug 3, 2022
@cowtowncoder cowtowncoder changed the title Deserialize value of EXTERNAL_PROPERTY type using custom NullValueProvider Deserialize missing value of EXTERNAL_PROPERTY type using custom NullValueProvider Aug 3, 2022
cowtowncoder added a commit that referenced this issue Aug 3, 2022
@cowtowncoder
Copy link
Member

Merged, will be in 2.14.0.

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