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

configOverride.setMergeable(false) not supported by ArrayNode #3338

Closed
ejl888 opened this issue Nov 26, 2021 · 9 comments
Closed

configOverride.setMergeable(false) not supported by ArrayNode #3338

ejl888 opened this issue Nov 26, 2021 · 9 comments
Labels
has-failing-test Indicates that there exists a test case (under `failing/`) to reproduce the issue
Milestone

Comments

@ejl888
Copy link

ejl888 commented Nov 26, 2021

Describe the bug
When updating a JsonNode containing an array. An update will always add the data to the array, even when the
the mapper is configured to not merge Object arrays, ArrayNode, and Collections.

Version information
2.13.0

To Reproduce

Below a Junit5 test

import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.node.ArrayNode;
import org.junit.jupiter.api.Test;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Collection;

import static net.javacrumbs.jsonunit.assertj.JsonAssertions.assertThatJson;

class ObjectMapperMergeTest {

    @Test
    void itShouldReplaceArrayNodeAndNotAddWhenConfiguredSo() throws IOException {
        // given
        ObjectMapper testSubject = new ObjectMapper();
        testSubject.configOverride(Collection.class).setMergeable(false);
        testSubject.configOverride(ArrayNode.class).setMergeable(false);
        testSubject.configOverride(ArrayList.class).setMergeable(false);
        testSubject.configOverride(Object[].class).setMergeable(false);

        JsonNode mergeTarget = testSubject.readTree("{"
                + "\"array\": [\"Mr.\", \"Ms.\" ],"
                + "\"number\": 888"
                + "}");

        // when
        JsonNode updateNode = testSubject.readTree("{"
                + "\"array\": [\"Mister\", \"Miss\" ]"
                + "}");
        JsonNode patched = testSubject.readerForUpdating(mergeTarget).readValue(updateNode);

        // then
        assertThatJson(patched).node("number").isIntegralNumber().isEqualTo(888);
       // fails because array contains "Mr." and "Ms." also
        assertThatJson(patched).node("array").isArray().containsExactly("Mister", "Miss");
    }
}

Expected behavior
I would like to have the mapper to replace the array, because now it will never replace it.

Additional context
Related issue JsonMerge got unexpected result for List #2315

@ejl888 ejl888 added the to-evaluate Issue that has been received but not yet evaluated label Nov 26, 2021
@ejl888
Copy link
Author

ejl888 commented Nov 26, 2021

Fix might add a check for configOverride(ArrayNode.class).setMergeable(false); on line 502 in JsonNodeDeserializer.
If it is set to false, one could set currArray to a new node instead of using the current Node with the existing values or remove all values before starting to add new values.

@cowtowncoder cowtowncoder added 2.13 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 Dec 25, 2021
@cowtowncoder
Copy link
Member

Thank you for reporting this issue & apologies for slow follow up. This looks like a legit issue, thank you for also providing a reproduction.

@cowtowncoder cowtowncoder changed the title readerForUpdating not honouring configOverride setMergeable(false) configOverride setMergeable(false) not supported by ArrayNode Jan 5, 2022
@cowtowncoder cowtowncoder added 2.14 and removed 2.13 labels Jan 5, 2022
@cowtowncoder
Copy link
Member

Yes, currently this setting is not considered for any JsonNode type (and specifically neither for Arrays nor Objects).
So either it would need to be added (and typically introspection should occur when deserializer is constructed, not dynamically), or perhaps added separate as one of new JsonNodeFeatures -- if implementation (see https://github.com/FasterXML/jackson-future-ideas/wiki/JSTEP-3) proceeds for 2.14.

@ejl888
Copy link
Author

ejl888 commented Jan 5, 2022

@cowtowncoder Adding it to the JsonNodeFeatures sounds ok by me.

@TsvetanKonstantinov
Copy link

@cowtowncoder Is there a workaround for this as of now?

@cowtowncoder
Copy link
Member

@TsvetanKonstantinov not that I know of.

@echatman
Copy link

has anyone found a workaround for this?

@cowtowncoder cowtowncoder changed the title configOverride setMergeable(false) not supported by ArrayNode configOverride.setMergeable(false) not supported by ArrayNode Jun 13, 2022
@cowtowncoder
Copy link
Member

Note to self: I think UntypedObjectDeserializer shows the way, see #createContextual().
Hoping to look into this more in next few days.

cowtowncoder added a commit that referenced this issue Jun 14, 2022
@cowtowncoder cowtowncoder added this to the 2.14.0 milestone Jun 17, 2022
@cowtowncoder
Copy link
Member

Finally found time to work on this and implemented merge-blocking for upcoming 2.14.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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

4 participants