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

PUT/PATCH of Entity object that is a Map silently dropping elements of Map [DATAREST-919] #1283

Closed
spring-projects-issues opened this issue Oct 11, 2016 · 6 comments
Assignees
Labels

Comments

@spring-projects-issues
Copy link

@spring-projects-issues spring-projects-issues commented Oct 11, 2016

Marc Zampetti opened DATAREST-919 and commented

When one has an Entity that has an attribute that is a Map, performing a PUT/PATCH to apply an update the Map will silently throw away any first-level sub-elements of the Map that are themselves a JSON object.

For example, if I have an entity like:

@Entity
public class Person
{
    @Id
    private Long id;

     private String name;

     private HashMap<String, Object> attributes;
}

And I create the initial record with the following JSON using POST /api/persons:

{
    "name" : "person1",
    "attributes" : {
          "sub1" : "ok",
          "sub2" : [ "ok1", "ok2" ],
          "sub3" : [ { "childOk1" : "ok" }],
          "sub4" : {
               "c1" : "v1"
          }
     }
}

Everything works fine. If I then try to update using PATCH or PUT /api/persons/1:

{
    "attributes" : {
          "sub1" : "ok",
          "sub2" : [ "ok1", "ok2" ],
          "sub3" : [ { "childOk1" : "ok" }],
          "sub4" : {
               "c1" : "v1",
               "c2" : "new"
          }
     }
}

Not only will the attributes.sub4.c2 value not be saved, the entire attributes.sub4 element will disappear. The rest of the items, including attributes.sub3 will remain, since its value is an array of objects instead of just an object.

If I then submit the exact same REST call, everything will return. In fact, if you just keep doing PUT or PATCH over and over, it will keep flipping back and forth from the two states.

It appears the issue is somewhere in DomainObjectReader.java, but I haven't been able to fully track it down as of yet. I wanted to get this logged since I suspect this could be a major bug for people, and given that it only appears during the PATCH/PUT phase, its possible this could be missed for awhile


Affects: 2.5.4 (Hopper SR4)

Reference URL: https://github.com/zampettim/spring-data-rest-datarest919

Backported to: 2.5.5 (Hopper SR5), 2.4.7 (Gosling SR7)

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 12, 2016

Marc Zampetti commented

I added a test application to demonstrate the issue using Spring Data REST and Spring Data Elasticsearch. The link to the code in GitHub is linked above.

To show the issue, the following command can be used.

curl -H "Content-Type: application/json" -X POST -d '{ "name" : "test1", "attributes" : { "a1" : "av1", "a2" : { "s1" : "sv1" } } }' http://localhost:8080

Will produce output:

  "name" : "test1",
  "attributes" : {
    "a1" : "av1",
    "a2" : {
      "s1" : "sv1"
    }
  },
  "_links" : {
    "self" : {
      "href" : "http://localhost:8080/persons/AVe2gPSkqtYHtXdCp91s"
    },
    "person" : {
      "href" : "http://localhost:8080/persons/AVe2gPSkqtYHtXdCp91s"
    }
  }
}

Now running the PATCH to add another sub-element to the attributes.a2 element:

curl -H "Content-Type: application/json" -X PATCH -d '{ "name" : "test1", "attributes" : { "a1" : "av1", "a2" : { "s1" : "sv1", "s2" : "sv2" } } }' http://localhost:8080/persons/AVe2gPSkqtYHtXdCp91s

produces the output:

{
  "name" : "test1",
  "attributes" : {
    "a1" : "av1"
  },
  "_links" : {
    "self" : {
      "href" : "http://localhost:8080/persons/AVe2gPSkqtYHtXdCp91s"
    },
    "person" : {
      "href" : "http://localhost:8080/persons/AVe2gPSkqtYHtXdCp91s"
    }
  }
}

Notice that the attributes.a2 element is gone. Running the exact same command again:

curl -H "Content-Type: application/json" -X PATCH -d '{ "name" : "test1", "attributes" : { "a1" : "av1", "a2" : { "s1" : "sv1", "s2" : "sv2" } } }' http://localhost:8080/persons/AVe2gPSkqtYHtXdCp91s

produces:

{
  "name" : "test1",
  "attributes" : {
    "a1" : "av1",
    "a2" : {
      "s1" : "sv1",
      "s2" : "sv2"
    }
  },
  "_links" : {
    "self" : {
      "href" : "http://localhost:8080/persons/AVe2gPSkqtYHtXdCp91s"
    },
    "person" : {
      "href" : "http://localhost:8080/persons/AVe2gPSkqtYHtXdCp91s"
    }
  }
}

Now the attributes.a2 element is back, with the additional sub-element added

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 12, 2016

Marc Zampetti commented

Also confirmed the example has the issue with Spring Boot 1.4.1.RELEASE and Hopper.SR4

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 17, 2016

Joseph Valerio commented

Seems to be an issue with DomainObjectReader line 239:240 where if the value is an ObjectNode, which in this case it is, it recursively calls doMerge():

224: 	private void doMergeNestedMap(Map<String, Object> source, ObjectNode node, ObjectMapper mapper) throws Exception {
225: 
226: 		if (source == null) {
227: 			return;
228: 		}
229: 
230: 		Iterator<Entry<String, JsonNode>> fields = node.fields();
231: 
232: 		while (fields.hasNext()) {
233: 
234: 			Entry<String, JsonNode> entry = fields.next();
235: 			JsonNode child = entry.getValue();
236: 			Object sourceValue = source.get(entry.getKey());
237: 
238: 			if (child instanceof ObjectNode && sourceValue != null) {
239: 				doMerge((ObjectNode) child, sourceValue, mapper);
240: 				fields.remove();
241: 			}
242: 		}
243: 	}

doMerge() early returns on line 156, which merges nothing, but does return the child Map which is ignored and then the child Map is removed on line 240 above. On a subsequent request, since the source value is missing, it bypasses this logic, causing the back and forth nature of the defect. I think the method above needs to do something with the returned map, but I am not sure where to apply it, as I would think it would need to be applied to the map that aleady has it as a member. I am also not sure of the level of support for nested patch on such elements. I almost think if the result of doMerge is a Map bypass the delete... I think original authors need to weigh in here.

147: 	private <T> T doMerge(ObjectNode root, T target, ObjectMapper mapper) throws Exception {
148: 
149: 		Assert.notNull(root, "Root ObjectNode must not be null!");
150: 		Assert.notNull(target, "Target object instance must not be null!");
151: 		Assert.notNull(mapper, "ObjectMapper must not be null!");
152: 
153: 		PersistentEntity<?, ?> entity = entities.getPersistentEntity(target.getClass());
154: 
155: 		if (entity == null) {
156: 			return mapper.readerForUpdating(target).readValue(root);
157: 		}

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 3, 2016

Marc Zampetti commented

Any update if anyone has reviewed this and any comments on the issue or possible solution?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 3, 2016

Oliver Drotbohm commented

I've put a fix in the just released 2.5.5.RELEASE, care to give that a try?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Nov 4, 2016

Marc Zampetti commented

The fix in 2.5.5.RELEASE does seem to resolve the issue as highlighted in the test case. I'll need to do some more work to test in my app, but this does appear to fix it.

FYI, this fix doesn't show up in the Changelog for the Hopper-SR5 release notes, by the way

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants