Patch request on an Entity with @Version/ETag fails when using a projection [DATAREST-1213]

[spring-projects-issues]

Dario Seidl opened DATAREST-1213 and commented

Hello!

I have a Spring Boot 2 project, with Spring Data Rest 3.0.5.

When I create an Entity with a @Version field to generate ETags, and I make a patch request on that entity with a projection, I get the following error from an assertion in ETag.getVersionInformation:

2018-03-13 11:37:57.459 ERROR 3951 --- [o-auto-1-exec-2] o.s.d.r.w.RepositoryRestExceptionHandler : Target bean of type com.sun.proxy.$Proxy129 is not of type of the persistent entity (at.darioseidl.etagdemo.Child)!

java.lang.IllegalArgumentException: Target bean of type com.sun.proxy.$Proxy129 is not of type of the persistent entity (at.darioseidl.etagdemo.Child)!
	at org.springframework.util.Assert.isTrue(Assert.java:134)
	at org.springframework.data.mapping.model.BasicPersistentEntity.getPropertyAccessor(BasicPersistentEntity.java:427)
	at org.springframework.data.rest.webmvc.support.ETag.getVersionInformation(ETag.java:170)
	at org.springframework.data.rest.webmvc.support.ETag.from(ETag.java:88)
	at org.springframework.data.rest.webmvc.HttpHeadersPreparer.prepareHeaders(HttpHeadersPreparer.java:80)
	at org.springframework.data.rest.webmvc.HttpHeadersPreparer.lambda$prepareHeaders$0(HttpHeadersPreparer.java:62)
	at java.util.Optional.map(Optional.java:215)
	at org.springframework.data.rest.webmvc.HttpHeadersPreparer.prepareHeaders(HttpHeadersPreparer.java:62)
	at org.springframework.data.rest.webmvc.RepositoryEntityController.saveAndReturn(RepositoryEntityController.java:452)
	at org.springframework.data.rest.webmvc.RepositoryEntityController.patchItemResource(RepositoryEntityController.java:397)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
	at java.lang.reflect.Method.invoke(Method.java:498)
	at org.springframework.web.method.support.InvocableHandlerMethod.doInvoke(InvocableHandlerMethod.java:209)
	at org.springframework.web.method.support.InvocableHandlerMethod.invokeForRequest(InvocableHandlerMethod.java:136)
	at org.springframework.web.servlet.mvc.method.annotation.ServletInvocableHandlerMethod.invokeAndHandle(ServletInvocableHandlerMethod.java:102)
	at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.invokeHandlerMethod(RequestMappingHandlerAdapter.java:870)
	at org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerAdapter.handleInternal(RequestMappingHandlerAdapter.java:776)
	at org.springframework.web.servlet.mvc.method.AbstractHandlerMethodAdapter.handle(AbstractHandlerMethodAdapter.java:87)
	at org.springframework.web.servlet.DispatcherServlet.doDispatch(DispatcherServlet.java:991)
	at org.springframework.web.servlet.DispatcherServlet.doService(DispatcherServlet.java:925)
	at org.springframework.web.servlet.FrameworkServlet.processRequest(FrameworkServlet.java:978)
	at org.springframework.web.servlet.FrameworkServlet.service(FrameworkServlet.java:852)
	at javax.servlet.http.HttpServlet.service(HttpServlet.java:742)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:231)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
	at org.apache.tomcat.websocket.server.WsFilter.doFilter(WsFilter.java:52)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
	at org.springframework.web.filter.RequestContextFilter.doFilterInternal(RequestContextFilter.java:99)
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
	at org.springframework.web.filter.HttpPutFormContentFilter.doFilterInternal(HttpPutFormContentFilter.java:109)
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
	at org.springframework.web.filter.HiddenHttpMethodFilter.doFilterInternal(HiddenHttpMethodFilter.java:81)
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
	at org.springframework.web.filter.CharacterEncodingFilter.doFilterInternal(CharacterEncodingFilter.java:200)
	at org.springframework.web.filter.OncePerRequestFilter.doFilter(OncePerRequestFilter.java:107)
	at org.apache.catalina.core.ApplicationFilterChain.internalDoFilter(ApplicationFilterChain.java:193)
	at org.apache.catalina.core.ApplicationFilterChain.doFilter(ApplicationFilterChain.java:166)
	at org.apache.catalina.core.StandardWrapperValve.invoke(StandardWrapperValve.java:199)
	at org.apache.catalina.core.StandardContextValve.invoke(StandardContextValve.java:96)
	at org.apache.catalina.authenticator.AuthenticatorBase.invoke(AuthenticatorBase.java:496)
	at org.apache.catalina.core.StandardHostValve.invoke(StandardHostValve.java:140)
	at org.apache.catalina.valves.ErrorReportValve.invoke(ErrorReportValve.java:81)
	at org.apache.catalina.core.StandardEngineValve.invoke(StandardEngineValve.java:87)
	at org.apache.catalina.connector.CoyoteAdapter.service(CoyoteAdapter.java:342)
	at org.apache.coyote.http11.Http11Processor.service(Http11Processor.java:803)
	at org.apache.coyote.AbstractProcessorLight.process(AbstractProcessorLight.java:66)
	at org.apache.coyote.AbstractProtocol$ConnectionHandler.process(AbstractProtocol.java:790)
	at org.apache.tomcat.util.net.NioEndpoint$SocketProcessor.doRun(NioEndpoint.java:1459)
	at org.apache.tomcat.util.net.SocketProcessorBase.run(SocketProcessorBase.java:49)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1142)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:617)
	at org.apache.tomcat.util.threads.TaskThread$WrappingRunnable.run(TaskThread.java:61)
	at java.lang.Thread.run(Thread.java:745)

I understand that a projection would not affect the patch, but when using return-body-on-update, it would make sense to use a projection to determine the result body of a patch.

I have created a sample project on Github. Simply run the tests to see the problem.

This looks very similar to bug report DATAREST-581, but this one has been marked as resolved a long time ago

---

Affects: 3.0.5 (Kay SR5)

Reference URL: https://github.com/darioseidl/etagdemo

Referenced from: pull request #355

Backported to: 3.3.5 (Neumann SR5), 3.2.11 (Moore SR11)

4 votes, 4 watchers

[spring-projects-issues]

Dario Seidl commented

In the last project, we managed to work around this issue by avoiding patch with projections.

Now I'm facing the problem again in a different project after upgrading to Spring Boot 2. This time we don't even have any @Version or @LastModifiedDate on our entities and don't need/want any ETag headers, but the assertion still ETag.getVersionInformation fails.

Is there a way to disable the HttpHeadersPreparer or the ETag altogether?

[spring-projects-issues]

Oliver Drotbohm commented

I'm not sure I want to go forward with this. As of now (and documented PATCH and PUT requests are not even supposed to support projections and even with this one fixed, I am pretty sure there are many other parts of the codebase that break trying to use them for those. You might get them to work in simple cases but e.g. open projections clearly won't work as the contain synthetic properties. I am even pretty sure that you'd never even get a properly projected instance created for a PUT request based on the current code, i.e. the assumption in your tests that the payload would consist of a projection instance is invalid in the first place. The PersistentEntityResource payload is created by PersistentEntityResourceHandlerMethodArgumentResolver and that one doesn't even know about projections in the first place.

To handle the situation better I'd like to find out what's causing the instance returned by payload.getContent() to be a proxy in the first place. For updates we actually trigger a by-id lookup which usually does not return proxies in the first place unless you've done something special to the repositories, altered that lookup or the like. Can you elaborate on that please?

[spring-projects-issues]

Dario Seidl commented

Thank you for the answer.

I understand not wanting to support this if it is not supposed to work. However, it did work in previous versions, and seems to work with some adjustments. I'd be happy to look some more into it.

I narrowed down where the proxy is created. You're right the PersistentEntityResource that is created by the PersistentEntityResourceHandlerMethodArgumentResolver is unaffected by the peojection and is never a proxy.

In the end, in RepositoryEntityController.saveAndReturn called by both putItemResource and patchItemResource, the PersistentEntityResourceAssembler has the information about the projection and in assembler.toFullResource the proxy is created/returned (calling ProxyProjectionFactory.createProjection internally).

I don't know about all cases, but patch and put with open projections (with a @Value property) do seem to work, except for this ETag/proxy issue. I don't see why it wouldn't be supposed to work?

Please take a the linked etagdemo project. It's a very simple project with a few tests to reproduce the issue. There is nothing special done to the repositories there, AFAIK. In this project I have only customized the HttpHeadersPreparer to workaround the issue. When using the default HttpHeadersPreparer the tests will fail, with the CustomHttpHeadersPreparer they succeed

[spring-projects-issues]

Dario Seidl commented

I have updated the pull request with web tests and moved the the code to unproxy the JDK proxy to the HttpHeadersPreparer. I believe this works and shouldn't break anything, but I guess the unproxy part can still be considered hacky. At least it should show clearly where to problem is:

In RepositoryEntityController.saveAndReturn, assembler.toFullResource returns a proxy and the proxy is passed to headersPreparer.prepareHeaders.

Maybe you have another idea how this could be solved in a cleaner way?

[spring-projects-issues]

Oliver Drotbohm commented

I think I now better understand the situation and you're basically right: PersistentEntityResourceAssembler encapsulates the process of applying a projection which evaluates the request parameter and thus creates a proxy. PersistentEntityResource still carries the correct PersistentEntity but HttpHeadersPreparer takes them apart and ….prepareHeaders(PersistentEntity, Object) assumes the latter to be matching the former which in case of projections is not given.

I think it's fair to assume that for that particular overload this is totally fine and we should probably add an assertion for this to make sure we fail earlier. I think adding a getTargetEntity(…) method to PersistentEntityResource that does the deep lookup and using that from prepareHeaders(PersistentEntityResource) is the best way to fix the issue as that would allow code using PER to make use of the unwapping effectively.

Feel free to update the PR accordingly or ping me in case you want me to take over. Happy to take it from here. Thanks for the vey helpful ping pong already!

[spring-projects-issues]

Dario Seidl commented

OK, great! I've updated the pull request with the getter and an assertion in prepareHeaders. I'd be glad if you could take another look at it and take it from there

[spring-projects-issues]

Dario Seidl commented

Hello, just checking in to ask if this is ready to be merged? Let me know if you need something else from me

[spring-projects-issues]

Josh Wand commented

This is biting us on upgrading our project too. Oliver Drotbohm would appreciate reviewing/merging the PR! 

Thanks!

[spring-projects-issues]

Prashant Srivastava commented

Hi Its failing for 

RepositoryEntityController.createAndReturn(RepositoryEntityController.java:486) if we give projection in POST call

prepareHeaders code need to be change for this Error:

-public HttpHeaders prepareHeaders(Optional<PersistentEntityResource> resource) {-

 -- 

-return resource//-

-.map(it -> prepareHeaders(it.getPersistentEntity(), it.getContent()))//-

-.orElseGet(() -> new HttpHeaders());-

-}-

public HttpHeaders prepareHeaders(Optional<PersistentEntityResource> resource)

{   return resource// .map(it -> prepareHeaders(it.getPersistentEntity(), it.getTargetEntity()))// .orElseGet(() -> new HttpHeaders()); }

[spring-projects-issues]

Adrian Fish commented

Oliver Drotbohm This issue is hitting us on the Sakai project. We just updated to Spring 5 and data rest 3.3.4 and our patch updates to our entities are failing to return our projected JSON. Same error as above.

Dario Seidl's PR looks sensible to me. If it can't just be merged, is there a workaround?

[spring-projects-issues]

Adrian Fish commented

Mark Paluch Do you have any insight into this issue? The manifestation of this is that projected entities can't have the request body returned because of a mismatch between a proxied and an unproxied entity. It seems to me that I can either run a patched spring data rest (don't want to) or rewrite my client side components to just not expect a body returned, and disable body returning on the server. Another option would be to be able to just turn etag generation off, make it configurable, but I can't find any docs on whether that's possible. In other words, just duck that entity comparison. Dario Seidl's patch works, or worked, but maybe the team wants to dig a little deeper

[spring-projects-issues]

Adrian Fish commented

Any update on this? Unless this gets fixed I'm going to have to rewrite a chunk of code, just because we upgraded to the latest data rest. I'm sure there are others in the same boat

[spring-projects-issues]

Dario Seidl commented

It's been a while since I've dealt with this, but we are also still affected in one project by this bug.

From what I remember: there is no way to generally disable ETags, at least not in a way that would workaround this issue. If an entity doesn't have a @Version, no ETag headers will be generated, but that doesn't help here.

We had a relatively big project without @Version or Etags, that relied on return-body-on-update, and when we upgraded it to Spring Boot 2 all PATCH and PUT requests with projetions stopped working.

The only way we found to make it work again was applying the workaround, that I have submitted as a patch here too. Otherwise we would have to rewrite all PATCHes and PUTs to be followed by a GET to get the updated version.

So we would too be very happy if this could be merged or resolved in another way. For what it's worth, our project has been running with that patch for 2+ years without issues now

[spring-projects-issues]

Oliver Drotbohm commented

Sorry for the unusually long delay. I think I can get this integrated into the services releases coming up next week

[spring-projects-issues]

Adrian Fish commented

That's great news! Thanks

[spring-projects-issues]

Oliver Drotbohm commented

That's in place now. Feel free to give the snapshots a spin. Release due on Wednesday

[spring-projects-issues]

Dario Seidl commented

Great, thank you! I gave it a quick try with Spring Boot 2.3.5 and Data Rest 3.3.5 and it seems to be working fine. :)