confluentinc / libserdes

Avro Serialization/Deserialization C/C++ library with Confluent schema-registry support
Apache License 2.0
5 stars 64 forks source link

fixes bug in a Schema::add overload #31

Closed drdo closed 4 years ago

drdo commented 4 years ago

Calling this particular overload would always result in a crash with an exception trying to construct an std::string from a NULL pointer.

ghost commented 4 years ago

It looks like @drdo hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence. Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

edenhill commented 4 years ago

The underlying serdes_schema_add() actually allows the schema to be added to the cache without a name, it is just the C++ schema_add() glue function that does not, so I think a better fix is to allow a NULL name there instead, like: .. serdes_schema_add(hnd->sd, name ? name.c_str() : NULL, ..)

drdo commented 4 years ago

The documentation comment for serdes_schema_add states "The schema name is required, but the id is optional."

Looking at the code it looks like it may just be the documentation that needs updating.

edenhill commented 4 years ago

Right, I believe the docs don't reflect the code, and there might have been an intention for a name-less add to load the schema from schema-registry using the id. But we have schema_get() for that purpose. Additionaly, the definition params are completely ignored int his variant of Schema::add, so it seems like a clumsy mistake.

However, as to not break the API I suggest that instead of removing this method we make schema_add() support NULL name, and adding a comment in Serdes.cpp that this method ignores the definition arguments.

drdo commented 4 years ago

That was actually the first thing I did (supporting NULL name in schema_add). But then I hit an error message "Schema name required".

Nevertheless, I will look into it in a bit to see if I can adapt the C code to support this.

drdo commented 4 years ago

I've updated the code to support a NULL name as you suggested.

edenhill commented 4 years ago

[clabot:check]

ghost commented 4 years ago

@confluentinc It looks like @drdo just signed our Contributor License Agreement. :+1:

Always at your service,

clabot

edenhill commented 4 years ago

Thank you!