linkeddata / rdflib.js

Linked Data API for JavaScript
http://linkeddata.github.io/rdflib.js/doc/
Other
565 stars 143 forks source link

Accept accept-patch header as well as MS_Author_via #473

Closed timbl closed 10 months ago

timbl commented 3 years ago

Accept-patch: is the header which the Solid Spec defines for finding out what content-types can be used for PATCH requests, not MS_Author_Via which rdflib currently uses in the UpdateManager. Suggest accept bth, for back compatibility.

timbl commented 3 years ago

This used to prevent the solid-ui stack working with CSS, until CSS added both headers.

bourgeoa commented 3 years ago

To be checked in node-solid-server and solid-rest

jeff-zucker commented 3 years ago

Solid-Rest sends both accept-patch and ms-author-via.

angelo-v commented 2 years ago

ESS only supports accept-patch, so it does not work with ESS currently

angelo-v commented 2 years ago

accept-patch is actually already respected by the update-manager editable function and it works for existing resources. But what about not-yet-existing resources? CSS nevertheless returns an accept-path header. And NSS returns the ms-author-via for 404 resources. But ESS does not send any of those headers, therefore it is not possible to create a resources with the update-manager update function.

Should the Spec enforce accept-patch even on 404 responses? Should rdflib just use PUT in that case? What else could be done?

csarven commented 2 years ago

I'd say that the https://solidproject.org/ED/protocol#server-accept-headers requirement (intended to) covers 404 responses. "Authorized" typically entailing something other than 403.

jeff-zucker commented 2 years ago

The rdflib updater code checks for ms-author-via and support-patch headers and it it doesn't find them it does this:

if ( !this.isHttpUri(uri as string) ) {
            if( !wacAllow ) return false;
            else return 'LOCALFILE';
          }

In other words, it uses PUT instead of PATCH for file:/// and other non-http URLs. Since it respects wacAllow, it would seem safe to just return LOCALFILE for all URLs with the missing headers. This would mean that all resources that allow editing according to the wacallow header would be editable, not just the non-http ones.

angelo-v commented 2 years ago

I'd say that the https://solidproject.org/ED/protocol#server-accept-headers requirement (intended to) covers 404 responses. "Authorized" typically entailing something other than 403.

Thanks a lot, I read it the same way, but there is an important addition: The Accept-* headers have to "correspond to acceptable HTTP methods listed in Allow header"

and right above that paragraph:

Servers MUST indicate the HTTP methods supported by the target resource by generating an Allow header field in successful responses.

So it seems to be spec conformal to omit the Allow header and in consequence the Accept-* headers in unsuccessful responses. This is very anoying. How can an app know how to create the non-existing resource? Should we change the spec in that regard?

angelo-v commented 2 years ago

Since it respects wacAllow, it would seem safe to just return LOCALFILE for all URLs with the missing headers. This would mean that all resources that allow editing according to the wacallow header would be editable, not just the non-http ones.

Returning LOCALFILE for a HTTP resource just sounds wrong. If it is just a naming issue we could rename it to "PUT" or something. But: ESS also does not return any WAC header since it uses ACP, so this is not an option for ESS anyway.

PS: of course we should discuss wether rdflibjs should support ACP, but perhaps in a seperate issue

csarven commented 2 years ago

So it seems to be spec conformal to omit the Allow header and in consequence the Accept-* headers in unsuccessful responses.

I don't think that's correct or accurate.

How can an app know how to create the non-existing resource?

I break that question down to: which HTTP method can be used to create and knowing if a resource at a specific target can be created:

# PUT, PATCH can be used to create /foo
HEAD /foo

404
Allow: GET, HEAD, OPTIONS, POST, PUT, PATCH, DELETE
Accept-Put: text/turtle
WAC-Allow: user="write read", public="read"

# It doesn't (generally) matter if Accept-* or WAC-Allow is present in the response.
# The request target is not wired to support PUT, PATCH at the time of this particular request.
# PUT, PATCH attempts will likely get a 405 response.
HEAD /bar

404
Allow: GET, HEAD, OPTIONS

# These methods are supported by the resource.
# Existence of the resource isn't known but user has permission to create or update.
HEAD /baz

403
Allow: GET, HEAD, OPTIONS, POST, PUT, PATCH, DELETE
WAC-Allow: user="write"

# Can conclude without the Allow header:
# POST and PATCH can be used to append to the target resource if it exists.
# POST and PATCH with the Slug header can be used to attempt to create the target resource if it doesn't exist.
OPTIONS /qux

204
WAC-Allow: user="append"

# Can conclude without the Allow and WAC-allow headers:
# PATCH can be used to attempt to create the target resource.
HEAD /quxx

404
Accept-Patch: text/n3
angelo-v commented 2 years ago

So it seems to be spec conformal to omit the Allow header and in consequence the Accept-* headers in unsuccessful responses.

I don't think that's correct or accurate.

Are you refering to the spec or to my statement? :)

How can an app know how to create the non-existing resource?

I break that question down to: which HTTP method can be used to create and knowing if a resource at a specific target can be created:

All your examples look fine to me, since they include an Allow header. ESS does not.

But indeed we need to add support for Accept-Put (and Accept-Post?) in rdflib as well.

csarven commented 2 years ago

Are you refering to the spec or to my statement? :)

Your statement but I may have misinterpreted.

I think 404 is most interesting here (e.g., in response to HEAD /quxx). Even without Allow or Accept-* headers, why wouldn't an application use PUT or PATCH /quxx (or possibly POST / with Slug) to attempt to create the resource it needs to?

I updated the examples above.

angelo-v commented 2 years ago

What I tried to say was: The Accept-* headers are a MUST, but only if the respective method is listed in the Allow header. If the server omits the Allow header it can also omit the Accept-* headers leaving the client without any clue how to create the resource.

Yes the client could just guess, but why should it assume any of those methods will work? If e.g. PUT is supported, why does the server not list it in the Allow header?

The spec could permit to omit the Allow header in case of 401 or 403 but I think 404 should mandate to include it to tell the client how to create the resource.

Nevertheless, regarding rdflib, we should perhaps at least try a PUT request in such cases. What do you think @jeff-zucker ?

jeff-zucker commented 2 years ago

@angelo-v - I think you are right that it is best to clean this up in the spec. However the LOCALFILE hack could be a way to make things work until such time as that happens. I think this will hit SolidOS worst in the case of forms which always use PATCH except for LOCALFILE which uses PUT. So a form that has a missing forms-result resource (i.e. that is expected to create the resource) would fail without the Accept headers unless we use something like the LOCALFILE bypass.

RubenVerborgh commented 2 years ago

accept-patch is actually already respected by the update-manager editable function

Perhaps "respected" is a strong word, because the only possible outcome is "SPARQL": https://github.com/linkeddata/rdflib.js/blob/c14dfd57d5159ad5ac1ee2523cc7924968e24f53/src/update-manager.ts#L144-L151 and the spec does not prescribe the SPARQL format.

So is the real TODO here to support N3 Patch?

How can an app know how to create the non-existing resource?

In 0.9, Solid servers MUST accept N3 Patch. So even if the Accept-Patch header is missing on 404 (which I don't think it should, but bear with me for a second), then the outcome of the function determining what is supported should still be "N3 Patch". I.e. the logic could/should be:

If this algorithm is present, then CSS can safely remove the non-standard, rdflib-specific configuration (https://github.com/CommunitySolidServer/CommunitySolidServer/issues/1412).

damooo commented 1 year ago

Accept-Patch: text/n3, should better be supported, if not to force any spec compliant solid server to wade through solidos code, to figure out why it is returning random update: Error: Update: Loaded <http://localhost:8000/test/tr1/index.ttl>but stil can't figure out what editing protcol it supports.