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

Switch to disable PUT for creation on item resources by default [DATAREST-948] #1318

Closed
spring-projects-issues opened this issue Nov 25, 2016 · 18 comments

Comments

@spring-projects-issues
Copy link

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

Abhijit Sarkar opened DATAREST-948 and commented

PUT /\{resource}/\{id} ends up creating a new resource if it doesn't exist. According to the doc for the item resource, the behavior should be as follows:

Replaces the state of the target resource with the supplied request body.

Thus for a non-existent id, I'd expect a HTTP status 404, not a new resource (you can't "replace" something that doesn't exist). I do not expect the user to call my API with a given id to use for a new resource, because the id almost always is internal implementation specific (like an auto-increment key in the DB). Note that POST /\{resource\} behavior is to create a new item resource and add it to the collection.


Affects: 2.5.5 (Hopper SR5)

Issue Links:

  • DATAREST-180 Update (Http PUT) operation should not create new entity

  • DATAREST-1034 Allow overriding exposure defined at the type level on the method

Referenced from: commits ca668eb, c5c7fa9, c3101b3, 28383ba, a145405, 1762b45, c5a3262, c355c75

3 votes, 9 watchers

@spring-projects-issues
Copy link
Author

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

Oliver Drotbohm commented

Well, generally speaking there's nothing wrong with clients creating new resources directly (see this example). Meaning, nothing in HTTP or REST forbids that. However, I agree, that it is not the most common pattern used and I agree we should not support this by default.

I'm going to introduce a configuration option to selectively activate that per aggregate root type and default to rejection for the upcoming Ingalls release train

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 25, 2017

Mike Snare commented

Any updates on this? Ran into this today and it was definitely surprising to write a test expecting a 404 and instead get back a 201 from a PUT to non-existent resource.

Running Spring boot 1.5.3.RELEASE which has data rest version 2.6.3.RELEASE.

Is there at least any sort of workaround? E.g. some way to intercept requests to a PUT endpoint and throw the exception myself?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Aug 26, 2017

Abhijit Sarkar commented

@olivergierke, any word on this?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 6, 2018

Adam Rosini commented

@olivergierke I mentioned this in the gitter about a month ago, you said to create a new issue for "selectively expos[ing] HTTP methods". Is just repurposing this enough?

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 7, 2018

Oliver Drotbohm commented

I just published a raw and unpolished API draft for some manual exposure configuration that works as follows:

RepositoryRestConfiguration config =ExposureConfiguration exposure = config.getExposureConfiguration();
// global configuration, similar API available for item and association resources
exposure.withCollectionExposure((metadata, methods) -> methods.disable(HttpMethod.POST));
// selectively by domain type
exposure.forDomainType(User.class).withCollectionExposure(…);
// global shortcuts for common use cases
exposure.disablePutForItemResource();

The API is available if you refer to version 3.1.0.DATAREST-948-SNAPSHOT. Please give the snapshots a try and provide feedback

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 8, 2018

Eugene Tenkaev commented

The creation of resource in PUT method should be disabled by default, I was very confused when resource was created.

@olivergierke About proposed API, it looks like you disable entire PUT for resource. But I need to disable only creation behaviour of PUT, not entire PUT method. I'm talking about this line:

exposure.disablePutForItemResource();

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 8, 2018

Oliver Drotbohm commented

While I agree that we should revisit the default, it's not something we can easily change in a minor version as it would break systems that rely on that default.

Regarding the configuration API, you're right, we need more than that. I am not sure I want pull in all this dynamic context into the new generic API, so I guess we'd have to keep a dedicated setting for this on ExposureConfiguration and evaluate that in RepositoryEntityController.putItemResource(…). I'll update the branch accordingly

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 8, 2018

Oliver Drotbohm commented

I just pushed the feature branch to now allow ….disablePutForCreation() either globally or on a per-type basis

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 8, 2018

Eugene Tenkaev commented

@olivergierke big thanks for the fast move(y) Also I'm totally agree that changing default behaviour not suitable for minor version change. It will be ok for me if in minor version you provide possibility to change this behaviour and in major the default behaviour will be changed.

Can't wait till this will be available as current version, not snapshot

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Mar 8, 2018

Oliver Drotbohm commented

I'll schedule this for inclusion in Lovelace M2, currently scheduled for end of March

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 4, 2018

Eugene Tenkaev commented

What the progress?
I have switched to: Spring Boot 2 2.0.1.RELEASE and Finchley.RC1. So right now it 3.0.6.RELEASE don't see that API there

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented May 8, 2018

Oliver Drotbohm commented

Sorry about the delay. I didn't manage to produce proper documentation in time for Lovelace M2 so that the feature branch never actually got merged. I've now done so and the APIs are available in the latest snapshots (Lovelace-BUILD-SNAPSHOT or 3.1.0.BUILD-SNAPSHOT respectively).

I've also pushed a preview of the documentation. Feedback highly appreciated

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jul 25, 2018

Steven Kordonowy commented

Hi Oliver Drotbohm, I noticed this is marked as resolved, will the branch https://github.com/spring-projects/spring-data-rest/tree/issue/DATAREST-948 still get a pull request? Maybe I am misunderstanding and it is already part of 3.1.0.BUILD-SNAPSHOT but I could not see it there.

Thanks!

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jul 25, 2018

Oliver Drotbohm commented

As indicated in the ticket metadata, this will be part of the RC1 release of Lovelace due tomorrow. The relevant commit on master is here

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Jul 25, 2018

Steven Kordonowy commented

Sounds great, thanks! Excited to get this fix, this is exactly what I need at the moment!

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 18, 2018

Predrag commented

Hello,

 

Is there any chance of this being backported to 3.0.x  ?

 

Pretty please ?

 

Many thanks !

 

 

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 18, 2018

Oliver Drotbohm commented

No, there is not as this introduces new API and thus doesn't qualify as bugfix

@spring-projects-issues
Copy link
Author

@spring-projects-issues spring-projects-issues commented Oct 18, 2018

Predrag commented

ok thanks for the reply !

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

Successfully merging a pull request may close this issue.

None yet
2 participants