spring-projects / spring-data-rest

Simplifies building hypermedia-driven REST web services on top of Spring Data repositories
https://spring.io/projects/spring-data-rest
Apache License 2.0
915 stars 561 forks source link

UriStringDeserializer does not handle managed/back reference #2036

Open alienisty opened 3 years ago

alienisty commented 3 years ago

If an entity does not have a (direct) repository and it has a bidirectional parent-child relationship with an entity which has a repository then, if we @JsonManagedReference and @JsonBackReference on the relationship attribues, the we get the following exception:

java.lang.IllegalArgumentException: Cannot handle managed/back reference 'person': type: value deserializer of type org.springframework.data.rest.webmvc.json.PersistentEntityJackson2Module$UriStringDeserializer does not support them
    at com.fasterxml.jackson.databind.JsonDeserializer.findBackReference(JsonDeserializer.java:397) ~[jackson-databind-2.12.3.jar:2.12.3]
    at com.fasterxml.jackson.databind.deser.std.ContainerDeserializerBase.findBackReference(ContainerDeserializerBase.java:104) ~[jackson-databind-2.12.3.jar:2.12.3]
    at com.fasterxml.jackson.databind.deser.BeanDeserializerBase._resolveManagedReferenceProperty(BeanDeserializerBase.java:882) ~[jackson-databind-2.12.3.jar:2.12.3]
    at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.resolve(BeanDeserializerBase.java:553) ~[jackson-databind-2.12.3.jar:2.12.3]
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCache2(DeserializerCache.java:293) ~[jackson-databind-2.12.3.jar:2.12.3]
    at com.fasterxml.jackson.databind.deser.DeserializerCache._createAndCacheValueDeserializer(DeserializerCache.java:244) ~[jackson-databind-2.12.3.jar:2.12.3]
    at com.fasterxml.jackson.databind.deser.DeserializerCache.findValueDeserializer(DeserializerCache.java:142) ~[jackson-databind-2.12.3.jar:2.12.3]
    at com.fasterxml.jackson.databind.DeserializationContext.findContextualValueDeserializer(DeserializationContext.java:558) ~[jackson-databind-2.12.3.jar:2.12.3]
    at com.fasterxml.jackson.databind.jsontype.impl.TypeDeserializerBase._findDeserializer(TypeDeserializerBase.java:203) ~[jackson-databind-2.12.3.jar:2.12.3]
    at com.fasterxml.jackson.databind.jsontype.impl.AsPropertyTypeDeserializer._deserializeTypedForId(AsPropertyTypeDeserializer.java:120) ~[jackson-databind-2.12.3.jar:2.12.3]
    at com.fasterxml.jackson.databind.jsontype.impl.AsPropertyTypeDeserializer.deserializeTypedFromObject(AsPropertyTypeDeserializer.java:107) ~[jackson-databind-2.12.3.jar:2.12.3]
    at com.fasterxml.jackson.databind.deser.AbstractDeserializer.deserializeWithType(AbstractDeserializer.java:263) ~[jackson-databind-2.12.3.jar:2.12.3]
    at com.fasterxml.jackson.databind.deser.impl.TypeWrappedDeserializer.deserialize(TypeWrappedDeserializer.java:74) ~[jackson-databind-2.12.3.jar:2.12.3]
    at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:322) ~[jackson-databind-2.12.3.jar:2.12.3]
    at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4593) ~[jackson-databind-2.12.3.jar:2.12.3]
    at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3601) ~[jackson-databind-2.12.3.jar:2.12.3]
    at org.springframework.http.converter.json.AbstractJackson2HttpMessageConverter.readJavaType(AbstractJackson2HttpMessageConverter.java:378) ~[spring-web-5.3.8.jar:5.3.8]
    at org.springframework.http.converter.json.AbstractJackson2HttpMessageConverter.readInternal(AbstractJackson2HttpMessageConverter.java:350) ~[spring-web-5.3.8.jar:5.3.8]
    at org.springframework.http.converter.AbstractHttpMessageConverter.read(AbstractHttpMessageConverter.java:199) ~[spring-web-5.3.8.jar:5.3.8]
    at org.springframework.data.rest.webmvc.config.PersistentEntityResourceHandlerMethodArgumentResolver.read(PersistentEntityResourceHandlerMethodArgumentResolver.java:246) ~[spring-data-rest-webmvc-3.5.2.jar:3.5.2]
    at org.springframework.data.rest.webmvc.config.PersistentEntityResourceHandlerMethodArgumentResolver.lambda$read$6(PersistentEntityResourceHandlerMethodArgumentResolver.java:202) ~[spring-data-rest-webmvc-3.5.2.jar:3.5.2]
    at java.base/java.util.Optional.orElseGet(Optional.java:369) ~[na:na]
    at org.springframework.data.rest.webmvc.config.PersistentEntityResourceHandlerMethodArgumentResolver.read(PersistentEntityResourceHandlerMethodArgumentResolver.java:202) ~[spring-data-rest-webmvc-3.5.2.jar:3.5.2]
    at org.springframework.data.rest.webmvc.config.PersistentEntityResourceHandlerMethodArgumentResolver.resolveArgument(PersistentEntityResourceHandlerMethodArgumentResolver.java:132) ~[spring-data-rest-webmvc-3.5.2.jar:3.5.2]
    at org.springframework.web.method.support.HandlerMethodArgumentResolverComposite.resolveArgument(HandlerMethodArgumentResolverComposite.java:121) ~[spring-web-5.3.8.jar:5.3.8]
    at org.springframework.web.method.support.InvocableHandlerMethod.getMethodArgumentValues(InvocableHandlerMethod.java:170) ~[spring-web-5.3.8.jar:5.3.8]
    at org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest(InvocableHandlerMethod.java:137) ~[spring-web-5.3.8.jar:5.3.8]
    at org.springframework.web.servlet.mvc.method.annotation.ServletInvocableHandlerMethod.invokeAndHandle(ServletInvocableHandlerMethod.java:106) ~[spring-webmvc-5.3.8.jar:5.3.8]
    at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.invokeHandlerMethod(RequestMappingHandlerAdapter.java:894) ~[spring-webmvc-5.3.8.jar:5.3.8]
    at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.handleInternal(RequestMappingHandlerAdapter.java:808) ~[spring-webmvc-5.3.8.jar:5.3.8]
    at org.springframework.web.servlet.mvc.method.AbstractHandlerMethodAdapter.handle(AbstractHandlerMethodAdapter.java:87) ~[spring-webmvc-5.3.8.jar:5.3.8]
    at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:1063) ~[spring-webmvc-5.3.8.jar:5.3.8]

Attached is a demo project to reproduce the issue. demo-2036.zip

alienisty commented 3 years ago

As far as I can tell, the fix should be as easy as skipping the creation of a UriStringDeserializer in both AggregateReferenceDeserializerModifier and AssociationUriResolvingDeserializerModifier if property.getManagedReferenceName() != null.

odrotbohm commented 3 years ago

The overall domain / resource arrangement you model here is unusual: implementing a bi-directional relationship between two aggregates ultimately raises the question how to actually properly create each of them in the first place. Multiple aggregates are not changed in one go, which renders the test case provided in the example invalid as it tries to create a Person and in the very same step attempt to change an already existing address through the same representation. That fundamentally violates the concept of aggregates in the first place.

There are a couple of options here:

My general impression is that the example too much thinks of "accessing data via a HTTP" rather than "exposing aggregates as REST resources". Spring Data REST is fundamentally built around the latter, which means that you're likely to run into issues if you don't follow the fundamental principles of designing DDD aggregates.

alienisty commented 3 years ago

Yes I agree, I focused more on reproducing the issue I was seeing and I didn't notice I was using POST to update an entity, sorry about that. I definitely agree that bidirectional associations are dangerous, for one thing they can easily introduce infinite loops, I can also appreciate the concept of DDD aggregates and their merits, but we are not leaving in an ideal world so if the framework provided some flexibility that would be great. For example, if you need to deal with legacy code, it is not always possible to port it all to the required structures, so an escape hatch for those cases would be great. Also, DDD aggregates do not lend themselves well to fine grained updates of root entities with a very large graph, where, I think, optimistic locking works better in ensuring transactional consistency. I have patched AggregateReferenceResolvingModule and AssociationUriResolvingDeserializerModifier to simply skip building a UriStringDeserializer if the properties specifies a managed backreference name, and that seems to have not caused any regression, but I'll leave it up to you to assess the suggestion.

odrotbohm commented 3 years ago

I'd certainly be willing to try to at least make back-references, not break the general functionality provided. That said, SD REST is built on some fundamental concepts and the design constraints they imply. That already is quite complex in the first place. Increasing that complexity by trying to support scenarios that fundamentally violate the constraints of the underlying concepts would broaden the scope of the project outside the bounds we can handle. SD REST is built in a way that it allows plugging custom resource handlers so that you can easily mix and match SD REST handled resources and manually implemented controllers. I'd advise switching to the latter if you feel you hit the boundaries of what you need to design.

I have patched AggregateReferenceResolvingModule and AssociationUriResolvingDeserializerModifier to simply skip building a UriStringDeserializer if the properties specifies a managed backreference name, and that seems to have not caused any regression, but I'll leave it up to you to assess the suggestion.

I'll have a look into that. Just to make sure I get the situation right: what you're essentially trying to deserialize is sth. logically equivalent to this:

Person {

  @JsonManagedReference
  Address address {

    @JsonBackReference
    Person owner; // <- Reference back to the original, top-level type
  }
}

Correct?

alienisty commented 3 years ago

Yes that is correct and the property annotated with @JasonManagedReference could also be a collection.

alienisty commented 2 years ago

Any ETA on this one?

alienisty commented 2 years ago

Would you be happy for me to create a PR for this?

alienisty commented 9 months ago

@odrotbohm is there any chance for this to be addressed?