Fix Unparsed::encoded_len implementation

[wiktor-k]

Supersedes #64

[overhacked]

I like your approach here better, since it contains the irregular length / untagged array behavior to the "oddball" Unparsed struct. Since you're enshrining the length in Unparsed, shouldn't the implementation of Encode::encode for Unparsed be updated to match? Instead of keeping the direct write of self.details.0 in <Extension as Encode>::encode?

I'll propose this myself if there's any way for me to push commits to this PR.

[wiktor-k]

Since you're enshrining the length in Unparsed, shouldn't the implementation of Encode::encode for Unparsed be updated to match? Instead of keeping the direct write of self.details.0 in ::encode?

Excellent point! I agree.

I've sent you a collaborator invitation. Try accepting it and pushing here, I hope it will work :)

[overhacked]

I'm working on these test failures. I think the "good" case is actually not compliant with the RFC, so I'm rereading it and I'm going to check what OpenSSH emits.

[overhacked]

My initial guess about the failing tests was wrong. The "good" bytes in the tests are definitely spec-compliant and are the same structure that OpenSSH ssh-add emits.

The problem is fixed by c50efaf, though. It was that the new Unparsed behavior does not encode with a length prepended, and the impl Encode for KeyConstraint was accessing the inner Vec<u8> directly. I updated the implementation to call encode_prefixed() instead of directly accessing the inner Vec<u8>.

I'm going to add one more commit to this series that makes the inner Vec<u8> private, because there's no reason to access it directly when Unparsed::from<Vec<u8>> and <Unparsed as Encode>::encode_prefixed are available for roundtripping.

[overhacked]

Edited my last comment, because that would be an API-breaking change that might hold back this PR until 0.5.0? @wiktor-k, what do you think?

[wiktor-k]

Edited my last comment, because that would be an API-breaking change that might hold back this PR until 0.5.0? @wiktor-k, what do you think?

Hmm... I don't mind that change but if you want this fix in 0.4 series we need to create a branch from that tag and merge this fix there. main contains a couple of breaking changes already and there are more in the pipeline (check out other PRs) so if this fix is important for you now we need to do it in a different way.

It's not a big problem, just a bit of shuffling the commits around. Before I do that I'd like to be sure I do get you right though 😁

(If you want to you can create the branch yourself if I'm not mistaken)

[overhacked]

Haha. 😅 I didn't realize how much refactoring of the serialization there was between 0.4.0 and main. As I was investigating the backport, I remembered that my WIP project is already using the main branch anyway, because I'm depending on the client module. I'm blocked for release until the next major version, so I'll just wait until this PR lands in a Cargo release.

I'll go ahead and push the commit to make Unparsed's inner Vec<u8> private. Part of me wants to put Unparsed in its own inline module at proto::message::unparsed so that no future message impl Encode could directly tinker with the opaque bytes. I'll throw that into the commit, but let me know if it's too big a change.

[wiktor-k]

In general I prefer smaller PRs but I don't want to break your flow here 😉

The separate module idea is also good. Please do that!

I already got the feeling that we could split the existing files into several smaller modules so let's take the opportunity now.

Thanks for the ideas and the implementation! 🙇‍♂️

[overhacked]

Hm. I was thinking about how large a file proto/message.rs is, also. I'll start a new PR--stacked on this one--moving Unparsed into a separate module AND separate the other message::* structs into submodules, instead of attaching all that to this PR. That way, this one can stay tightly focused on fixing the Extension serialization bug. The Unparsed.0 change in visibility was really just preventative housekeeping after solving that bug; it makes sense to make it into its own changeset.

[wiktor-k]

Okay, is there anything here to merge this too? 🤔

[overhacked]

LGTM. If you want to go ahead and merge, I'll open another PR off main that refactors the proto::message tests' hex!() blocks into tests/messages/*.bin files.

[wiktor-k]

Thank you! 🙇‍♂️

[overhacked]

@wiktor-k Before I open a PR for moving the hex!() blocks into tests/messages:

The tests under proto::message are almost (but not quite) "roundtrip" tests, except they include an additional assert!(decoded, expected) that compares the deserialization result with a statically-constructed object. The roundtrip tests don't have any static objects.

The static object initialization doesn't actually test any code, though, because it just uses the default Struct {}-style construction; there are no new() or other methods involved. So, I don't think there would be any coverage lost by converting all the proto::message tests to roundtrip tests? If the impl Decode can successfully get all the seralized bytes into the struct's fields, and impl Encode serializes to the same bytes, then a statically-constructed object would always work also, I think.

Beyond roundtrip tests, the only unit tests needed (and not yet existing) for proto::message types would be for type-specific functions such as proto::Extension::new_message that serialize/deserialize but in some specific, wonky way.

[overhacked]

Also, if you'd like to chat about any of these changes, what message platforms do you use? I'm @overhacked.52 on Signal, or I can pop onto IRC or whatever.

[wiktor-k]

Yup. Most of these tests logic would be handled by the round-trip and that's what I like :)

The only thing that's not is that the deserializatuon produces good stuff in correct places. The test currently compare the entire structure which is okay but kind of complex (since, for example you need to have the key data). I think comparing just a couple of fields against static data (say assert_eq!("baloo@angela", deserialized.key.comment)).

That's what is not covered by round-trip and if one messes one field up these field by field comparisons will be easier to diagnose rather than "this vec of bytes is not equal to this vec of bytes".

As for comms I'll ping you on Signal but it's problematic for two reasons: first, we're in different timezones and Signal is more sync than comments. Second: there are other contributors who like to see the reasoning for changes and it creates a paper trail of what happened and why.

Phew 😅 thanks for digging into that. The new release will be really cool! 🤩