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

Incorrect target type when disabling coercion, trying to deserialize String from Array/Object #3924

Closed
joca-bt opened this issue May 12, 2023 · 6 comments · Fixed by #3962
Closed
Labels
2.16 Issues planned for 2.16 good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project
Milestone

Comments

@joca-bt
Copy link

joca-bt commented May 12, 2023

When disabling coercions and providing a wrong type on an array element, the error message is specifying that we are trying to coerce the type of the array when it should mention we are trying to coerce the type of the element of the array. See the example below. This is another instance of #3690.

// Test class.
public static class Input<T> {
    private T field;

    public T getField() {
        return field;
    }

    public void setField(T field) {
        this.field = field;
    }
}

// Building a strict ObjectMapper.
ObjectMapper objectMapper = JsonMapper.builder()
    .withCoercionConfigDefaults(config -> {
        config.setCoercion(CoercionInputShape.Boolean, CoercionAction.Fail)
            .setCoercion(CoercionInputShape.Integer, CoercionAction.Fail)
            .setCoercion(CoercionInputShape.Float, CoercionAction.Fail)
            .setCoercion(CoercionInputShape.String, CoercionAction.Fail)
            .setCoercion(CoercionInputShape.Array, CoercionAction.Fail)
            .setCoercion(CoercionInputShape.Object, CoercionAction.Fail);
    })
    .build();
TypeFactory typeFactory = objectMapper.getTypeFactory();

// Test.
JavaType arrayType = typeFactory.constructParametricType(List.class, String.class);
JavaType inputType = typeFactory.constructParametricType(Input.class, arrayType);
try {
    // Returns class java.lang.String + VALUE_NUMBER_INT -> correct.
    objectMapper.readValue("{ \"field\": [ 1 ] }", inputType);
    // Returns class java.util.ArrayList + START_ARRAY -> was expecting target type to be String.
    objectMapper.readValue("{ \"field\": [ [ 1 ] ] }", inputType);
    // Returns class java.util.ArrayList + START_OBJECT -> was expecting target type to be String.
    objectMapper.readValue("{ \"field\": [ { \"field\": 1 } ] }", inputType);
} catch (MismatchedInputException exception) {
    JsonParser parser = (JsonParser) exception.getProcessor();
    System.out.println(exception.getTargetType());
    System.out.println(parser.currentToken());
}

// A truly strict String parser.
// If we register this deserializer in the ObjectMapper above we will then get the expected target type.
public class StringDeserializer extends JsonDeserializer<String> {
    @Override
    public String deserialize(JsonParser parser, DeserializationContext context) throws IOException {
        if (!parser.hasToken(VALUE_STRING)) {
            throw context.wrongTokenException(parser, String.class, VALUE_STRING, null);
        }

        return parser.getText();
    }
}

This is our test suite for JSON strict parsing which we have used to open many such issues in the past, in case you want to take inspiration for Jackson tests.
Test.zip

@joca-bt joca-bt added the to-evaluate Issue that has been received but not yet evaluated label May 12, 2023
@cowtowncoder cowtowncoder added 2.16 Issues planned for 2.16 and removed to-evaluate Issue that has been received but not yet evaluated labels May 22, 2023
@cowtowncoder
Copy link
Member

Sounds like there is missing handling indeed, thank you for reporting this @joca-bt!

Unfortunately I don't think I have time to investigate this further right now; but I hope someone else could check it out; there should be enough information to dig into it.

Change itself should probably go in 2.16 branch since it changes exception messages that are part of existing behavior (even if fixing sub-optimal message).

@cowtowncoder cowtowncoder added the good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project label May 22, 2023
@JooHyukKim
Copy link
Member

I wrote a solution locally and the result seems reasonable.

May I ask you to review the error messages below and see if they are the expected ones before I make a PR, @joca-bt @cowtowncoder ?

Case : "{"field": [ 1 ] }"

Cannot coerce Integer value (1) to `java.lang.String` value (but could if coercion was enabled using `CoercionConfig`)
 at [Source: UNKNOWN; line: 1, column: 14] (through reference chain: com.fasterxml.jackson.databind.convert.DisableCoercions3690Test$Input3924["field"]->java.util.ArrayList[0])

Case : "{ "field": [ [ 1 ] ] }"

Cannot coerce Array value ([) to `java.lang.String` value (but could if coercion was enabled using `CoercionConfig`)
 at [Source: UNKNOWN; line: 1, column: 14] (through reference chain: com.fasterxml.jackson.databind.convert.DisableCoercions3690Test$Input3924["field"]->java.util.ArrayList[0])

Case : "{ "field": [ { "field": 1 } ] }"

Cannot coerce Object value ({) to `java.lang.String` value (but could if coercion was enabled using `CoercionConfig`)
 at [Source: UNKNOWN; line: 1, column: 14] (through reference chain: com.fasterxml.jackson.databind.convert.DisableCoercions3690Test$Input3924["field"]->java.util.ArrayList[0])

@cowtowncoder
Copy link
Member

@JooHyukKim That looks good to me; coercion failure happening at element level, not for Array/List.

@JooHyukKim
Copy link
Member

@cowtowncoder Thank you for the feedback! 🙏🏼🙏🏼 will make PR soon

@joca-bt
Copy link
Author

joca-bt commented Jun 1, 2023

For me in particular the message is irrelevant, we care about the exception fields having the right type, since we rely on that and not the messages.

@JooHyukKim
Copy link
Member

For me in particular the message is irrelevant, we care about the exception fields having the right type, since we rely on that and not the messages.

@joca-bt Great point 👍🏻 I added assertion like below(or check the test I made on PR), does this prove to support your usecase?

 assertEquals(String.class, e.getTargetType());

@cowtowncoder cowtowncoder changed the title Incorrect target type when disabling coercion Incorrect target type when disabling coercion, deserializing Array Jun 5, 2023
@cowtowncoder cowtowncoder changed the title Incorrect target type when disabling coercion, deserializing Array Incorrect target type when disabling coercion, trying to deserialize String from Array/Object Jun 5, 2023
@cowtowncoder cowtowncoder added this to the 2.16.0 milestone Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.16 Issues planned for 2.16 good first issue Issue that seems easy to resolve and is likely a good candidate for contributors new to project
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants