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

JsonSetter(contentNulls = FAIL) is ignored in JsonCreator(DELEGATING) argument #4200

Closed
1 task done
k163377 opened this issue Nov 12, 2023 · 10 comments
Closed
1 task done
Labels
2.16 Issues planned for 2.16 has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue
Milestone

Comments

@k163377
Copy link
Contributor

k163377 commented Nov 12, 2023

Search before asking

  • I searched in the issues and found nothing similar.

Describe the bug

I specified JsonSetter(contentNulls = FAIL) or SKIP in the constructor argument with JsonCreator(DELEGATING), but it was ignored.

Version Information

2.15.3

Reproduction

If other than DELEGATING, an InvalidNullException is thrown as expected.

import com.fasterxml.jackson.annotation.JsonCreator;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonSetter;
import com.fasterxml.jackson.annotation.Nulls;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.exc.InvalidNullException;
import org.junit.jupiter.api.Test;

import java.util.Map;

import static org.junit.jupiter.api.Assertions.assertThrows;

public class GitHubXXX {
    static class DelegatingWrapper {
        private final Map<String, String> value;

        @JsonCreator(mode = JsonCreator.Mode.DELEGATING)
        DelegatingWrapper(@JsonSetter(contentNulls = Nulls.FAIL) Map<String, String> value) {
            this.value = value;
        }

        public Map<String, String> getValue() {
            return value;
        }
    }

    @Test
    public void fails() {
        ObjectMapper mapper = new ObjectMapper();

        assertThrows(
                InvalidNullException.class,
                () -> mapper.readValue("{\"foo\":null}", DelegatingWrapper.class)
        );
    }

    static class SetterWrapper {
        private Map<String, String> value;

        public Map<String, String> getValue() {
            return value;
        }

        @JsonSetter(contentNulls = Nulls.FAIL)
        public void setValue(Map<String, String> value) {
            this.value = value;
        }
    }

    static class PropertiesWrapper {
        private final Map<String, String> value;

        @JsonCreator(mode = JsonCreator.Mode.PROPERTIES)
        PropertiesWrapper(
                @JsonSetter(contentNulls = Nulls.FAIL)
                @JsonProperty("value")
                Map<String, String> value
        ) {
            this.value = value;
        }

        public Map<String, String> getValue() {
            return value;
        }
    }

    static class DefaultWrapper {
        private final Map<String, String> value;

        @JsonCreator(mode = JsonCreator.Mode.DEFAULT)
        DefaultWrapper(
                @JsonSetter(contentNulls = Nulls.FAIL)
                @JsonProperty("value")
                Map<String, String> value
        ) {
            this.value = value;
        }

        public Map<String, String> getValue() {
            return value;
        }
    }

    @Test
    void valid() {
        ObjectMapper mapper = new ObjectMapper();

        assertThrows(
                InvalidNullException.class,
                () -> mapper.readValue("{\"value\":{\"foo\":null}}", SetterWrapper.class)
        );
        assertThrows(
                InvalidNullException.class,
                () -> mapper.readValue("{\"value\":{\"foo\":null}}", PropertiesWrapper.class)
        );
        assertThrows(
                InvalidNullException.class,
                () -> mapper.readValue("{\"value\":{\"foo\":null}}", DefaultWrapper.class)
        );
    }
}

Expected behavior

An InvalidNullException is thrown.

Additional context

Fixing this issue may make it easier to resolve FasterXML/jackson-module-kotlin#399.

@k163377 k163377 added the to-evaluate Issue that has been received but not yet evaluated label Nov 12, 2023
@k163377
Copy link
Contributor Author

k163377 commented Nov 12, 2023

I have debugged it and it seems that all parsing related to PropertyMetadata is skipped.

@SuppressWarnings("unchecked")
private JsonDeserializer<Object> _findDelegateDeserializer(DeserializationContext ctxt,
JavaType delegateType, AnnotatedWithParams delegateCreator) throws JsonMappingException
{
// Need to create a temporary property to allow contextual deserializers:
BeanProperty.Std property = new BeanProperty.Std(TEMP_PROPERTY_NAME,
delegateType, null, delegateCreator,
PropertyMetadata.STD_OPTIONAL);

I could not fully check the path, but it seems that the parsing of the parameters was done before this process, so I personally think it is appropriate to copy the results of that process.

@JooHyukKim
Copy link
Member

I tried debugging also, it seems like "value" field for DelegateWrapper is treated only as a field property and ContainerDeserializerBase._nullProvider is set to StringDeserializer.

image

@cowtowncoder cowtowncoder added the 2.17 Issues planned at earliest for 2.17 label Nov 13, 2023
@cowtowncoder
Copy link
Member

This does sound like a bug, and not related to general challenge wrt Creator property detection (since there is explicit @JsonCreator annotation).

@cowtowncoder
Copy link
Member

@k163377 is probably right in pointing that this is why annotations are not found -- there is no property accessor (Constructor/Factory method parameter declaration) being passed and a placeholder (with no annotations) is given.
This would prevent other annotations from being accessible similarly.

And why is it not being passed? It is not being retained during Creator collection process, it looks like.
Not sure how easy it'd be to add that plumbing, will have a look.

@cowtowncoder
Copy link
Member

Hmmh. Access is via ValueInstantiator which only provides type information, not accessor.

I might just add a failing test for now.

@cowtowncoder cowtowncoder added has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue and removed to-evaluate Issue that has been received but not yet evaluated labels Nov 28, 2023
cowtowncoder added a commit that referenced this issue Nov 28, 2023
@cowtowncoder
Copy link
Member

Ok doh. I am blind. _findDelegateDeserializer() is directly passed the accessor. It need not be passed. :-)

But need to see how to change BeanProperty.Std to give/use necessary information.

@cowtowncoder
Copy link
Member

Ok, phew. Was able to figure it out, relatively cleanly. Fix will be in 2.17.0 at least, although I guess I can double-check if this might be backportable.

@cowtowncoder
Copy link
Member

Ok, and with that backporting to 2.16.1 worked fine.

@cowtowncoder cowtowncoder added 2.16 Issues planned for 2.16 and removed 2.17 Issues planned at earliest for 2.17 labels Nov 28, 2023
@cowtowncoder cowtowncoder added this to the 2.16.1 milestone Nov 28, 2023
@k163377
Copy link
Contributor Author

k163377 commented Nov 28, 2023

Wow, thank you for even back porting.

@cowtowncoder
Copy link
Member

No problem @k163377 -- I do it for safe fixes. Thank you for reporting this, pointing out where the problem is. Saved me lots of time.

dongjoon-hyun pushed a commit to apache/spark that referenced this issue Dec 27, 2023
### What changes were proposed in this pull request?
This pr aims to upgrade jackson from 2.16.0 to 2.16.1

### Why are the changes needed?
The new version bring some fix:

- [#4200](FasterXML/jackson-databind#4200): JsonSetter(contentNulls = FAIL) is ignored in delegating JsonCreator argument
- [#4216](FasterXML/jackson-databind#4216): Primitive array deserializer not being captured by DeserializerModifier
- [#4219](FasterXML/jackson-databind#4219): JsonNode.findValues() and findParents() missing expected values in 2.16.0

The full release notes as follows:

- https://github.com/FasterXML/jackson/wiki/Jackson-Release-2.16.1

### Does this PR introduce _any_ user-facing change?
No

### How was this patch tested?
Pass Github Actions

### Was this patch authored or co-authored using generative AI tooling?
No

Closes #44494 from LuciferYang/SPARK-46508.

Authored-by: yangjie01 <yangjie01@baidu.com>
Signed-off-by: Dongjoon Hyun <dhyun@apple.com>
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 has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue
Projects
None yet
Development

No branches or pull requests

3 participants