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: #1542. Add Content-Digest. #1543

Merged
merged 7 commits into from
Sep 8, 2021
Merged

Fix: #1542. Add Content-Digest. #1543

merged 7 commits into from
Sep 8, 2021

Conversation

ioggstream
Copy link
Contributor

@ioggstream ioggstream commented Jun 14, 2021

This PR

  • adds Content-Digest
  • adds Want-Content-Digest
  • add examples
  • do we need a further rationale on the discussion?

@ioggstream ioggstream requested a review from LPardue June 14, 2021 07:17
@LPardue
Copy link
Contributor

LPardue commented Jun 14, 2021

I think this is a good start. I took a stab at updating the concept overview to better draw out the differences between Digest and Content-Digest - see https://github.com/httpwg/http-extensions/compare/ioggstream-1542...lucas/content-digest-plusplus?expand=1

Think we should wait for WG feedback before landing this PR into main branch though.

Co-authored-by: lucas <lucaspardue.24.7@gmail.com>
@ioggstream
Copy link
Contributor Author

@LPardue integrated your suggestion in the PR, thanks!

@LPardue
Copy link
Contributor

LPardue commented Jun 14, 2021

Thanks. (And thanks for putting together the bulk of this pr!)

I think the changes to overview set an example of the kinds of changes we probably need to make throughout the document. But for now what's here is indicative enough of the Content-Digest change to let people assess it does solve their issues.

@ioggstream
Copy link
Contributor Author

ioggstream commented Jun 15, 2021

After we get consensus on the doc's content, I'm even ok to do an editorial rewrite from scratch :DDD

@@ -318,6 +327,79 @@ Want-Digest: sha-256
Want-Digest: sha-512;q=0.3, sha-256;q=1, unixsum;q=0
~~~


# The Content-Digest Field {#content-digest}
Copy link
Member

Choose a reason for hiding this comment

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

So, strictly speaking, this field name is appropriate according to the current terminology in Semantics.

What triggered my unease was that the Content- prefix is used on several headers that don't refer to the actual content of the message (using the current Semantics definition) -- they refer to the representation. See httpwg/http-core#878.

I think it's safe to ship this as Content-Digest as long as it's being super-precise about what it's referring to.

I could also see flipping Content-Digest's semantics with Digest, to align with most (but not all) other uses of the Content- prefix. I'm not convinced that doing so is necessary, or even preferable; just mentioning it as something to consider.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good insight Mark, we'll provide an example of Content-Digest vs Digest in the same message.

About flipping, I think that Content-Digest, being similar to Content-md5 seems to me a more intuitive name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Flipping names seems bad to me. Prefer the way we suggested

Copy link
Contributor

@b---c b---c Jun 18, 2021

Choose a reason for hiding this comment

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

Wouldn't flipping the names be problematic with respect to compatibility with existing implementations of RFC3230's Digest?

Copy link
Contributor

@MikeBishop MikeBishop left a comment

Choose a reason for hiding this comment

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

As noted at the interim, this looks like a worthwhile direction. Minor editorial nits while reading through, but no disagreements with the proposal.

draft-ietf-httpbis-digest-headers.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-digest-headers.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-digest-headers.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-digest-headers.md Outdated Show resolved Hide resolved

# The Content-Digest Field {#content-digest}

The `Content-Digest` field contains a comma-separated list of one or more content digest
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we're defining a new header here, can we just use structured fields for it entirely? So it'd be a dictionary with the field names as algorithms and the values as binaries.

Copy link
Contributor

Choose a reason for hiding this comment

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

Excellent question. I like the idea but think that it would make it awkward because the algorithms define their encoding. So you'd have to carve out some exceptions and differences, which might catch implementers out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wanted to switch to SF, but after a long discussion it was suggested to retain the current grammar,
even because SF do not support uppercase dictionary keys: this is a show stopper wrt being backward compatible.

Moreover I don't agree wrt using different parsers for C-D and D: it is valuable that the same parser can process both fields.

ioggstream and others added 2 commits June 22, 2021 09:50
Co-authored-by: Mike Bishop <mbishop@evequefou.be>
Co-authored-by: Mike Bishop <mbishop@evequefou.be>
@ioggstream
Copy link
Contributor Author

@LPardue should we create a separate example section for Content-Digest or just add some examples in parallel with Digest?

* Fix: #1377. Reboot digest-algorithm values registry.

* Lucas' hint

* remove registration requirements
* Add examples for content-digest.

* contentMD5 -> want-content-digest

Co-authored-by: Lucas Pardue <lucaspardue.24.7@gmail.com>
draft-ietf-httpbis-digest-headers.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-digest-headers.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-digest-headers.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-digest-headers.md Outdated Show resolved Hide resolved
draft-ietf-httpbis-digest-headers.md Outdated Show resolved Hide resolved
It can be used in both requests and responses.

~~~
Want-Content-Digest = 1#want-digest-value
Copy link
Contributor

Choose a reason for hiding this comment

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

Given how similar Want-Digest and Want-Content-Digest are, and that they share the want-digest-value we can probably editorialize this a bit. But in order not to block this PR, we can do that work in a follow up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

filed #1632

Co-authored-by: Lucas Pardue <lucaspardue.24.7@gmail.com>
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.

6 participants