@JsonMerge deep update doesn't call setter for Collection/Map

[cmolodo]

If an object property is serialized/deserialized with accessor methods and annotated with @JsonMerge, the setter is never called, because the "newValue" is always the same object as the "oldValue" in MergingSettableBeanProperty.

This means that if your setter normally does some additional logic, the merge won't be propagated/handled correctly. I ran into this with a Map for which I wanted the setter to be called after update, but it applies to any object. (If curious, I was trying to set up a system to allow both merge and deletion of Map entries, by explicitly setting the value to null in the JSON, and then removing any entries with null values in the setter. Currently, there's no way to handle removal of Map entries if the property is marked with @JsonMerge.)

Note that it also means that if the property isn't a Collection/Map and the getter returns a copy of the underlying object, the merge will also fail, though that's probably not a common use case.

Contrived self-contained example:

public class JsonMergeTest {
    @JsonAutoDetect(fieldVisibility = Visibility.NONE, isGetterVisibility = Visibility.ANY,
            getterVisibility = Visibility.ANY, setterVisibility = Visibility.ANY)
    public static class JsonMergeContainer {
        private JsonMergeChild child1;
        private Map<Integer, JsonMergeChild> childMap;
        private List<JsonMergeChild> childList;

        @JsonMerge
        public JsonMergeChild getChild1() {
            // By returning a newly-generated object, the merge will fail
            // because setter isn't called
            return new JsonMergeChild(child1);
        }

        @JsonMerge
        public void setChild1(JsonMergeChild child1) {
            this.child1 = child1;
        }

        @JsonMerge
        public Map<Integer, JsonMergeChild> getChildMap() {
            return childMap;
        }

        @JsonMerge
        public void setChildMap(Map<Integer, JsonMergeChild> childMap) {
            // This Map will be updated correctly because the merge modifies the
            // returned Map, but the additional logic to update the List
            // won't be called
            this.childMap = childMap;
            if (childMap != null) {
                // Filter out any children with id 10+, contrived logic to illustrate issue
                this.childList = childMap.values().stream().filter(child -> child.getId() < 10).collect(Collectors.toList());
            }
            else {
                this.childList = null;
            }
        }

        @JsonIgnore
        public List<JsonMergeChild> getChildList() {
            return childList;
        }
    }

    public static class JsonMergeChild {
        private int id;
        private String name;

        public JsonMergeChild() {
        }

        public JsonMergeChild(int id, String name) {
            this.id = id;
            this.name = name;
        }

        public JsonMergeChild(JsonMergeChild copy) {
            this.id = copy.id;
            this.name = copy.name;
        }

        public int getId() {
            return id;
        }

        public void setId(int id) {
            this.id = id;
        }

        public String getName() {
            return name;
        }

        public void setName(String name) {
            this.name = name;
        }

        // Methods added to make test and test output cleaner
        @Override
        public String toString() {
            return "JsonMergeChild{id=" + id + ", name='" + name + "'}";
        }

        @Override
        public boolean equals(Object o) {
            if (this == o) {
                return true;
            }
            if (o == null || getClass() != o.getClass()) {
                return false;
            }
            JsonMergeChild that = (JsonMergeChild) o;
            return id == that.id &&
                   Objects.equals(name, that.name);
        }

        @Override
        public int hashCode() {
            return Objects.hash(id, name);
        }
    }

    @Test
    public void testMerge() throws Exception {
        ObjectMapper mapper = new ObjectMapper();
        JsonMergeContainer container = new JsonMergeContainer();
        container.setChild1(new JsonMergeChild(1, "Child 1"));
        Map<Integer, JsonMergeChild> map = new HashMap<>();
        for (int i = 2; i < 5; i++) {
            map.put(i, new JsonMergeChild(i, "Child " + i));
        }
        container.setChildMap(map);

        String json = "{\"child1\":{\"id\":10,\"name\":\"child 1 updated\"},"
                      + "\"childMap\":{\"2\":{\"id\":20},\"3\":{\"name\":\"child 3 updated\"}}}";
        JsonMergeContainer result = mapper.readerForUpdating(container).readValue(json);

        // Passing tests based on current logic
        assertThat(result.getChildMap(), allOf(hasKey(2), hasKey(3), hasKey(4)));
        assertThat(result.getChildMap().get(2), is(new JsonMergeChild(20, "Child 2")));
        assertThat(result.getChildMap().get(3), is(new JsonMergeChild(3, "child 3 updated")));
        assertThat(result.getChildMap().get(4), is(new JsonMergeChild(4, "Child 4")));

        // Failing test because getter generated new object, so underlying object not updated
        assertThat(result.getChild1(), is(new JsonMergeChild(10, "child 1 updated")));

        // Failing tests because childMap setter not called, so childList not updated to filter out the modified child
        assertThat(result.getChildList(), hasSize(2));
        assertThat(result.getChildList(), containsInAnyOrder(new JsonMergeChild(3, "child 3 updated"),
                                                             new JsonMergeChild(4, "Child 4")));
    }
}

[cowtowncoder]

If I understand your description correctly, yes, this is how things work and are designed to work. Merging does mean that an existing Object will be updated, and since no change for identity occurs, neither assignment (to field), nor setting (via setter) is called.

I assume this issue can then be considered as an RFE to have a mechanism to alternatively call setter regardless of change (or not), in case there is a setter (no point in re-assigning). It would probably have to be a global MapperFeature since there are no other customization mechanisms via @JsonMerge handling (it is simple Boolean valued setting, essentially).

[cmolodo]

That makes sense, thank you! I definitely take your point about the distinction between merging and standard deserialization, so this isn't a bug, sorry. I was just naively expecting that since non-merges always call the setter, merges would as well. I understand why you wouldn't "set" the value for field assignment, but I guess I'm still not sure why merges shouldn't always call a setter if using accessor methods instead? (Not trying to be argumentative, just curious!)

In any case, yes, please consider this an RFE. It would definitely be very useful, at least for me, to have some option such as a global MapperFeature to force use of the setter. But I recognize it may not be a very common use case.

[cowtowncoder]

@cmolodo No problem at all -- I can see how one could think setter would still be involved. I just hadn't even thought of that myself.

As to why not calling setter: performance (or, more generally, avoiding doing things that are/seem unnecessary). Calls to methods via reflection are significant part of data-binding overhead, so in general things are only done minimal number of times deemed necessary. Plus I honestly did not think of use case where setter has validation logic. If I had, I might have added support for deciding on this earlier.

[smoomrik]

Looks like in that case @JsonSetter with null skips will not work in conjunction with @JsonMerge.