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.

Sign up for GitHub

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

Jump to bottom

Introduce constraint on PATCH to only patch target resource #349

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Introduce constraint on PATCH to only patch target resource #349

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c582593
Introduce constraint on PATCH to only patch target resource
kjetilk Nov 12, 2021
5746d45
Add that 422 should be returned
kjetilk Nov 12, 2021
b22c3d1
Update protocol.html
kjetilk Nov 14, 2021
d5f8975
Incorporate @RubenVerborgh's feedback
kjetilk Nov 23, 2021
74a7772
Resolve conflict
kjetilk Nov 23, 2021
6339dd2
Object of req level needs hyphen
kjetilk Nov 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions protocol.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,6 +708,11 @@ <h3 property="schema:name">Writing Resources</h3>

<p><span about="" id="server-protect-containment" rel="spec:requirement" resource="#server-protect-containment"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:MUST">MUST</span> NOT allow HTTP <code>POST</code>, <code>PUT</code> and <code>PATCH</code> to update a container’s containment triples; if the server receives such a request, it <span rel="spec:requirementLevel" resource="spec:MUST">MUST</span> respond with a <code>409</code> status code.</span></span> [<a href="https://github.com/solid/specification/issues/40#issuecomment-573358652" rel="cito:citesAsSourceDocument">Source</a>]</p>

<p>
<span about="" id="server-patch-not-other" rel="spec:requirement" resource="#server-patch-not-other"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:MUST-NOT">MUST NOT</span> allow a client to explicitly request multi-resource changes via the <code>PATCH</code> method, but the server is permitted to propagate side-effects of a change to the target resource to other resources.</span></span> [<a href="https://github.com/solid/specification/issues/125#issuecomment-873035679" rel="cito:citesAsSourceDocument">Source</a>]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR title and the first comment is clear to me. But I don't quite understand the way the requirement reads.

The first sentence seems as though it is the payload that will 1) attempt to change different resources 2) besides the request's target resource. And then it says "but" it can propagate the changes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but the server is permitted to propagate side-effects of a change to the target resource to other resources.

I think this bit provides more confusion than clarity. If there are explicit allowed and/or expected side-effects that aren't covered elsewhere in the specification those should be mentioned. For example, if a patch is used to create the target resource R in container P, containment information in P is affected, but that is covered elsewhere in the spec, so it wouldn't need to be mentioned. Are there some specific side-effects we want to address here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I can think of. @RubenVerborgh , would you be OK to revert to the previous commit?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that's correct then, but happy to abstain from voting.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@RubenVerborgh I think the idea is that your concerns are addressed with my generic language elsewhere in the spec. I tend to agree that the last commits are more explicit and is thus clearer. But I also see the point of @csarven and @justinwb , containment triples are dealt with elsewhere, and so, the fact that PATCH can create a resource and thus have the side effect of adding a containment triple, doesn't need to be mentioned here. That some changes to a resource will also have a the side-effect of updating timestamps are also defined elsewhere, again, it doesn't need to be mentioned here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are mentioned in other places, but this statement creates a contradiction with those other places. Whereas the only intention here was to say "PATCH targets one resource".

Copy link
Member

@csarven csarven Nov 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be clear: my review comment above was about the soundness of the statements in the PR. No judgement on the solution until the statements are clarified.

#349 (comment) is the suggestion to make the request semantics more clear and what happens when applied to the target resource.

For example, if the payload of the PATCH were to result in deleting resources besides the resource that's same as the effective request URI, then those are part of the semantics of PATCH with a particular media type. That is not a "side effect". It is part of processing the request. "Side effects" may indeed be things like changes to containment when a resource is created with PATCH; server creating a memento resource of the target resource; adding/tracking creator of the resource; associating an ACL resource and a description resource with the created resource; updating authoritative information, and so on.

Kjetil, I only mentioned containment as a simple example to you in private and later in the editors meeting ( https://github.com/solid/specification/blob/main/meetings/2021-11-24.md#constraint-on-patch-to-target-resource ) . I think Justin understood and agreed with my point in the meeting and repeating it here because you asked him to leave a comment.

Ruben, I'd like to know what potential conflicts you are referring to and how to avoid them. Or are we talking passed each other?

I think we are in agreement with the intention re "PATCH targets one resource" and "the update should only change the description of the target resource", right?

PATCH /foo
INSERT DATA foo bar baz
INSERT DATA qux bar baz
DELETE DATA quux bar baz
INSERT DATA quuz bar baz

That only needs to be processed to update the description of the resource foo. It should not update the descriptions of the resources qux, quux, quuz (in their respective resource descriptions - external to foo) or create resources qux and quux if they happen to not exist or any of the example side effects mentioned above. If we want those things, they should be incorporated into the semantics of the request, case by case. Later on.

We shouldn't spend more time on dissecting the current requirement which is problematic and not really want we want. Kjetil, can you update?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a bit of rephrasing will help matters? I've linked to a relevant DBpedia resource, but that could be changed however the Editors deem appropriate.

Suggested change
<span about="" id="server-patch-not-other" rel="spec:requirement" resource="#server-patch-not-other"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:MUST-NOT">MUST NOT</span> allow a client to explicitly request multi-resource changes via the <code>PATCH</code> method, but the server is permitted to propagate side-effects of a change to the target resource to other resources.</span></span> [<a href="https://github.com/solid/specification/issues/125#issuecomment-873035679" rel="cito:citesAsSourceDocument">Source</a>]
<span about="" id="server-patch-not-other" rel="spec:requirement" resource="#server-patch-not-other"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:MUST-NOT">MUST NOT</span> allow a client request to explicitly change multiple resources via the <code>PATCH</code> method. However, a <code>PATCH</code> method request that explicitly changes one resource <span rel="spec:requirementLevel" resource="spec:MAY">MAY</span> cause the server to change one or more other resources as a side-effect of the explicitly requested change (akin to a <a href="http://dbpedia.org/resource/Database_trigger">`TRIGGER` firing in a SQL DBMS</a>).</span></span> [<a href="https://github.com/solid/specification/issues/125#issuecomment-873035679" rel="cito:citesAsSourceDocument">Source</a>]

<span about="" id="server-patch-not-other-response" rel="spec:requirement" resource="#server-patch-not-other-response"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:SHOULD">SHOULD</span> respond with a <code>422</code> status code [<cite><a class="bibref" href="#bib-rfc4918">RFC4918</a></cite>] and a message body that explains the error if it determines that such an instruction is attempted.</span></span>
</p>

<div class="note" id="conditional-update" inlist="" rel="schema:hasPart" resource="#conditional-update">
<h4 property="schema:name"><span>Note</span>: Conditional Update</h4>
<div datatype="rdf:HTML" property="schema:description">
Expand Down Expand Up @@ -1070,6 +1075,8 @@ <h3 property="schema:name">Normative References</h3>
<dd><a href="https://datatracker.ietf.org/doc/html/rfc3864" rel="cito:citesAsAuthority"><cite>Registration Procedures for Message Header Fields</cite></a>. G. Klyne; M. Nottingham; J. Mogul. IETF. September 2004. Best Current Practice. URL: <a href="https://datatracker.ietf.org/doc/html/rfc3864">https://datatracker.ietf.org/doc/html/rfc3864</a></dd>
<dt id="bib-rfc3986">[RFC3986]</dt>
<dd><a href="https://datatracker.ietf.org/doc/html/rfc3986" rel="cito:citesAsAuthority"><cite>Uniform Resource Identifier (URI): Generic Syntax</cite></a>. T. Berners-Lee; R. Fielding; L. Masinter. IETF. January 2005. Internet Standard. URL: <a href="https://datatracker.ietf.org/doc/html/rfc3986">https://datatracker.ietf.org/doc/html/rfc3986</a></dd>
<dt id="bib-rfc4918">[RFC4918]</dt>
<dd><a href="https://datatracker.ietf.org/doc/html/rfc4918" rel="cito:citesAsAuthority"><cite>HTTP Extensions for Web Distributed Authoring and Versioning (WebDAV)</cite></a>. L. Dusseault, Ed.. IETF. June 2007. Proposed Standard. URL: <a href="https://datatracker.ietf.org/doc/html/rfc4918">https://datatracker.ietf.org/doc/html/rfc4918</a></dd>
<dt id="bib-rfc5023">[RFC5023]</dt>
<dd><a href="https://datatracker.ietf.org/doc/html/rfc5023" rel="cito:citesAsAuthority"><cite>The Atom Publishing Protocol</cite></a>. J. Gregorio, Ed.; B. de hOra, Ed.. IETF. October 2007. Proposed Standard. URL: <a href="https://datatracker.ietf.org/doc/html/rfc5023">https://datatracker.ietf.org/doc/html/rfc5023</a></dd>
<dt id="bib-rfc5789">[RFC5789]</dt>
Expand Down