Wrong detection of magic byte during avro deserialization when the first field is empty

[tszappanos]

In the case of BINARY encoding, at the empty value of the first field, the serializer inserts 0 into the first Byte and then the deserializer evaluates it as a magic byte and ends with an error.
The first byte should always be used for the magic byte. Even if it is not a value of 0.
Serializer code (AbstractKafkaSerializer) is not compatible:
if (headersHandler != null && headers != null) { headersHandler.writeHeaders(headers, schema.toArtifactReference()); serializeData(headers, parsedSchema, data, out); } else { out.write(MAGIC_BYTE); getIdHandler().writeId(schema.toArtifactReference(), out); serializeData(parsedSchema, data, out); }
with the deserializer (AbstractKafkaDeserializer) code
if (data[0] == MAGIC_BYTE){ return deserialize(topic, data); } else if (headers == null){ throw new IllegalStateException("Headers cannot be null"); } else { ArtifactReference artifactReference = headersHandler.readHeaders(headers);

Schema example (optional field):
"fields": [{ "name": "alternativeName", "type": ["null", { "type": "string", "avro.java.string": "String", "pattern": "^. {0,100} $" }], "doc": "", "default": null }

[famarting]

so, if I understood correctly, because of using a schema that allows to have an empty value in the first field... the deserializers incorrectly understand that as the magic byte and try to do the deserialization as if the Artifact Global ID was after that first byte.

I agree this is an issue.

Btw, the reason why this is happening now is because now (after Apicurio Registry 2.0.0.Final) the serializers use the headers to write the Artifact Global ID by default. This way in your usecase, the Global ID is in the message headers, so there is no magic byte in the message, and because of your schema allowing to have empty values in the first field , that ends up with the deserializer getting confused and thinking your messages are not using the headers to pass the Artifact Global ID while they are actually using it.

I have to think of a fix for this, meanwhile I think disabling the headers in the serializer will solve your issue (https://github.com/Apicurio/apicurio-registry/blob/master/serdes/serde-common/src/main/java/io/apicurio/registry/serde/SerdeConfig.java#L151) @tszappanos

[tszappanos]

so, if I understood correctly, because of using a schema that allows to have an empty value in the first field... the deserializers incorrectly understand that as the magic byte and try to do the deserialization as if the Artifact Global ID was after that first byte.

I agree this is an issue.

Btw, the reason why this is happening now is because now (after Apicurio Registry 2.0.0.Final) the serializers use the headers to write the Artifact Global ID by default. This way in your usecase, the Global ID is in the message headers, so there is no magic byte in the message, and because of your schema allowing to have empty values in the first field , that ends up with the deserializer getting confused and thinking your messages are not using the headers to pass the Artifact Global ID while they are actually using it.

I have to think of a fix for this, meanwhile I think disabling the headers in the serializer will solve your issue (https://github.com/Apicurio/apicurio-registry/blob/master/serdes/serde-common/src/main/java/io/apicurio/registry/serde/SerdeConfig.java#L151) @tszappanos

thanks for the workaround, it looks like it works fine.

[EricWittmann]

@famartinrh Do you want to keep this issue open until we have a fix? Or is the workaround sufficient for this use-case?

[famarting]

the workaround could be sufficient, but maybe it's better to know from the users if they really wanted to use the headers to transmit the globalId, wdyt @tszappanos is the workaround sufficient?

[tszappanos]

@famartinrh , after a deeper analysis, we found that we need to correct the behavior of globalids in the headers. The workaround is not enough. When could this bugfix be ready, please? thanks.

[EricWittmann]

@tszappanos Fabian just submitted a PR to fix this and it has been merged into both master and 2.0.x. We'll be doing a 2.0.1.Final release this week (maybe today). The fix will be in that release.

[EricWittmann]

Fixed in #1573