-
Notifications
You must be signed in to change notification settings - Fork 44
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
Add heuristic to determine resource type #160
Conversation
0337fad
to
a3f2196
Compare
a3f2196
to
c13dd42
Compare
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.
Clear for me.
Perhaps a suggestion for the first column format: "String ending in a slash (e.g., foo/
)" such that it is explicit what to generalize on.
main/resource-access.bs
Outdated
<tr> | ||
<td><code>foo/</code></td> | ||
<td>`Link` header other than container type.</td> | ||
<td>Create a resource with `foo` appended to the request-URI, check for |
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.
This exception I find very weird. I don't think we should derive anything from non-recognized Link
headers. I.e., all no
and other
cases should be identical for me.
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.
That's indeed what's intended. What's derived is that it is a non-Container resource. Slug will be ignored in this case. Would rephrasing help?
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.
My interpretation of what written seems to be correct (so writing is ok); but I don't understand why from an unknown header can be derived the resource is non-Container. I don't think we should derive that.
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.
What's recognised is "ldp:Container or a specialisation" (ldp:BasicContainer - as that's the only one currently required) as the "container type". I used "other than container type" to express their absence. Open to rephrasing.
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.
Ah okay, so only Link
header values with a type
link relation are included then? Or do we really mean all Link
header values?
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.
Only the existence of Link with rel=type LDPC or LDP-BC is checked.
Note the check in prior row says "Link header with ldp:Container or a specialisation." - We are really talking about rel=type here.
So we can just mention the actual relation.
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.
Yes, I would make it clear that it is a rel=type
header value. Then I have no further objections.
We could perhaps call them "indicated types" or so, and then declare above the table how those indicated types are extracted from Link
. No strong opinion.
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 used "or a specialisation" instead of ldp:BasicContainer figuring that if other container types are supported in the future, they will also follow the slash semantics. If server gets Link rel=type LDP-DC, it will create a container resource with trailing slash.
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.
All fine with me, my issue/misunderstanding was in the phrasing of "Link
header other than container type."
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.
How about e40d685 for this draft?
Good suggestions. Will this work for now: 3207529 ? |
Yeah. (Maybe "slashes" for grammar? But I don't mind.) |
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.
Good stuff!
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.
A few comments inline. Also wondering if it is worth mentioning whether/how different request methods may or may not affect the heuristic and any associated results?
main/resource-access.bs
Outdated
|
||
Servers MUST apply the following heuristics to determine a request's resource | ||
type | ||
[[Source](https://github.com/solid/specification/issues/128#issuecomment-573033297)]. |
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.
Consider explaining under what circumstances the server needs to fall back to this heuristic approach. (i.e. when the client does not specify an interaction model)?
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.
👍 from me. (Although I'd love to see container and resource names changed from foo/
to example/
).
[exit rabbit-holes] I've revisited this issue from the requirements. Some initial considerations:
Most relevant functional requirements (F1 is borrowed from LDP-UCR):
Non-functional requirement (along the lines of LDP's NF2.3):
In context of creating resources: PUT and PATCH can be used to meet F1 and F2.1 but not F2.2. POST can meet F1 and F2 with different conformance configurations:
Notes:
Status check:
My current preference is that we simplify the implementation as much as possible and I think the issues surrounding heuristics introduces a degree of complexity with of course some "benefits" that are not critical to meeting the functional requirements. Note #108 (comment) as an example where certain code paths are blocked but the requirements met by other conformance rules. |
Focusing on F2.2:
Note: F1.3 will not be prescribed for Proposal:
Note: C3.2.2 can be prescribed as MAY but it will be used towards F2.1 instead. If agreed, I can update the PR and follow up on the related issues. Open to go through alternatives if anyone would like. |
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.
👍
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.
👍
👍 from me to proceed with updating PR |
Updated PR c1aac42 based on #160 (comment) . Will continue to add related criteria in new PRs. Thanks all! |
Criteria for servers to apply heuristics to determine a request's resource type. Based on consensus at #128 (comment)