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

Deserialization of request body for creating PUT requests has to consider identifier derived from the URI [DATAREST-1050] #1396

Closed
spring-projects-issues opened this issue Apr 7, 2017 · 8 comments
Assignees
Milestone

Comments

@spring-projects-issues
Copy link

spring-projects-issues commented Apr 7, 2017

Marc Easen opened DATAREST-1050 and commented

Within the RepositoryEntityController the putItemResource() method will check to see if an item exists, if it does not it will proceed to create it. This is not idempotent/safe.

This bug was invoked by using @RepositoryRestResource which uses the RepositoryEntityController behind the scenes.

In the scenario where the entity/resource has a auto generated ID, the putItemResource() is able to create multiple items.

RFC2616 section 9.1.2 (https://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html) states: "_Methods can also have the property of "idempotence" in that (aside from error or expiration issues) the side-effects of N > 0 identical requests is the same as for a single request. The methods GET, HEAD, PUT and DELETE share this property. _"

The change in PUT functionality from being update only to create or update can be traced back to "DATAREST-348 - Support for json-patch+json and merge-patch+json media types"


Affects: 2.6.1 (Ingalls SR1), 2.5.8 (Hopper SR8)

Issue Links:

  • DATAREST-1304 PUT and PATCH don't work, when custom entity lookup is configured
    ("is depended on by")

Backported to: 2.6.2 (Ingalls SR2), 2.5.9 (Hopper SR9)

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Apr 10, 2017

Oliver Drotbohm commented

I'm not sure I get the point here. Creating instances via PUT is completely fine, as long as subsequent calls to the URI cause updates. That's exactly the point of idempotency. You can repeat the request as often as you want. The outcome — a particular resource state at that particular URI — will stay the same. So your conclusion about the general behaviour not being idempotent (stated in the first sentence) is incorrect.

If, however, the current implementation causes subsequent releases to create new instances as well, then we have to consider this a bug of course. As far as I can see, the PersistentEntityResource created for a PUT request tries to lookup an existing instance with the identifier derived from the URI (see PersistentEntityResourceHandlerMethodArgumentResolver.resolveArgument(…). If that lookup returns an instance, the entire request is considered an update to the existing resource. I.e. you shouldn't be able to create multiple aggregate instances by PUTting to an item resource URI. I'd be happy to see a sample project that shows the erroneous behaviour.

We unfortunately can't apply the change provided in the PR as it's a behaviour change that some of our users rely on. The fact aside that simply changing a couple of lines in the code without proper test cases is not something that's gonna cut it anyway

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Apr 10, 2017

Marc Easen commented

Take the case where the ID of the entity is randomly generated server side (i.e. a UUID). If you make multiple PUT requests, the controller will generate you multiple resources - which means the current implementation is not idempotent.

If uses are relying on this PUT behaviour then they have unfortunately misunderstood RFC2616, PUT should not be used to create resource. No other REST framework that I could find supports this odd PUT behaviour.

I am willing to add some unit tests around this, but as it stands the PR didn't break any unit tests and I couldn't find any other unit tests that I could base as test from

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Apr 10, 2017

Oliver Drotbohm commented

It feels you're confusing quite a few things here. If a PUT request is issued against an item resource, there already is a well defined ID contained in the URI, so there is no id generation going on. As a REST framework we can't make any assumptions of the underlying persistence mechanism.

You also seem to really misread section 9.6 of RFC2616. In no way it forbids resource creation. Literally, the third sentence explicitly states:

If the Request-URI does not point to an existing resource, and that URI is capable of being defined as a new resource by the requesting user agent, the origin server can create the resource with that URI.

So we're exactly following the spec here. There is no "odd PUT behavior". Again, I am talking about the general expectations of what a PUT is allowed to do and what not. There seems to be a very fundamental misunderstanding on your side. I think we need to resolve that first as we cannot discuss consequences based on invalid assumptions.

That said, I think you're still casting some light on a corner case that might cause issues here: we currently don't force the identifier supplied in the URI onto the aggregate root that has been deserialized from the request. That might cause identifier generation to kick in on the call to the persistence layer, which on subsequent requests would still necessarily not obtain the instance created and thus indeed again create a new instance.

The fix for that would be to force the identifier derived from the URI on the deserialized instance, so that either the persistence backend would either reject that operation as it expects identifier generation but sees a predefined one, or at least allow transparent creation and updates to the same resource.

tl;dr — I think you have a point. I just think your conclusion of how to fix this is invalid and based on a misunderstanding about what PUT is specified to achieve

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Apr 10, 2017

Peter Coulton commented

It's almost following the spec, but not in the case of a generated id.

If the Request-URI does not point to an existing resource, and that URI is capable of being defined as a new resource by the requesting user agent, the origin server can create the resource with that URI.

So we're exactly following the spec here.

I think the important part of that quote is with that URI. In the current implementation the created resource will not match the request URI. This means that multiple PUT requests to a single resource URI causes new resources to be created each time, this is not idempotent.

For example:

Request:
PUT /resources/30dc42d7-8b42-4a36-ae12-8083cfa43599

Response:
Status: 201
Location: /resources/7e4ede28-b020-461b-830a-e403402f6306

The request URI and the created resource URI are not the same, breaking the idempotence of the PUT request.

In this scenario it's not possible to create the resource at the URI of the request because we should not allow the client to specify the id and there isn't a resource already at the URI. This means the request must fail.

Also, from RFC 2616:

[...] the URI in a PUT request identifies the entity enclosed with the request -- the user agent knows what URI is intended and the server MUST NOT attempt to apply the request to some other resource

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Apr 10, 2017

Oliver Drotbohm commented

You're basically validating my point. However, this is not what the original poster described, claimed in his first comment and provided a fix for. The argument was that resource creation via PUT is non-idempotent (which it is not) and something the spec forbids (which it doesn't).

The spec doesn't know anything about ids except URI. Moreover it doesn't know anything about generation of resource ids. In fact, it even must not. So there's a bug in our implementation that needs to be more defensive and force the backend identifier provided via the URI on the deserialized aggregate instance.

I'll update the ticket summary accordingly and prepare a fix

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Apr 10, 2017

Marc Easen commented

Upon reading the spec I concede that I was wrong with regards to using PUT to create a resource, which maybe valid in some situations
e.g. PUT /users/easen. However I think we all agree this can be problem when the ID's are automatically generated - which was stated in my original comment.

The other reason why I raised this it wasn't clear from DATAREST-348 that if this change was desired or was implemented by accident.

Can you explain what you mean by

So there's a bug in our implementation that needs to be more defensive and force the backend identifier provided via the URI on the deserialized aggregate instance.

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Apr 11, 2017

Oliver Drotbohm commented

Spring Data REST has always supported that kind of arrangement. The code touches you saw for DATAREST-348 we're simply due to a lot of refactorings in that area.

Here's what I mean with that line you quote.

  1. Assume we receive a request like this:\
PUT /foos/4711 { "name" : "Bar" }
  1. That means the result of the deserialization is an instance of Foo with the name property set to "Bar". Foo's identifier however has not been set to 4711. That causes the identifier generation to kick in normally, create some random other id so that subsequent requests like the one shown in 1. would create new objects.
  2. If we go ahead and now make sure that 4711 is also set on the Foo instance created, the backend can either simply store that instance, i.e. skip identifier generation, or detect that generation is intended but a value has ben manually assigned and thus reject that call. If the first path is chosen, a subsequent call like 1. would then find the existing instance with identifier 4711, result in a proper update and thus implement the idempotent nature PUT requires.

We can also think about introducing configuration means to disable the support for creating instances via PUT in the first place. In fact, for 3.0 I am even in favour of disabling it by default, requiring the user to opt in for that support. The support for that had been initially developed before I took over the project and we didn't want to remove it so far as that'd probably break some users. 3.0 is a great chance to do something about this however.

Feel free to create a new ticket that's asking for a configuration flag to support PUT for creation per aggregate type

@spring-projects-issues
Copy link
Author

spring-projects-issues commented Apr 11, 2017

Oliver Drotbohm commented

That should be fixed now. Feel free to give the snapshots a try. And thanks for the discussion! :)

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

No branches or pull requests

2 participants