feat(MPM-384): Option to parse schema before exporting

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

feat(MPM-384): Option to parse schema before exporting #12

Closed bajdzun closed 2 years ago

bajdzun commented 2 years ago

There was still the issue with namespaces when embedded schema uses embedded schema, namespaces that are the same were not removed so I added additional Avro::parse() before exporting schema to a file.

I used the same option that was added before, maybe naming is not appropriate anymore, @nick-zh what do you think?

nick-zh commented 2 years ago

@bajdzun thx i will look into it as well, to see how we can best solve this. I was thinking about some improvements yesterday evening myself, but i need to do some more elaborate testing. I am on the road and busy today, so i will have to get back to you this evening, latest tomorrow morning :v:

Things i have in my mind:

validating the end Avro with Avro::parse, same as you did (not sure yet though what lib should do the namespace replacing)
from what i understand from your statement, Avro::parse will remove same namespaces, so i assume this is a spec and the lib currently does not adhere to it (meaning we should probably fix that properly as default behaviour and release as major)
check what happens with namespaces that are in a different "space"
need to check what other changes we have after Avro::parse for different usecases (maybe i missed something else)

nick-zh commented 2 years ago

So i quickly checked in the the spec and found this: https://avro.apache.org/docs/current/spec.html#names

References to previously defined names are as in the latter two cases above: if they contain a dot they are a fullname, if they do not contain a dot, the namespace is the namespace of the enclosing definition.

So even though they do not state that it must be like this, i think it is safe to assume that this should be the convention:

same namespace of embedded type when same, shall be omitted

nick-zh commented 2 years ago

My proposal would be:

this lib correctly uses namespaces according to spec (they were mostly added for the merging anyway to avoid naming conflicts)
this lib uses Avro:parse to validate that the result is the same (if that is possible and there are not other deviations that make this complicated), but uses its own result

What do you think @bajdzun , any additional thoughts @healerz? I am glad to propose a PR as well that you can verify, lmk :v:

bajdzun commented 2 years ago

So i quickly checked in the the spec and found this: https://avro.apache.org/docs/current/spec.html#names
References to previously defined names are as in the latter two cases above: if they contain a dot they are a fullname, if they do not contain a dot, the namespace is the namespace of the enclosing definition.
So even though they do not state that it must be like this, i think it is safe to assume that this should be the convention:

same namespace of embedded type when same, shall be omitted

That's right, I did some testing with flix-tech/avro-php library and they follow that convention, if namespaces are the same, they ignore them, if they are different, they don't remove them

nick-zh commented 2 years ago

Changes have been implemented as discussed in (#14). Thanks a lot again @bajdzun for the help and the collaboration :pray: