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

Tighten consistency requirements for resource types #341

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

Conversation

kjetilk
Copy link
Member

@kjetilk kjetilk commented Oct 29, 2021

With the sound of the deadline of the milestone approaching with rising pitch, I decided to draft what I think is coming out of #301. There doesn't seem to be significant opposition to most of this given the test results, with the possible exception of the status code. I put 409 in anyway. We can conclude that debate in #301.

The PR should address both my initial concern, and @langsamu's first concern of changing resource types. The text isn't very extensive, and does not cover it in as much detail as we did on the issue, but I think we can enforce this by encoding the status codes in tests.

protocol.html Outdated Show resolved Hide resolved
Copy link
Contributor

@RubenVerborgh RubenVerborgh left a comment

Choose a reason for hiding this comment

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

LGTM; I'm still in favor of 400, but let's settle that in #301 indeed.

protocol.html Outdated Show resolved Hide resolved
protocol.html Outdated Show resolved Hide resolved
protocol.html Outdated Show resolved Hide resolved
Co-authored-by: Aaron Coburn <[email protected]>
Copy link
Member Author

@kjetilk kjetilk left a comment

Choose a reason for hiding this comment

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

Responses to @RubenVerborgh . The github workflow isn't very handy at these short things :-)

protocol.html Outdated Show resolved Hide resolved
protocol.html Outdated Show resolved Hide resolved
@kjetilk kjetilk requested a review from a team November 1, 2021 16:48
Copy link
Member

@dmitrizagidulin dmitrizagidulin left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@csarven csarven left a comment

Choose a reason for hiding this comment

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

I like the idea of introducing information on consistency. However, I do not think that we have (significant) normative requirements in SP to specify this just yet. We should get those up to speed (in either direction) to know what consistency will be, especially with the role of type link relation. Details: #301 (comment)

@@ -665,6 +665,22 @@ <h3 property="schema:name">Resource Type Heuristics</h3>

<p><span about="" id="server-post-uri-assignment" rel="spec:requirement" resource="#server-post-uri-assignment"><span property="spec:statement">When a successful <code>POST</code> request creates a resource, the <span rel="spec:requirementSubject" resource="spec:Server">server</span> <span rel="spec:requirementLevel" resource="spec:MUST">MUST</span> assign a URI to that resource.</span></span> <span about="" id="server-slug-uri-assignment" rel="spec:requirement" resource="#server-slug-uri-assignment"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:MAY">MAY</span> allow clients to suggest the URI of a resource created through <code>POST</code>, using the HTTP <code>Slug</code> header as defined in [<cite><a class="bibref" href="#bib-rfc5023">RFC5023</a></cite>].</span></span></p>

<p>
<span about="" id="server-ensure-consistency" rel="spec:requirement" resource="#server-ensure-consistency"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:MUST">MUST</span> only accept requests where there is consistency between the path ending (<a href="#uri-slash-semantics">URI Slash Semantics</a>), any HTTP <code>Link</code> headers with <code>rel="type"</code> and the media type given in the <code>Content-Type</code> header of the request.</span></span>
Copy link
Member

Choose a reason for hiding this comment

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

How will this be tested without knowledge of specific required types? It appears to introduce unnecessary variance.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did not understand how it introduces variance, to the contrary, it is a constraint...

It is clear that it cannot be tested in the general case, there's an open world out there, but we do have knowledge of some specific resource types, the LDP ones, and we can test them.

Copy link
Member

Choose a reason for hiding this comment

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

It is introducing a (vague) requirement that depends on variable resource typing. Not only there is no useful testing for the general case but the specific testing is very limited. As mentioned, there is currently only one place where LDP's specific type is used - and that's being blown out of proportion as it seems.

Copy link
Member Author

Choose a reason for hiding this comment

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

I assume that you have inspected the existing tests?

Copy link
Member

@csarven csarven Nov 16, 2021

Choose a reason for hiding this comment

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

Can you refer to the tests corresponding to the requirements in the SP?

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 at present, but if this was merged, I could.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<span about="" id="server-ensure-consistency" rel="spec:requirement" resource="#server-ensure-consistency"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:MUST">MUST</span> only accept requests where there is consistency between the path ending (<a href="#uri-slash-semantics">URI Slash Semantics</a>), any HTTP <code>Link</code> headers with <code>rel="type"</code> and the media type given in the <code>Content-Type</code> header of the request.</span></span>
<span about="" id="server-ensure-consistency" rel="spec:requirement" resource="#server-ensure-consistency"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:MUST">MUST</span> only accept requests where there is consistency between the path ending (<a href="#uri-slash-semantics">URI Slash Semantics</a>), any HTTP <code>Link</code> headers with <code>rel="type"</code>, and the media type given in the <code>Content-Type</code> header of the request.</span></span>

@@ -665,6 +665,22 @@ <h3 property="schema:name">Resource Type Heuristics</h3>

<p><span about="" id="server-post-uri-assignment" rel="spec:requirement" resource="#server-post-uri-assignment"><span property="spec:statement">When a successful <code>POST</code> request creates a resource, the <span rel="spec:requirementSubject" resource="spec:Server">server</span> <span rel="spec:requirementLevel" resource="spec:MUST">MUST</span> assign a URI to that resource.</span></span> <span about="" id="server-slug-uri-assignment" rel="spec:requirement" resource="#server-slug-uri-assignment"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:MAY">MAY</span> allow clients to suggest the URI of a resource created through <code>POST</code>, using the HTTP <code>Slug</code> header as defined in [<cite><a class="bibref" href="#bib-rfc5023">RFC5023</a></cite>].</span></span></p>

<p>
<span about="" id="server-ensure-consistency" rel="spec:requirement" resource="#server-ensure-consistency"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:MUST">MUST</span> only accept requests where there is consistency between the path ending (<a href="#uri-slash-semantics">URI Slash Semantics</a>), any HTTP <code>Link</code> headers with <code>rel="type"</code> and the media type given in the <code>Content-Type</code> header of the request.</span></span>
<span about="" id="server-reject-inconsistency" rel="spec:requirement" resource="#server-reject-inconsistency"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:MUST">MUST</span> reject inconsistent requests using a <code>415</code> (for media type inconsistency) or <code>409</code> (for all other inconsistencies) status code as per Section 4.3.4 of [<cite><a class="bibref" href="#bib-rfc7231">RFC7231</a></cite>].</span></span>
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this repeating RFC 7231? It is already covered in #http-server.

Copy link
Member Author

Choose a reason for hiding this comment

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

To some extent, it is, but there is considerable room for interpretation since HTTP isn't entirely consistent itself at this point, as the discussion between @RubenVerborgh and myself shows. Therefore, it needs to be explicit.

Copy link
Member

Choose a reason for hiding this comment

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

<p>
<span about="" id="server-ensure-consistency" rel="spec:requirement" resource="#server-ensure-consistency"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:MUST">MUST</span> only accept requests where there is consistency between the path ending (<a href="#uri-slash-semantics">URI Slash Semantics</a>), any HTTP <code>Link</code> headers with <code>rel="type"</code> and the media type given in the <code>Content-Type</code> header of the request.</span></span>
<span about="" id="server-reject-inconsistency" rel="spec:requirement" resource="#server-reject-inconsistency"><span property="spec:statement"><span rel="spec:requirementSubject" resource="spec:Server">Servers</span> <span rel="spec:requirementLevel" resource="spec:MUST">MUST</span> reject inconsistent requests using a <code>415</code> (for media type inconsistency) or <code>409</code> (for all other inconsistencies) status code as per Section 4.3.4 of [<cite><a class="bibref" href="#bib-rfc7231">RFC7231</a></cite>].</span></span>
<span about="" id="server-existing-type-inconsistency" rel="spec:requirement" resource="#server-existing-type-inconsistency"><span property="spec:statement">When a <span rel="spec:requirementSubject" resource="spec:Server">server</span> receives a request that attempts to change the resource type, it <span rel="spec:requirementLevel" resource="spec:MUST">MUST</span> verify that the resource type of the request is compatible with the current resource type and reject the requests using a <code>409</code> status code if it is not.</span></span>
Copy link
Member

Choose a reason for hiding this comment

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

For this to be effective, the resource types need to be a normative requirement.

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree, will discuss in #301.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: To Do
Development

Successfully merging this pull request may close these issues.

Slash semantics and conflicting requests on resource creation
6 participants