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

Put the If-None-Match: * requirement on the server #292

Merged
merged 2 commits into from
Aug 4, 2021

Conversation

kjetilk
Copy link
Member

@kjetilk kjetilk commented Aug 2, 2021

Tests have shown inconsistent treatment of the If-None-Match: * header. I believe there is consensus that Solid Servers must support this feature based on the issues discussions in #40 and #108. I also think we cannot put such a requirement on the client, also we can't test that. Thus, I have changed the wording to make it a requirement on the servers.

@kjetilk kjetilk requested a review from csarven August 2, 2021 21:31
@kjetilk kjetilk self-assigned this Aug 2, 2021
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.

Tests have shown inconsistent treatment of the If-None-Match: * header.

Vague. [Citation needed]

I believe there is consensus that Solid Servers must support this feature based on the issues discussions in #40 and #108.

Already in the Protocol:

A data pod MUST implement the server part of HTTP/1.1 Conditional Requests [RFC7232] to ensure that updates requested by clients will only be applied if given preconditions are met.

Moreover, for clients the Protocol includes:

A Solid client MAY implement the client parts of HTTP/1.1 Conditional Requests [RFC7232] to only trigger updates when certain preconditions are met.

Hence, neither the original or the proposed text is necessary.


As for:

I also think we cannot put such a requirement on the client

The requirement is not pushed to the client. Client behaviour is compatible with servers irrespective of the header in the request.

, also we can't test that.

Why? (Putting aside the fact that it is not the client that actually prevents undesired modification.)

Thus, I have changed the wording to make it a requirement on the servers.

Redundant.


I don't agree with the proposed change.

As mentioned, since server support is a MUST and client support is a MAY (as per #http section), no text is necessary. The purpose of the existing text in the Protocol was to (strongly?) encourage clients to use the header. So, I propose to change it to a Note - it is not a whole lot different than a MAY here but at least it is not redundant and still gets to put some emphasis on its use.

Co-authored-by: Sarven Capadisli <[email protected]>
@kjetilk
Copy link
Member Author

kjetilk commented Aug 3, 2021

Right. I hadn't noticed that there was a general must on the conditional requests. Then, I agree that a note is appropriate.

@csarven csarven merged commit 30bdd72 into main Aug 4, 2021
@csarven csarven deleted the proposal/inm-asteriks branch November 30, 2021 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants