proto: make PartialDecode API public

[thynson]

Context: #1860.

This PR did a bit more than just expose symbols to public, a trait ConnectionIdValidator was introduced, due to the fact that a QUIC Load balancer have to support different kind of Connection ID Schemes, per QUIC-LB-DRAFT Secion 4.1.
Also some documents was added to suppress the clippy warning after those symbol being made public.

[thynson]

I'm not sure if ConnectionIdValidator should be move to quinn_proto::shared.

[Ralith]

What does this new trait add over the caller performing whatever validation they like themselves after decoding a header, as is already done with ConnectionIdGenerator::validate?

[djc]

I'm not convinced this ConnectionIdValidator is the right abstraction. Given that we're dealing with parsing, it seems like that should be front and center, maybe something like:

trait ConnectionIdParser {
     fn parse(&self, buf: &mut impl Buf) -> Result<ConnectionId, PacketDecodeError>;
}

(Feels like it could even be TryFrom impl?)

Also in terms of commit history, I'd like to see one commit that makes all the existing API that you need public, and then a separate commit that adds any new stuff you need (with as little as possible duplication).

[djc]

What does this new trait add over the caller performing whatever validation they like themselves after decoding a header, as is already done with ConnectionIdGenerator::validate?

It looks like parsing the header needs the expected CID len as input.

[thynson]

Thanks for review. I'm going to polish it based on your comments

[thynson]

The idea of ConnectionIdValidator comes from s2n-quic, but they have Validator and Generator and Format: Validator + Generator(see here is what s2n-quic does).

While in quinn, ConnectionIdGenerator has validate for now, so the duplication of functionality seems unavoidable unless we change trait ConnectionIdGenerator to requires ConnectionIdParser. Shall we go in this way?

[thynson]

While in quinn, ConnectionIdGenerator has validate for now, so the duplication of functionality seems unavoidable unless we change trait ConnectionIdGenerator to requires ConnectionIdParser. Shall we go in this way?

After further digging, I believe ConnectionIdGenerator.parse and ConnectionIdValidator.validate provides different semantics. The former is used to decide whether a state-less reset should be sent, so rename ConnectionIdValidator to ConnectionIdParser seems right to me.

[djc]

This is looking much better already.

[Ralith]

Mostly cosmetic issues, general direction looks good. Could you squash the constructor-unification commits down into the commits that introduce the new constructors?

[Ralith]

LGTM, thanks!

[djc]

I ended up squashing and reordering these changes. I changed the commits to (a) first make the APIs public and then (b) add the ConnectionIdParser trait and implementation. The previous approach had some intermediate states that did not make sense. @thynson can you check that this still satisfies your use case (there should be no changes in terms of the final public API, but still good to check)?

[thynson]

It's totally fine to me .