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

Support creating a Container using PUT #1465

Closed
Vinnl opened this issue Aug 17, 2020 · 13 comments
Closed

Support creating a Container using PUT #1465

Vinnl opened this issue Aug 17, 2020 · 13 comments

Comments

@Vinnl
Copy link
Contributor

Vinnl commented Aug 17, 2020

According to the Solid spec:

Servers MUST create a container with URI path ending /{id}/ in container / for requests including the HTTP Link header with rel="type" targeting a valid LDP container type.

However, when I send a PUT request to a Container URL with a Link header <http://www.w3.org/ns/ldp#BasicContainer>; rel="type", NSS responds with:

Can't write file: PUT not supported on containers, use POST instead

@bourgeoa
Copy link
Member

This is the new spec. Does it support also creation of missing containers with PUT ?

@Vinnl
Copy link
Contributor Author

Vinnl commented Aug 17, 2020

If you mean "would the workaround of creating a dummy file inside the Container and then deleting it again" still work, then yes:

Servers MUST create intermediate containers and include corresponding containment triples in container representations derived from the URI path component of PUT, POST and PATCH requests.

And yes, that's what I'm currently doing for NSS specifically. This issue is linked from a comment there so I know when I can remove that code :)

@bourgeoa
Copy link
Member

No, I was meaning : if the container URI contains missing container do you create the container and all missing ones or do you return an error.

@csarven
Copy link
Member

csarven commented Aug 17, 2020

@Vinnl The requirement you've originally quoted in this issue is in context of POST. I can update the spec paragraph to clarify if that helps.

NSS doesn't currently support PUT to create a container - only as side-effect when creating a non-container resource. Background/proposal to support creating containers with PUT: #1108 and #513

Spec going forward clarifies slash semantics and hierarchical containment, PUT /path/to/container/ will create all required intermediate containers. Client doesn't need include the Link header with a valid LDP container type for PUT. Server is not expected to observe it.

@bourgeoa
Copy link
Member

@csarven So if I read correctly your comment : Why the new spec does not state that PUT to a container URI (ending with /) is not allowed and should return error (405 or 400) ?

@csarven
Copy link
Member

csarven commented Aug 17, 2020

@bourgeoa Can you please rephrase your question or comment?

It would be great if NSS can accept requests with PUT to create a container, as well as intermediate containers if they don't exist.

Note slash semantics requirement:

The slash character in the URI path indicates hierarchical relationship segments, and enables relative referencing. The semantics of the slash character is shared by servers and clients. Paths ending with a slash denote a container resource.

NSS may also do the following:

Servers MUST respond with the 405 status code to requests using HTTP methods that are not supported by the target resource.

Vinnl added a commit to inrupt/solid-client-js that referenced this issue Aug 18, 2020
Unfortunately it does not support the spec-supported method of
creating a Container:
nodeSolidServer/node-solid-server#1465

As a workaround, we create a dummy file within the Container we
want to create, which should lead to the server creating the
Container on the fly. The dummy file is then immediately deleted
again.

Although this method theoretically works for other servers as well,
it is a bit cumbersome and more error-prone due to the multiple
consecutive requests. Therefore I decided to use it specifically
when we detect NSS's message about not supporting the
spec-prescribed method of creating a Container, so that it can be
easily removed wholesale once NSS is no longer a problem, and the
implementation does not surprise other contributors.
@bourgeoa
Copy link
Member

@csarven
Your rephrase of my comment is not at all what I meant. May be I was wrong, reading the spec and comments I inferred that PUT for a container was not allowed, if that was correct (not sure reading the spec) was asking if it could be more affirmative.

I know and understand building spec is difficult.
Here I am not sure of the final result. I am not asking to change it, just to understand it.

Vinnl added a commit to inrupt/solid-client-js that referenced this issue Aug 18, 2020
Unfortunately it does not support the spec-supported method of
creating a Container:
nodeSolidServer/node-solid-server#1465

As a workaround, we create a dummy file within the Container we
want to create, which should lead to the server creating the
Container on the fly. The dummy file is then immediately deleted
again.

Although this method theoretically works for other servers as well,
it is a bit cumbersome and more error-prone due to the multiple
consecutive requests. Therefore I decided to use it specifically
when we detect NSS's message about not supporting the
spec-prescribed method of creating a Container, so that it can be
easily removed wholesale once NSS is no longer a problem, and the
implementation does not surprise other contributors.
Vinnl added a commit to inrupt/solid-client-js that referenced this issue Aug 18, 2020
Unfortunately it does not support the spec-supported method of
creating a Container:
nodeSolidServer/node-solid-server#1465

As a workaround, we create a dummy file within the Container we
want to create, which should lead to the server creating the
Container on the fly. The dummy file is then immediately deleted
again.

Although this method theoretically works for other servers as well,
it is a bit cumbersome and more error-prone due to the multiple
consecutive requests. Therefore I decided to use it specifically
when we detect NSS's message about not supporting the
spec-prescribed method of creating a Container, so that it can be
easily removed wholesale once NSS is no longer a problem, and the
implementation does not surprise other contributors.
@csarven
Copy link
Member

csarven commented Aug 18, 2020

@bourgeoa Why would I rephrase your comment? I asked you to rephrase what you wrote because it is not at all clear if you are asking a question or making a comment. What I wrote was entirely about the state of NSS and the direction it can take based on the upcoming spec.

If there is anything that's unclear in the spec, please quote the part that we can clarify or help with with respect to NSS. Like I said:

NSS should support PUT to create container - we've known for years that it doesn't - however, going forward, if it can't or even for any specific container (for any reason), it is within its right.

@bourgeoa
Copy link
Member

bourgeoa commented Aug 18, 2020

@csarven
Ok I think the problem arise because I in the same time spoke of NSS and the spec direction.

spec direction :

  • you wrote NSS should support PUT to create container is it a MUST in the new spec ?
  • in the spec writing resources : When a server supports the HTTP PUT, POSTandPATCH methods does it imply that a server MAY support for some resource and not for others, like when you write if it can't or even for any specific container (for any reason), it is within its right. I don't know where to find that in the spec

Sorry if I miss something. Reading spec are really difficult because container is a resource. May I suggest to clarify with non container resource and container. I suppose mostly in the writing resource paragraph.

NSeydoux pushed a commit to inrupt/solid-client-js that referenced this issue Aug 21, 2020
Unfortunately it does not support the spec-supported method of
creating a Container:
nodeSolidServer/node-solid-server#1465

As a workaround, we create a dummy file within the Container we
want to create, which should lead to the server creating the
Container on the fly. The dummy file is then immediately deleted
again.

Although this method theoretically works for other servers as well,
it is a bit cumbersome and more error-prone due to the multiple
consecutive requests. Therefore I decided to use it specifically
when we detect NSS's message about not supporting the
spec-prescribed method of creating a Container, so that it can be
easily removed wholesale once NSS is no longer a problem, and the
implementation does not surprise other contributors.
Vinnl referenced this issue in inrupt/solid-client-js Nov 11, 2020
@ThisIsMissEm
Copy link

@bourgeoa I think the spec is now very clear on all matters talked about above: https://solidproject.org/TR/protocol#writing-resources

Writing Resources

Servers MUST support the HTTP PUT, POST and PATCH methods [RFC7231]. [Source] [Source]

Servers MUST create intermediate containers and include corresponding containment triples in container representations derived from the URI path component of PUT and PATCH requests. [Source]

Might be worth looking into whether we can make NSS support PUT for Containers, that way we can clean up some code that's only used when talking with NSS instances (I'm assuming it'd be as simple as adding a router.put rule alongside a router.post or something)

(note: this isn't a high priority from the DevTools Team side at Inrupt, I just was going through our code, noticed the comment and was curious as to if it still held true, hence now commenting.

@ThisIsMissEm
Copy link

But maybe this was actually closed by the work in #1557 — I'm not sure?

@bourgeoa
Copy link
Member

Yes it was closed with #1557

@ThisIsMissEm
Copy link

Wonderful!

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

No branches or pull requests

4 participants