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

The merge+patch implementation does not conform to RFC 7386 #2325

Closed
alienisty opened this issue Oct 26, 2023 · 0 comments
Closed

The merge+patch implementation does not conform to RFC 7386 #2325

alienisty opened this issue Oct 26, 2023 · 0 comments
Assignees
Labels
type: bug A general bug

Comments

@alienisty
Copy link

alienisty commented Oct 26, 2023

The current implementation of the mergePatch algorithm fails to properly handle merge of arrays, in fact, if the target object field;

  • is an array, we can't patch it's content at all.
  • is a collection, we can change existing elements, but we can only append one element.

The following test cases (to add to DomainObjectReaderUnitTests) will fail with the current implementation even if the expectations are valid according to RFC 7386:

  @BeforeEach
  void setUp() {

    KeyValueMappingContext<?, ?> mappingContext = new KeyValueMappingContext<>();
    // ...
    mappingContext.getPersistentEntity(ArrayListHolder.class);
    // ...
  }
     
  //...

  @Test // issue #2325
  void arraysCanMutateAndAppendDuringMerge() throws Exception {
    ObjectMapper mapper = new ObjectMapper();
    ArrayHolder target = new ArrayHolder(new String[] { "ancient", "old", "older" });
    JsonNode node = mapper.readTree("{ \"array\" : [ \"new\", \"old\", \"newer\", \"bleeding edge\" ] }");

    ArrayHolder updated = reader.doMerge((ObjectNode) node, target, mapper);
    assertThat(updated.array).containsExactly("new", "old", "newer", "bleeding edge");
  }

  @Test // issue #2325
  void arraysCanAppendMoreThanOneElementDuringMerge() throws Exception {
    ObjectMapper mapper = new ObjectMapper();
    ArrayListHolder target = new ArrayListHolder( "ancient", "old", "older" );
    JsonNode node = mapper.readTree("{ \"values\" : [ \"ancient\", \"old\", \"older\", \"new\", \"newer\" ] }");

    ArrayListHolder updated = reader.doMerge((ObjectNode) node, target, mapper);
    assertThat(updated.values).containsExactly("ancient", "old", "older", "new", "newer");
  }

  @Test // issue #2325
  void arraysCanRemoveElementsDuringMerge() throws Exception {
    ObjectMapper mapper = new ObjectMapper();
    ArrayHolder target = new ArrayHolder(new String[] { "ancient", "old", "older" });
    JsonNode node = mapper.readTree("{ \"array\" : [ \"ancient\" ] }");

    ArrayHolder updated = reader.doMerge((ObjectNode) node, target, mapper);
    assertThat(updated.array).containsExactly("ancient");
  }

// ...

  static class ArrayListHolder {
    Collection<String> values;

    ArrayListHolder(String... values) {
      this.values = new ArrayList<>(Arrays.asList(values));
    }
    
    public void setValues(Collection<String> values) {
      this.values = values;
    }
  }

// ...
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Oct 26, 2023
odrotbohm added a commit that referenced this issue Nov 16, 2023
odrotbohm added a commit that referenced this issue Nov 16, 2023
odrotbohm added a commit that referenced this issue Nov 16, 2023
@odrotbohm odrotbohm added this to the 3.7.18 (2021.2.18) milestone Nov 16, 2023
@odrotbohm odrotbohm added type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 16, 2023
odrotbohm added a commit that referenced this issue Nov 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants