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

Inconsistent behaviour with nulls for records #3847

Open
joca-bt opened this issue Mar 28, 2023 · 14 comments
Open

Inconsistent behaviour with nulls for records #3847

joca-bt opened this issue Mar 28, 2023 · 14 comments

Comments

@joca-bt
Copy link

joca-bt commented Mar 28, 2023

Forbidding nulls works correctly with classes but doesn't with records. Moreover, the behaviour with records changes if we use @JsonProperty.

Let's take the following ObjectMapper definition:

ObjectMapper objectMapper = JsonMapper.builder()
    .defaultSetterInfo(JsonSetter.Value.construct(Nulls.FAIL, Nulls.FAIL))
    .withCoercionConfigDefaults(config -> config.setCoercion(CoercionInputShape.String, CoercionAction.Fail))
    .build();

When testing nulls with a class everything works as expected:

public class Test {
    private String fieldName;

    public void setFieldName(String fieldName) {
        this.fieldName = fieldName;
    }
}

objectMapper.readValue("{ \"fieldName\": \"value\" }", Test.class); // ok (no exception is thrown)
objectMapper.readValue("{ \"fieldName\": null }", Test.class); // ok, throws InvalidNullException as expected
objectMapper.readValue("{ }", Test.class); // ok (no exception is thrown)

When testing the same class but adding @JsonProperty to it everything still works as expected:

public class Test {
    private @JsonProperty("field_name") String fieldName;

    public void setFieldName(String fieldName) {
        this.fieldName = fieldName;
    }
}

objectMapper.readValue("{ \"field_name\": \"value\" }", Test.class); // ok (no exception is thrown)
objectMapper.readValue("{ \"field_name\": null }", Test.class); // ok, throws InvalidNullException as expected
objectMapper.readValue("{ }", Test.class); // ok (no exception is thrown)

When testing nulls with a record null hating doesn't seem to be working:

public record Test(String fieldName) {}

objectMapper.readValue("{ \"fieldName\": \"value\" }", Test.class); // ok (no exception is thrown)
objectMapper.readValue("{ \"fieldName\": null }", Test.class); // not ok, was expecting a InvalidNullException to be thrown but it wasn't
objectMapper.readValue("{ }", Test.class); // ok (no exception is thrown)

When testing the same record but adding @JsonProperty to it null hating starts working but something else breaks:

public record Test(@JsonProperty("field_name") String fieldName) {}

objectMapper.readValue("{ \"field_name\": \"value\" }", Test.class); // ok (no exception is thrown)
objectMapper.readValue("{ \"field_name\": null }", Test.class); // ok, throws InvalidNullException as expected
objectMapper.readValue("{ }", Test.class); // not ok, throws InvalidNullException when it shouldn't
@joca-bt joca-bt added the to-evaluate Issue that has been received but not yet evaluated label Mar 28, 2023
@JooHyukKim
Copy link
Member

JooHyukKim commented Mar 28, 2023

Issues #2974 #2970 might answer your question.

Btw, I failed to recreate the "Inconsistent behavior" at this PR #3848. Could you check again your test cases, @joca-bt please? I will tag you.

@joca-bt
Copy link
Author

joca-bt commented Mar 28, 2023

Here's a simple project replicating it.

test2.zip

@JooHyukKim
Copy link
Member

JooHyukKim commented Mar 29, 2023

Here's a simple project replicating it.
test2.zip

Can you post github link instead and delete comment with zip file link? For security concerns. Or even better check the PR #3848 I mentioned.

@joca-bt
Copy link
Author

joca-bt commented Mar 29, 2023 via email

@yihtserns
Copy link
Contributor

@joca-bt From the attached project, I see that you're using version 2.14.2. I think this issue has been fixed in 2.15.0 by #3724, which is why #3848 wasn't able to reproduce the issue.

You can verify this by testing with com.fasterxml.jackson.core:jackson-databind:2.15.0-rc2 (don't forget to also upgrade any other Jackson dependency to the same version, to avoid any unnecessary problems).

@cowtowncoder cowtowncoder removed the to-evaluate Issue that has been received but not yet evaluated label Apr 6, 2023
@cowtowncoder
Copy link
Member

@joca-bt if you are able to verify this against 2.15.0-rc2 please let us know so I can close this (or keep open if issue remains).

@joca-bt
Copy link
Author

joca-bt commented Apr 6, 2023

I can test it in the weekend. Where can I get the snapshot jar from?

@cowtowncoder
Copy link
Member

@joca-bt Not snapshot, pre-release candidate (2.15.0-rc2), from Maven Central. :)

@joca-bt
Copy link
Author

joca-bt commented Apr 7, 2023

Ran the test cases in the zip I shared against 2.15.0-rc:

  • Fixes line 27 objectMapper.readValue("{ \"fieldName\": null }", Test3.class); // should throw but doesn't. It now throws an InvalidNullException.
  • Breaks line 28 objectMapper.readValue("{ }", Test3.class);. It now throws an InvalidNullException when it shouldn't.
  • Line 32 remains the same (broken) objectMapper.readValue("{ }", Test4.class); // throws but shouldn't. Still throws an InvalidNullException when it shouldn't.

Overall, there are still some issues with records.

@yihtserns
Copy link
Contributor

yihtserns commented Apr 7, 2023

Right I totally missed the { } scenario.

Actually your class is not identical to your record, e.g.:

public record Test(String fieldName) { }

...is actually equivalent to:

public class Test {
  private String fieldName;
  public Test(String fieldName) { this.fieldName = fieldName) }
}

...which when fed with empty hash { }, will also throw InvalidNullException.


The javadoc for Nulls & JsonSetter both mentioned "explicit null value". Does that mean there is a config to differentiate between the input containing <field name>: null vs input not having the field (i.e. absent)? 🤔

@cowtowncoder
Copy link
Member

@yihtserns Missing value (absent) is generally differently handled from explicit null -- former causes no action wrt setters for example. But in case of Creators (constructors/factory methods) it needs to be handled so there can be a difference. For deserialization purposes JsonDeserializer does have separate methods (getNullValue(), getAbsentValue() to allow different coercions. This gets mostly tricky with things like Optional<...>.

@cowtowncoder
Copy link
Member

cowtowncoder commented Apr 7, 2023

I think this issue probably needs to be broken up into separate ones, targeting what is not apparently working with 2.15.0-rc2.
I am not sure all cases are working incorrectly, but they are easier to handle via separate issues usually as there may be separate root causes.

@yihtserns
Copy link
Contributor

yihtserns commented Apr 9, 2023

Created #3870 for "fail explicit null + constructor creator + absent value = InvalidNullException".

@yihtserns
Copy link
Contributor

yihtserns commented Apr 10, 2023

Just to repeat myself, a Class equivalent of Record is not one with setter, but this:

public class Test {
    private final String fieldName;

    @ConstructorProperties({"fieldName"})
    public Test(String fieldName) {
        this.fieldName = fieldName;
    }

    public String getFieldName() {
        return this.fieldName;
    }
}

(There were some behavioural discrepancies between a Class with constructor creator vs Records, but they are now mostly aligned in 2.15.0.)

...which will have this behaviour:

objectMapper.readValue("{ \"fieldName\": \"value\" }", Test.class); // no exception thrown
objectMapper.readValue("{ \"fieldName\": null }", Test.class); // throws InvalidNullException
objectMapper.readValue("{ }", Test.class); // throws InvalidNullException

Knowing that, this issue is really about the behavioural difference between a POJO with constructor creator vs setter, i.e. #3870.

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

4 participants