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

Fix: #936. Cache semantics impacts on Digest and on the representation. #937

Merged
merged 3 commits into from Aug 26, 2020

Conversation

ioggstream
Copy link
Contributor

@ioggstream ioggstream commented Sep 24, 2019

This PR

Clarifies the following points:

  • resource identification is inherited from SEMANTICS
  • validator fields (ETags & Last Modified) which apply to the identified resource, apply to Digest too.

@ioggstream
Copy link
Contributor Author

I'll merge the content-location stuff and leave the resource identification to this issue (which is the hard part). This will push forward the ongoing work on POST/PATCH.

@ioggstream ioggstream changed the title Fix: #936. Further HTTP semantics impacts on the representation. Fix: #936. Cache semantics impacts on Digest and on the representation. Oct 10, 2019
@LPardue LPardue linked an issue Mar 10, 2020 that may be closed by this pull request
@ioggstream
Copy link
Contributor Author

@reschke @LPardue PTAL :)

@reschke
Copy link
Contributor

reschke commented Aug 17, 2020

Consider me confused.

Currently, the draft says:

"The resource is specified by the effective request URI and any validator field contained in the message."

...and then, in that section, doesn't mention "resource" anymore.

What was the intent of that statement? Can't it be just removed?

@ioggstream
Copy link
Contributor Author

Currently, the draft says:

"The resource is specified by the effective request URI and any validator field contained in the message."

...and then, in that section, doesn't mention "resource" anymore.

What was the intent of that statement?

The statement originates from https://tools.ietf.org/html/rfc3230#section-4.3.2 .
My understanding is that the original author stated explicitly that Digest must be aligned with Last-Modified & co.
This could be trivial though...

Can't it be just removed?

It is reasonable to me to remove the whole statement

 The resource is specified by the effective request URI and any validator field contained in the message

and delegate the resource specification to https://httpwg.org/http-core/draft-ietf-httpbis-semantics-latest.html#target.resource

@LPardue @mnot WDYT?

@ioggstream
Copy link
Contributor Author

My bad, Mark replied #949 (comment) with the above proposal. Let's do it then!

@ioggstream
Copy link
Contributor Author

Current wording integrates @reschke and @mnot suggestion (iiuc).

@@ -284,6 +282,9 @@ A sender MAY send a representation-data-digest using a digest-algorithm without
knowing whether the recipient supports the digest-algorithm, or even knowing
that the recipient will ignore it.

Digest MAY be cached and the freshness information of the resource, eventually conveyed by validator
Copy link
Member

Choose a reason for hiding this comment

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

Generally, we try to avoid re-stating requirements in other specs, or constraining how they're interpreted, because that makes the composition of the specs more brittle -- there's more chance of misinterpretation or unintentional misalignment (either as written or after some future change in the dependency).

So if you say anything at all, it shouldn't be a RFC2119 requirement; just some prose about the relationship for reader convenience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @mnot. Let's drop those lines then!

@ioggstream ioggstream requested a review from mnot August 18, 2020 08:02
Comment on lines 238 to 241
A representation digest consists of
the value of a checksum computed on the entire selected `representation data` of a resource
the value of a checksum computed on the entire selected `representation data`
(see Section 7 of {{SEMANTICS}}) of a resource identified according to Section 7.3.2 of {{SEMANTICS}}
together with an indication of the algorithm used (and any parameters)
Copy link
Contributor

Choose a reason for hiding this comment

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

This new text look technically fine but the sentence is a bit long winded now, especially with the
parameter comment in brackets. How about some editorializing like this, which intentially avoids saying anything about parameters.

A representation digest consists of a `digest-algorithm` followed by an `encoded digest output`.

~~~ abnf
   representation-data-digest = digest-algorithm "="
                                <encoded digest output>
~~~

The `digest-algorithm` identifies both an algorithm and output encoding format which are applied
to a representation. A checksum value is first computed on the entire selected `representation data`
(see Section 7 of {{SEMANTICS}}) of a resource identified according to Section 7.3.2 of {{SEMANTICS}}.
This is then encoded using the required format.

The list of defined `digest-algorithms` is provided in {{algorithms}}.

The example below shows the `sha-256` digest-algorithm which uses base64 encoding.

~~~ example
   sha-256=X48E9qOokqqrvdts8nOJRJN3OWDUoyWxBf7kbu9DBPE=
~~~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this is technically ok, I suggest to:

In the rewording, we could even avoid mentioning the "encoding" part as I think it should only be mentioned in the digest-algorithm section.

WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

sgtm

@LPardue
Copy link
Contributor

LPardue commented Aug 18, 2020

Yeah I forgot about the clash with 850. Let's land this and I see you've made a signpost back to my editorial suggestion so we don't lose it.

@ioggstream
Copy link
Contributor Author

Ok, to me this is complete. I'll leave to @LPardue the decision to merge or to wait for further feedback.

@ioggstream ioggstream merged commit 54472b0 into master Aug 26, 2020
@ioggstream ioggstream deleted the ioggstream-936 branch August 26, 2020 13:22
richanna pushed a commit to richanna/http-extensions that referenced this pull request Oct 20, 2020
…ntation. (httpwg#937)

* Fix: httpwg#936. Relationship with validators.
* Don't re-state other specs' requirements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Explain reference to cache-validators
4 participants