Codegen for encoding unknown tagged fields is incorrect

[thedodd]

As can be seen in the code below, if version >= 1 then we encode at least one byte even if no tagged fields are present. I do believe that this is incorrect according to the spec. Unknown tagged fields should not take up any space on the wire at all if they are not being used.

impl Encodable for ResponseHeader {
    fn encode<B: ByteBufMut>(&self, buf: &mut B, version: i16) -> Result<(), EncodeError> {
        types::Int32.encode(buf, &self.correlation_id)?;
        if version >= 1 {
            let num_tagged_fields = self.unknown_tagged_fields.len();
            if num_tagged_fields > std::u32::MAX as usize {
                error!("Too many tagged fields to encode ({} fields)", num_tagged_fields);
                return Err(EncodeError);
            }
            types::UnsignedVarInt.encode(buf, num_tagged_fields as u32)?;

            write_unknown_tagged_fields(buf, 0.., &self.unknown_tagged_fields)?;
        }
        Ok(())
    }

That code should likely be updated to:

    fn encode<B: ByteBufMut>(&self, buf: &mut B, version: i16) -> Result<(), EncodeError> {
        types::Int32.encode(buf, &self.correlation_id)?;
        if version >= 1 {
            let num_tagged_fields = self.unknown_tagged_fields.len();
            if num_tagged_fields > std::u32::MAX as usize {
                error!("Too many tagged fields to encode ({} fields)", num_tagged_fields);
                return Err(EncodeError);
            }
            if !self.unknown_tagged_fields.is_empty() {
                types::UnsignedVarInt.encode(buf, num_tagged_fields as u32)?;
                write_unknown_tagged_fields(buf, 0.., &self.unknown_tagged_fields)?;
            }
        }
        Ok(())
    }

KIP-482:

Tagged fields are always optional. When they are not present in a message, they do not take up any space.

With the current implementation shown here, they will always take up at least 1 byte, even when no tagged fields are used.

[Diggsey]

You're misinterpreting the documentation. It's saying that fields which are not present don't take up any space themselves, not that the count is omitted. The field number is always required otherwise the decoder would not be able to determine whether there were any fields.

[thedodd]

Serialization

Tag Sections

In a flexible version, each structure ends with a tag section. This section stores all of the tagged fields in the structure.

The tag section begins with a number of tagged fields, serialized as a variable-length integer. If this number is 0, there are no tagged fields present. In that case, the tag section takes up only one byte.

If the number of tagged fields is greater than zero, the tagged fields follow. They are serialized in ascending order of their tag. Each tagged field begins with a tag header.

Yea, reading further into the serialization section of the spec, I was indeed misinterpreting the spec, or rather simply did not read that KIP fully.

Looks like this issue was due to the fact that I was not using ApiKey.request_header_version() when constructing the header response and instead was just directly passing the api version from the request header to the encoder. A bit of a footgun perhaps. Basically it was causing clients to fail to decode responses, and that coupled with the line of information in the KIP I referenced made me suspect the issue.