search

php-kafka / php-avro-schema-generator

PHP avro subschema merger and experimental PHP Class avro schema generator
BSD 3-Clause "New" or "Revised" License
5 stars 5 forks source link

Exclude namespaces for embedded schemas if they are the same as root schema namespace #5

Closed bajdzun closed 2 years ago

bajdzun commented 2 years ago

According to flix-tech/avro-php library when schema (after merging with avro:subschema:merge) is parsed it will not contain namespaces for embedded schemas if they are the same as root schema namespace.

I suggest this (or similar) change to follow the same behaviour.

nick-zh commented 2 years ago

@bajdzun @healerz thx, sry for the early, I will have a look tomorrow ✌️

nick-zh commented 2 years ago

@bajdzun can you check what happens when we have the same embedded record twice in the same root schema? If I remember correctly, that was a problem. Not sure if I covered that in a test properly (can't check right now, sry) 😅

EDIT: all good i checked it, current logic covers it properly

nick-zh commented 2 years ago

Hey guys

So many thanks for pointing this out. During testing, i noticed that non name spaced schema were not supported, so i added support for this as well (release 1.2.0). Now i understand that one might still want namespaces in sub schemas to avoid name conflicts in a big project, so i think this PR will solve this. To avoid breaking the current behaviour i would suggest, that we add it as an option to the command: --optimize-sub-schema-namespaces or something similar. In the next major we could change this behaviour and "invert" it so we need an option if we explicitly want it, wdyt?

PS: If you sync the branch, CI should be running now hopefully :v:

healerz commented 2 years ago

@nick-zh your suggestion sounds good to me, actually a good point regarding a potentially problematic BC. We noticed that the implementation we use strips the namespace from embedded entities before sending it to the schema registry. (@bajdzun correct me if my statement is not correct 🙂 ). But ofc, this depends on the implementation of the schema registry client, so we can't know for sure if this could have an impact if other implementations are used.

nick-zh commented 2 years ago

Hey @bajdzun lgtm thx. Failure is due to the CodeClimate secret not being passed to your fork :smile: