Use empty binary, not nil, for static table entries with no default value

[mtrudel]

Summary

HPAX.Table's @static_table stores nil as the value for the many static entries with no default (RFC 7541, Appendix A — e.g. :authority, host, user-agent). Decoding a plain Indexed Header Field Representation (RFC 7541 §6.1) against one of these — fully legal, and something real clients (and h2spec) do — returns that literal nil, silently violating this module's own documented contract that decoded values are always binaries (HPAX.header_value() :: binary()):

HPAX.decode(<<0xBA>>, HPAX.new(1000))
# {:ok, [{"user-agent", nil}], %HPAX.Table{}}   <- nil, not ""

Any caller relying on that documented contract (e.g. calling byte_size/1 on every decoded value) crashes on such a request. It also meant the encoder could never emit a compact single-byte indexed reference when asked to encode one of these headers with a literal "" value, since the {:full, index} match in lookup_by_header/3 is guarded with is_binary(value) and never fired for the nil-valued entries.

Fix

Store "" instead of nil for these static table entries. Decoded values are now always binaries as documented, and encoding a header whose value is "" for one of these names now correctly compresses to one byte.

Test plan

• Regression test: indexed lookup of a no-default-value static entry returns "" not nil
• Regression test: encoding such a header with a literal "" value now compresses to a single byte
• Updated existing tests that asserted the old nil-based behavior
• Full test suite passes, 0 failures

[mtrudel]

This also stands ready for review, split out from #26. I've had the robots go over all of hpax's dependent libraries per hex.pm to see if any of them may require updates to this (since it's technically a change in external behaviour) and all came up clean, other than Bandit (which as of recently now checks for decoded header size, and was bailing on the nils that were emitted here as a result).

[whatyouhide]

Isn't this technically changing behaviour, though? I understand it was spec'ed without nil but still?

[mtrudel]

That's exactly why I broke it out into a separate PR. On the encode side this is a clear win, since we were never encoding down to a single byte for many static entries. On the decode side it's less clear; on the one hand I kinda like the "" representation since that's closer to what you'd actually parse out of headers on the wire, but it is a change.

Up to you (obviously) which was you want to go here. I can easily fix Bandit to work well with either decode style

[whatyouhide]

I can't remember if HPACK can represent absence vs empty values; given the PR, I assume they're indistinguishable?

[mtrudel]

I can't remember if HPACK can represent absence vs empty values; given the PR, I assume they're indistinguishable?

HPAX has never (either with or without this PR) been able to encode headers with a nil value (ie: HPAX.encode([{:store, ":authority", nil}], context) raises an ArgumentError). But that doesn't mean that there's no observed differences, and it's those differences that are at the heart of this PR.

The first of the differences is fairly minor, having to do with encoding a header that's in the static table, in the particular case where its value is "".

On main, we get the following:

encoding_context = HPAX.new(1000)
{encoded, _context} = HPAX.encode([{:store, ":authority", ""}], encoding_context)
#=> encoded is [[], ["A", [<<0>>, ""]]], context has 1 entry: [{":authority", ""}]
HPAX.decode(IO.iodata_to_binary(encoded), encoding_context)
#=> {:ok, [{":authority", ""}], ...}

With this PR:

encoding_context = HPAX.new(1000)
{encoded, _context} = HPAX.encode([{:store, ":authority", ""}], encoding_context)
#=> encoded is <<129>>, context has no entries
HPAX.decode(IO.iodata_to_binary(encoded), encoding_context)
#=> {:ok, [{":authority", ""}], ...}

They both roundtrip to the same thing, but HPAX as it stands today will create a new row in the table to store this row per RFC7541§6.2.1 (Literal Header Field with Incremental Indexing). This PR optimizes this case to instead encode it per RFC751§6.1 (Indexed Header Field Representation), saving a byte on the wire and not consuming a context entry. It's a tiny optimization, and not really worth it on its own.

The second difference is the more impactful one. If a peer happens to send a static table entry with a "" value in the minimally encoded form (ie: per RFC751§6.1 (Indexed Header Field Representation), HPAX will decode it as follows:

On main:

encoding_context = HPAX.new(1000)
HPAX.decode(<<129>>, encoding_context)
#=> {:ok, [{":authority", nil}], ...}

With this PR:

encoding_context = HPAX.new(1000)
HPAX.decode(<<129>>, encoding_context)
{:ok, [{":authority", ""}], ...}

There's a bit of an asymmetry there - we currently don't accept nil values when encoding, but we may emit them when decoding 6.1 encoded messages. That is really the thing that this is fixing.

Again, it's a fairly minor corner in practice, though it IS a legal one. In particular, h2spec will send such headers in its http2/5.1 test which is what precipitated this whole workup on my side.

[whatyouhide]

Ok, I’m on board with this change. Thank you so much for (patiently) walking me through the change and for the explanation 🫶

[mtrudel]

Thanks for the merge! Could I bug you to push a new version with this change?