Re: [whatwg/fetch] Add support for compression dictionary transport (PR #1854)

@fred-wang commented on this pull request.



> +   <li><p>If <var>dictionaryValue</var> is null or <var>dictionaryValue</var>["<code>match</code>"]
+   does not <a for=map>exist</a>, then return <var>response</var>.
+
+   <li><p>If <var>dictionaryValue</var>["<code>type</code>"] <a for=map>exists</a> and its
+   <a>bare item</a> is not an <a>implementation-defined</a> supported dictionary type, then return
+   <var>response</var>.
+
+   <li><p>If <var>dictionaryValue</var>["<code>id</code>"] <a for=map>exists</a> and its
+   <a>bare item</a>'s <a for=string>length</a> is greater than 1024, then <a for=map>remove</a>
+   <var>dictionaryValue</var>["<code>id</code>"].
+
+   <li>
+    <p>If <var>dictionaryValue</var>["<code>match-dest</code>"] <a for=map>exists</a>:
+
+    <ol>
+     <li><p>Let <var>matchDestList</var> be <var>dictionaryValue</var>["<code>match-dest</code>"][0].

What does the `[0]`  mean here?

> +   "<code>dictionary</code>", and <var>response</var>'s <a for=response>header list</a>.
+
+   <li><p>If <var>dictionaryValue</var> is null or <var>dictionaryValue</var>["<code>match</code>"]
+   does not <a for=map>exist</a>, then return <var>response</var>.
+
+   <li><p>Let <var>compressionDictionaryCache</var> be the result of
+   <a>determining the compression-dictionary cache partition</a> given <var>request</var>.
+
+   <li><p>If <var>compressionDictionaryCache</var> is null, then return <var>response</var>.
+
+   <li><p>Let <var>pattern</var> be the result of
+   <a for=/>creating a URL pattern</a> given the bare item of <var>dictionaryValue</var>["<code>match</code>"],
+   the <a lt="URL serializer">serialization</a> of <var>request</var>'s <a for=request>current URL</a>,
+   and an empty map.
+
+   <li><p>If <var>pattern</var> is failure or <var>pattern</var> <a for=/>has regexp groups</a>,

Thank you so much for these tests! In general they look good but I have some suggestions/comments/questions:

> Dictionary with unknown match-dest is not used for fetch() API

So this is aligned with your PR because we return the response if matchDestList is empty (so similar to "Dictionary registration with empty match-dest list acts as wildcard'"). But can you please also add a similar test that does not hit the empty case:

```
compression_dictionary_promise_test(async (t) => {
  const match_dest = encodeURIComponent('("asdf", "")');
  ...
```

As I read your PR, "asdf" would be removed but the dictionary registration would still succeed, right? (similar to "Dictionary registration with matching fetch destination")

> Dictionary registration with 1024 character dictionary ID

For completeness, can we also have a test that checks the character range. That would also exercise parsing/serialization of the dquotes and backslash is properly done as per rfc965:

```
+compression_dictionary_promise_test(async (t) => {
+  // https://www.rfc-editor.org/info/rfc9651/#name-parsing-a-string
+  // double quotes and backslash are escaped in headers per RFC9651.
+  let dictionaryIDsuffix = " !#$%&'()*+,-./0123456789:;<=>?@ABCDEFGHIJKLMNOPQRSTUVWXYZ[]^_`abcdefghijklmnopqrstuvwxyz{|}~";
+  let dictionaryID = `\\"\\\\${dictionaryIDsuffix}`;
+  const dict = await (await fetch(`${kRegisterDictionaryPath}?id=${encodeURIComponent(dictionaryID)}`)).text();
+  // Wait until `available-dictionary` header is available.
+  assert_equals(
+      await waitUntilAvailableDictionaryHeader(t, {}),
+      kDefaultDictionaryHashBase64);
+  assert_equals(await checkHeader('dictionary-id', {}), `"${dictionaryID}"`);
+}, `Dictionary registration with dictionary ID (valid characters and backslash escaping)`);
```

> Dictionary with 1025 character dictionary ID is not registered

This test does not seem aligned with your PR? The ID would be ignored, so that should just behave like the "Simple dictionary registration and unregistration" case?

Also we can have a test for invalid character range (like '€') but in that case https://httpwg.org/specs/rfc9651.html#error-handling says the entire field should be ignored, so we would indeed really check that there is no available-dictionary header here (as opposed to a successful registration without ID).

Note that per https://httpwg.org/specs/1.html#error-handling the spec author can decide whether they want to ignore the entire field or define other handling (here remove the id from the dictionary).

> Overlapping match patterns prioritize the more specific dictionary

Would be nice to test a bit more the priority from https://www.rfc-editor.org/info/rfc9842/#section-2.2.3

At least I think
- A Use-As-Dictionary with "match-dest" has priority over a Use-As-Dictionary without "match-dest" (with a longer "match" length even).
- Two Use-As-Dictionaries with the same match's length and matchDest's emptiness but different fetch time (the most recent wins).

Would be nice to test for the "best match" algorithms that involve more than 2 dictionaries too.

> + then return a <a>network error</a>.
+
+ <li><p>Let <var>availableDictionaryItem</var> be the result of
+ <a for="header list">getting a structured field value</a> given <a><code>Available-Dictionary</code></a>,
+ "<code>item</code>", and <var>request</var>'s <a for=request>header list</a>.
+
+ <li><p>If <var>availableDictionaryItem</var> is null, then return a <a>network error</a>.
+
+ <li><p>Let <var>availableDictionaryHash</var> be the <a>bare item</a> of <var>availableDictionaryItem</var>.
+
+ <li><p>Let <var>newBody</var> be a new <a for=/>body</a> whose <a for=body>stream</a> is the
+ result of transforming <var>response</var>'s <a for=response>body</a>'s <a for=body>stream</a>
+ with an algorithm that verifies that the dictionary hash in the stream matches
+ <var>availableDictionaryHash</var> and decodes the rest of the stream with the applicable
+ algorithm as defined in [[!RFC9842]]. If verification or decoding fails,
+ error the transformed stream.

Thank you, I see! So this is referring to the remaining instances of "hash" (that I disregarded yesterday because I thought they were irrelevant):

https://www.rfc-editor.org/info/rfc9842/#name-dictionary-compressed-brotl
https://www.rfc-editor.org/info/rfc9842/#name-dictionary-compressed-zstan

> The header consists of a fixed 4-byte sequence and a 32-byte hash of the external dictionary that was used.

> A "Dictionary-Compressed Zstandard" stream is a binary stream that starts with a 40-byte fixed header

So what do you think of this minor clarifications:

> that first verifies that the dictionary hash in the stream's header matches availableDictionaryHash and decodes the rest of the stream with the applicable algorithm as defined in §4. Dictionary-Compressed Brotli and §5. Dictionary-Compressed Zstandard of [!RFC9842]].

So now I agree this is more an error at the stream level. Still I don't understand the implication of "error the transformed stream" (probably because I'm not familiar with the spec terminologies). What will be newBody after such an error?


-- 
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/fetch/pull/1854#pullrequestreview-4417247608
You are receiving this because you are subscribed to this thread.

Message ID: <whatwg/fetch/pull/1854/review/4417247608@github.com>