search

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
913 stars 561 forks source link

StackOverflowError in SDR 4.3 when calling PATCH with nested bidirectional relations #2393

Closed Zetten closed 3 months ago

Zetten commented 3 months ago

Following an update to Spring Boot 3.3.1 (Spring Data 2024.0.0 / Spring Data REST 4.3.1 / Spring Data JPA 3.3.1) I'm getting a test failure when trying to PATCH an entity which includes a nested collection of entities which reference the parent.

I've created a minimal reproduction here: https://github.com/Zetten/spring-data-rest-4.3-patch-stackoverflow. It's a somewhat contrived example based on my real entity graph, but roughly: a TaskConfig can be used to send the results 'outputs' of some processing task to a Basket, which is essentially modelled as a link table TaskOutputCollector with two ManyToOne relations.

image

Only the Basket and TaskConfig resources are exported via Repository interfaces. My issue occurs when calling PATCH on a TaskConfig to update its outputs field, i.e. replacing the list of associated TaskOutput objects to change a field on one of the nested TaskOutputCollector records:

        mockMvc.perform(patch(taskConfigUrl)
                        .contentType(MediaType.APPLICATION_JSON)
                        .content("""
                                    {
                                      "outputs": [{
                                          "outputIdentifier": "first-output",
                                          "collectors": [{
                                              "basket": "%s",
                                              "replaceContents": true
                                          }]
                                      }]
                                    }
                                """.formatted(basketUrl)))
                .andExpect(status().is2xxSuccessful());

In Spring Boot 3.2.7 this works fine. Following the update to Spring Boot 3.31, HTTP PATCH now calls mergeForPut, which tries to resolve and merge the relations recursively (TaskOutput -> TaskOutputCollector -> the holding TaskOutput) and therefore errors out with the below StackOverflowError.

I believe this is obviously due to the RFC-compliant handling of entities in arrays in merge patches - but the entity graph makes sense, so it seems like the merge should be a bit smarter about detecting recursion.

Handler dispatch failed: java.lang.StackOverflowError
jakarta.servlet.ServletException: 
    at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:1104)
    at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:979)
    at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:1014)
    at org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:888)
    at org.springframework.test.web.servlet.TestDispatcherServlet.service(TestDispatcherServlet.java:72)
    at jakarta.servlet.http.HttpServlet.service(HttpServlet.java:614)
    at org.springframework.mock.web.MockFilterChain$ServletFilterProxy.doFilter(MockFilterChain.java:165)
    at org.springframework.mock.web.MockFilterChain.doFilter(MockFilterChain.java:132)
    at org.springframework.web.filter.CompositeFilter$VirtualFilterChain.doFilter(CompositeFilter.java:108)
    at org.springframework.security.web.FilterChainProxy.lambda$doFilterInternal$3(FilterChainProxy.java:231)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:365)
    at org.springframework.security.web.access.intercept.AuthorizationFilter.doFilter(AuthorizationFilter.java:100)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
    at org.springframework.security.web.access.ExceptionTranslationFilter.doFilter(ExceptionTranslationFilter.java:126)
    at org.springframework.security.web.access.ExceptionTranslationFilter.doFilter(ExceptionTranslationFilter.java:120)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
    at org.springframework.security.web.authentication.AnonymousAuthenticationFilter.doFilter(AnonymousAuthenticationFilter.java:100)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
    at org.springframework.security.web.servletapi.SecurityContextHolderAwareRequestFilter.doFilter(SecurityContextHolderAwareRequestFilter.java:179)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
    at org.springframework.security.web.savedrequest.RequestCacheAwareFilter.doFilter(RequestCacheAwareFilter.java:63)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
    at org.springframework.security.web.authentication.logout.LogoutFilter.doFilter(LogoutFilter.java:107)
    at org.springframework.security.web.authentication.logout.LogoutFilter.doFilter(LogoutFilter.java:93)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
    at org.springframework.web.filter.CorsFilter.doFilterInternal(CorsFilter.java:91)
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
    at org.springframework.security.web.header.HeaderWriterFilter.doHeadersAfter(HeaderWriterFilter.java:90)
    at org.springframework.security.web.header.HeaderWriterFilter.doFilterInternal(HeaderWriterFilter.java:75)
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
    at org.springframework.security.web.context.SecurityContextHolderFilter.doFilter(SecurityContextHolderFilter.java:82)
    at org.springframework.security.web.context.SecurityContextHolderFilter.doFilter(SecurityContextHolderFilter.java:69)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
    at org.springframework.security.web.context.request.async.WebAsyncManagerIntegrationFilter.doFilterInternal(WebAsyncManagerIntegrationFilter.java:62)
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
    at org.springframework.security.web.session.DisableEncodeUrlFilter.doFilterInternal(DisableEncodeUrlFilter.java:42)
    at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:116)
    at org.springframework.security.web.FilterChainProxy$VirtualFilterChain.doFilter(FilterChainProxy.java:374)
    at org.springframework.security.web.FilterChainProxy.doFilterInternal(FilterChainProxy.java:233)
    at org.springframework.security.web.FilterChainProxy.doFilter(FilterChainProxy.java:191)
    at org.springframework.web.filter.CompositeFilter$VirtualFilterChain.doFilter(CompositeFilter.java:113)
    at org.springframework.web.servlet.handler.HandlerMappingIntrospector.lambda$createCacheFilter$3(HandlerMappingIntrospector.java:195)
    at org.springframework.web.filter.CompositeFilter$VirtualFilterChain.doFilter(CompositeFilter.java:113)
    at org.springframework.web.filter.CompositeFilter.doFilter(CompositeFilter.java:74)
    at org.springframework.security.config.annotation.web.configuration.WebMvcSecurityConfiguration$CompositeFilterChainProxy.doFilter(WebMvcSecurityConfiguration.java:230)
    at org.springframework.web.filter.DelegatingFilterProxy.invokeDelegate(DelegatingFilterProxy.java:352)
    at org.springframework.web.filter.DelegatingFilterProxy.doFilter(DelegatingFilterProxy.java:268)
    at org.springframework.test.web.servlet.setup.MockMvcFilterDecorator.doFilter(MockMvcFilterDecorator.java:151)
    at org.springframework.mock.web.MockFilterChain.doFilter(MockFilterChain.java:132)
    at org.springframework.test.web.servlet.MockMvc.perform(MockMvc.java:201)
    at com.cgi.eoss.gaiascope.datastore.job.JobRestRepositoryTest.testReplaceOutputFilesInBasket(JobRestRepositoryTest.java:1490)
    at java.base/jdk.internal.reflect.DirectMethodHandleAccessor.invoke(DirectMethodHandleAccessor.java:103)
    at java.base/java.lang.reflect.Method.invoke(Method.java:580)
    at org.junit.runners.model.FrameworkMethod$1.runReflectiveCall(FrameworkMethod.java:59)
    at org.junit.internal.runners.model.ReflectiveCallable.run(ReflectiveCallable.java:12)
    at org.junit.runners.model.FrameworkMethod.invokeExplosively(FrameworkMethod.java:56)
    at org.junit.internal.runners.statements.InvokeMethod.evaluate(InvokeMethod.java:17)
    at org.springframework.test.context.junit4.statements.RunBeforeTestExecutionCallbacks.evaluate(RunBeforeTestExecutionCallbacks.java:76)
    at org.springframework.test.context.junit4.statements.RunAfterTestExecutionCallbacks.evaluate(RunAfterTestExecutionCallbacks.java:84)
    at org.junit.internal.runners.statements.RunBefores.evaluate(RunBefores.java:26)
    at org.springframework.test.context.junit4.statements.RunBeforeTestMethodCallbacks.evaluate(RunBeforeTestMethodCallbacks.java:75)
    at org.junit.internal.runners.statements.RunAfters.evaluate(RunAfters.java:27)
    at org.springframework.test.context.junit4.statements.RunAfterTestMethodCallbacks.evaluate(RunAfterTestMethodCallbacks.java:86)
    at org.springframework.test.context.junit4.statements.SpringRepeat.evaluate(SpringRepeat.java:84)
    at org.junit.runners.ParentRunner.runLeaf(ParentRunner.java:366)
    at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:252)
    at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.runChild(SpringJUnit4ClassRunner.java:97)
    at org.junit.runners.ParentRunner$4.run(ParentRunner.java:331)
    at org.junit.runners.ParentRunner$1.schedule(ParentRunner.java:79)
    at org.junit.runners.ParentRunner.runChildren(ParentRunner.java:329)
    at org.junit.runners.ParentRunner.access$100(ParentRunner.java:66)
    at org.junit.runners.ParentRunner$2.evaluate(ParentRunner.java:293)
    at org.springframework.test.context.junit4.statements.RunBeforeTestClassCallbacks.evaluate(RunBeforeTestClassCallbacks.java:61)
    at org.springframework.test.context.junit4.statements.RunAfterTestClassCallbacks.evaluate(RunAfterTestClassCallbacks.java:70)
    at org.junit.runners.ParentRunner$3.evaluate(ParentRunner.java:306)
    at org.junit.runners.ParentRunner.run(ParentRunner.java:413)
    at org.springframework.test.context.junit4.SpringJUnit4ClassRunner.run(SpringJUnit4ClassRunner.java:191)
    at com.google.testing.junit.runner.internal.junit4.CancellableRequestFactory$CancellableRunner.run(CancellableRequestFactory.java:108)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:137)
    at org.junit.runner.JUnitCore.run(JUnitCore.java:115)
    at com.google.testing.junit.runner.junit4.JUnit4Runner.run(JUnit4Runner.java:116)
    at com.google.testing.junit.runner.BazelTestRunner.runTestsInSuite(BazelTestRunner.java:145)
    at com.google.testing.junit.runner.BazelTestRunner.main(BazelTestRunner.java:76)
Caused by: java.lang.StackOverflowError
    at com.fasterxml.jackson.databind.util.internal.PrivateMaxEntriesMap.drainWriteBuffer(PrivateMaxEntriesMap.java:441)
    at com.fasterxml.jackson.databind.util.internal.PrivateMaxEntriesMap.drainBuffers(PrivateMaxEntriesMap.java:393)
    at com.fasterxml.jackson.databind.util.internal.PrivateMaxEntriesMap.tryToDrainBuffers(PrivateMaxEntriesMap.java:381)
    at com.fasterxml.jackson.databind.util.internal.PrivateMaxEntriesMap.drainOnReadIfNeeded(PrivateMaxEntriesMap.java:358)
    at com.fasterxml.jackson.databind.util.internal.PrivateMaxEntriesMap.afterRead(PrivateMaxEntriesMap.java:315)
    at com.fasterxml.jackson.databind.util.internal.PrivateMaxEntriesMap.get(PrivateMaxEntriesMap.java:622)
    at com.fasterxml.jackson.databind.util.LRUMap.get(LRUMap.java:69)
    at com.fasterxml.jackson.databind.type.TypeFactory._fromClass(TypeFactory.java:1519)
    at com.fasterxml.jackson.databind.type.TypeFactory._fromAny(TypeFactory.java:1452)
    at com.fasterxml.jackson.databind.type.TypeFactory.resolveMemberType(TypeFactory.java:822)
    at com.fasterxml.jackson.databind.introspect.AnnotatedClass.resolveType(AnnotatedClass.java:233)
    at com.fasterxml.jackson.databind.introspect.AnnotatedMethod.getParameterType(AnnotatedMethod.java:145)
    at com.fasterxml.jackson.databind.introspect.POJOPropertyBuilder._rawTypeOf(POJOPropertyBuilder.java:1425)
    at com.fasterxml.jackson.databind.introspect.POJOPropertyBuilder._getSetterInfo(POJOPropertyBuilder.java:288)
    at com.fasterxml.jackson.databind.introspect.POJOPropertyBuilder.getMetadata(POJOPropertyBuilder.java:239)
    at com.fasterxml.jackson.databind.introspect.POJOPropertiesCollector._anyIndexed(POJOPropertiesCollector.java:1376)
    at com.fasterxml.jackson.databind.introspect.POJOPropertiesCollector._sortProperties(POJOPropertiesCollector.java:1278)
    at com.fasterxml.jackson.databind.introspect.POJOPropertiesCollector.collectAll(POJOPropertiesCollector.java:529)
    at com.fasterxml.jackson.databind.introspect.POJOPropertiesCollector.getAnySetterMethod(POJOPropertiesCollector.java:377)
    at com.fasterxml.jackson.databind.introspect.BasicBeanDescription.findAnySetterAccessor(BasicBeanDescription.java:304)
    at org.springframework.data.rest.webmvc.json.MappedProperties.<init>(MappedProperties.java:82)
    at org.springframework.data.rest.webmvc.json.MappedProperties.forDeserialization(MappedProperties.java:134)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.lambda$mergeForPut$0(DomainObjectReader.java:150)
    at java.base/java.util.Optional.map(Optional.java:260)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.mergeForPut(DomainObjectReader.java:148)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.lambda$mergeCollections$7(DomainObjectReader.java:511)
    at java.base/java.util.Optional.map(Optional.java:260)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.mergeCollections(DomainObjectReader.java:494)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader$MergingPropertyHandler.doWithPersistentProperty(DomainObjectReader.java:709)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader$LinkedAssociationSkippingAssociationHandler.doWithAssociation(DomainObjectReader.java:642)
    at org.springframework.data.mapping.model.BasicPersistentEntity.doWithAssociations(BasicPersistentEntity.java:351)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.lambda$mergeForPut$0(DomainObjectReader.java:162)
    at java.base/java.util.Optional.map(Optional.java:260)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.mergeForPut(DomainObjectReader.java:148)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader$MergingPropertyHandler.lambda$doWithPersistentProperty$0(DomainObjectReader.java:714)
    at java.base/java.util.Optional.map(Optional.java:260)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader$MergingPropertyHandler.lambda$doWithPersistentProperty$1(DomainObjectReader.java:714)
    at java.base/java.util.Optional.flatMap(Optional.java:289)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader$MergingPropertyHandler.doWithPersistentProperty(DomainObjectReader.java:714)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader$LinkedAssociationSkippingAssociationHandler.doWithAssociation(DomainObjectReader.java:642)
    at org.springframework.data.mapping.model.BasicPersistentEntity.doWithAssociations(BasicPersistentEntity.java:351)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.lambda$mergeForPut$0(DomainObjectReader.java:162)
    at java.base/java.util.Optional.map(Optional.java:260)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.mergeForPut(DomainObjectReader.java:148)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.lambda$mergeCollections$7(DomainObjectReader.java:511)
    at java.base/java.util.Optional.map(Optional.java:260)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.mergeCollections(DomainObjectReader.java:494)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader$MergingPropertyHandler.doWithPersistentProperty(DomainObjectReader.java:709)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader$LinkedAssociationSkippingAssociationHandler.doWithAssociation(DomainObjectReader.java:642)
    at org.springframework.data.mapping.model.BasicPersistentEntity.doWithAssociations(BasicPersistentEntity.java:351)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.lambda$mergeForPut$0(DomainObjectReader.java:162)
    at java.base/java.util.Optional.map(Optional.java:260)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.mergeForPut(DomainObjectReader.java:148)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader$MergingPropertyHandler.lambda$doWithPersistentProperty$0(DomainObjectReader.java:714)
    at java.base/java.util.Optional.map(Optional.java:260)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader$MergingPropertyHandler.lambda$doWithPersistentProperty$1(DomainObjectReader.java:714)
    at java.base/java.util.Optional.flatMap(Optional.java:289)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader$MergingPropertyHandler.doWithPersistentProperty(DomainObjectReader.java:714)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader$LinkedAssociationSkippingAssociationHandler.doWithAssociation(DomainObjectReader.java:642)
    at org.springframework.data.mapping.model.BasicPersistentEntity.doWithAssociations(BasicPersistentEntity.java:351)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.lambda$mergeForPut$0(DomainObjectReader.java:162)
    at java.base/java.util.Optional.map(Optional.java:260)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.mergeForPut(DomainObjectReader.java:148)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.lambda$mergeCollections$7(DomainObjectReader.java:511)
    at java.base/java.util.Optional.map(Optional.java:260)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.mergeCollections(DomainObjectReader.java:494)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader$MergingPropertyHandler.doWithPersistentProperty(DomainObjectReader.java:709)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader$LinkedAssociationSkippingAssociationHandler.doWithAssociation(DomainObjectReader.java:642)
    at org.springframework.data.mapping.model.BasicPersistentEntity.doWithAssociations(BasicPersistentEntity.java:351)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.lambda$mergeForPut$0(DomainObjectReader.java:162)
    at java.base/java.util.Optional.map(Optional.java:260)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.mergeForPut(DomainObjectReader.java:148)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader$MergingPropertyHandler.lambda$doWithPersistentProperty$0(DomainObjectReader.java:714)
    at java.base/java.util.Optional.map(Optional.java:260)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader$MergingPropertyHandler.lambda$doWithPersistentProperty$1(DomainObjectReader.java:714)
    at java.base/java.util.Optional.flatMap(Optional.java:289)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader$MergingPropertyHandler.doWithPersistentProperty(DomainObjectReader.java:714)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader$LinkedAssociationSkippingAssociationHandler.doWithAssociation(DomainObjectReader.java:642)
    at org.springframework.data.mapping.model.BasicPersistentEntity.doWithAssociations(BasicPersistentEntity.java:351)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.lambda$mergeForPut$0(DomainObjectReader.java:162)
    at java.base/java.util.Optional.map(Optional.java:260)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.mergeForPut(DomainObjectReader.java:148)
    at org.springframework.data.rest.webmvc.json.DomainObjectReader.lambda$mergeCollections$7(DomainObjectReader.java:511)
    at java.base/java.util.Optional.map(Optional.java:260)
...
odrotbohm commented 3 months ago

It sounds like what you're trying to do is not achievable through a simple merge patch, as it violates the array handling semantics. That aside, bi-directional relationship are a notoriously known for causing trouble. So unless you can get plain Jackson to produce a merge result, I don't think we're going to introduce more complexity to handle this situation.

Have you tried using actual JSON Patch requests that allow a much more precise definition of what's supposed to be updated in which way? Alternatively, if the state transition is a domain-relevant one, it might be worth introducing a dedicated link relationship for it and manually handle the request.

Zetten commented 3 months ago

Thanks for the tips.

I'll spend a few minutes testing Jackson merging - I thought the @JsonManagedReference and @JsonBackReference should help there.

Have you tried using actual JSON Patch requests that allow a much more precise definition of what's supposed to be updated in which way?

Yeah, that's been a dead end so far - it seems that patching one property on two levels of nested objects just doesn't resolve correctly (having changed Set collectors to a List for index-based access):

[{
    "op": "replace",
    "path": "outputs/0/collectors/0/replaceContents",
    "value": true
}]

Leads to Couldn't find writable property for pointer segment replaceContents on class com.example.demo.entities.TaskOutput in outputs/0/collectors/0/replaceContents.

Interestingly it does seem to work for the collectors objects themselves - i.e. I could try to replace outputs/0/collectors/0 entirely - but then I have a backreference issue, as I'm also using @MapsId to pull an embedded composite ID out of the TaskOutputCollector#taskOutput property, so I would have to add IDs to my entity responses and rebuild everything just to patch one property!

Alternatively, if the state transition is a domain-relevant one, it might be worth introducing a dedicated link relationship for it and manually handle the request.

But I think this will be the ultimate way forward. It's going to be required for another feature I've got in mind anyway!

Zetten commented 3 months ago

Yeah, I'll just continue with my refactoring to a dedicated entity for the relationship - good excuse for it now :smile:

I do wonder if checking for Jackson's @JsonBackReference could be used as a simple heuristic to escape circular references, as that should only exist in such circumstance. The check could be e.g. in DomainObjectReader#doWitthPersistentProperty (or implemented as a isBackReference() on something in the PersistentProperty hierarchy) - but admittedly I don't fully grok how all the Spring Data bits fit together so I'm not sure what the impact/tradeoffs could be!