-
Notifications
You must be signed in to change notification settings - Fork 76
Restrict POST target to existing resources #205
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
Conversation
acoburn
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Big 👍 on this. With this change, the difference between creating resources with POST on the one hand and PUT and PATCH on the other becomes much more clear.
|
We've seen confusion the other way around: a developer using solid-client was confused that POST did not create intermediate Containers (in NSS, I think), whereas PUT did. That does mostly make sense to me. What would the expected course of action when a POST fails because the Container does not exist yet? Is there a situation imaginable in which the failure mode would not immediately be handled by creating the intermediate Containers first? And if so, would that situation occur more often than the one in which you want to create the intermediate Containers anyway? Anecdotally, I'd expect developers to want the intermediate Containers to usually be created. That use case would be more difficult with the current proposal: they will have to inspect failed requests, PUT the intermediate Containers on fail, and then retry the POST. Alternatively, if intermediate Containers would be created automatically, then developers who would not want that would have to do a |
|
I thought that POST was limiting it scope to container creation (folder) for which the server did not choose a name and on the resource (files) side where the server could not create intermediate containers and could choose a resource name. |
👍 for me based on RFC 7231:
and
So a 404 means there is no representation, so no "target resource" to perform the processing. Also, given the second part of the 404 definition, there might be security considerations as well. |
justinwb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I agree with the other reviewers that this is sensible and clears up some expected behavior between POST and PUT/PATCH
|
Vincent, Alain: Note key proposed functional requirements in #160 (comment) which is what some of this stems from:
Server and client need to agree on who allocates a URI to a resource: In RFC 7231:
In Solid spec:
When the client believes that a resource with a particular URI does not have a representation, it can use PUT or PATCH to allocate the URI of a resource. POST is an append operation on an existing resource. In the context of WAC, Write access is required to assign client-specified URI which is typically done with PUT or PATCH. Append access is required for POST. If the container of the target resource of a POST request does not exist, Write access would be required to fulfil the request. POST is not particularly suitable if client wants to specify any part of the URI - but it could try its luck with Slug for the last URI segment. So then an application should not make requests like PUT and PATCH can be used to meet the requirement on client assigning a URI (F2.1), however they can't be used towards server assigning a URI (F2.2). POST is fundamental to having server assign a URI (F2.2). Note: I did not investigate a hybrid functional requirement like: Are there significant use cases? Which HTTP method to use becomes a simpler question to address once client knows its intentions for URI allocation. Edit: Does that clarify or address your concerns? |
I'm merely relaying the confusion we saw with a developer, and which I think served as the trigger for you creating this PR? The confusion stems, I think, from seeing a |
|
I've outlined the rationale behind the design so that it can be inspected further. The PR aims to clarify what method to use for different intentions. Systems can only reliably fulfill F2.1 and F2.2, but not something like F2.3. This is arguably a simple and consistent design. The recommendation to developers is that they should keep their implementations within that (until of course F2.3 is acknowledged and spec'd). |
|
This addresses the discrepancy and that's great. We should update Inrupt's docs to make sure the behaviour is clear. In terms of developer experience I agree with @Vinnl that some developers might expect a "Create" function to handle intermediate containers as well, which might not be addressed in the spec given #160 (comment). In that case we might look into adding a way to do this in the SDK itself. |
That's all fine; but that's a client thing then. Recall that the majority of developer (including myself) will access Solid pods via libraries. |
|
Yes, based on @csarven comment it seems like this is not a change that is going to happen on the spec any time soon. If that's the case, we should look into what can be changed on the libraries to make a better developer experience. |
|
Thanks all. I think we are good to go. Here is a bit more info/support on not requiring POST to create nested containers. #160 (comment) concluded that it would require Slug with shared slash semantics in order to create nested containers with POST - it was deemed that system should do either client or server naming in a reliable way. That was also in context of target container existing. Whereas the central example of this PR is about target a resource which wouldn't necessarily have all of the required containers - targeting an non-existent resource. Just to say that these decisions are consistent. |
There is a slight discrepancy in the spec where it recommends:
and
For example, say
/foo/exists, it is not entirely clear what happens with:The expectation would've been along the lines of creating a new container with a server determined name eg.
/foo/bar/baz/qux/.However, that raises a bit of a question as to why
bar/baz/are created where client determines the name.In order to minimise confusion and to keep things consistent, the PR restricts
POSTto target only existing resources.