PookieBuns commented on PR #591: URL: https://github.com/apache/avro-rs/pull/591#issuecomment-5077330353
Hi thanks for the work you all put in this. Just wanted to drop my 2 cents here. I feel uneasy about the fix at best. Primarily because https://github.com/apache/avro-rs/pull/591/changes#diff-8854c9473e6bcbc7e33057928eaaf78aed3463f171b425658f5af852855e6afeR2280 this fails, and in a way thats silent in that it doesn't fail serialization but instead serializes incorrectly. IMO if we wanted to support serializing enums using a Union schema that contains said enum schema, this is a clear edge case that is I feel is common enough to happen in the wild from my experience. (i.e., this is the typical way to express nullable enums in avro). Furthermore, my stance as a user is its okay / annoying to fail serialization and I just need to fix my code to get it running, but its potentially catastrophic if it fails silently and sends "incorrect" data out If we wanted to support this, I feel like a less "faulty" solution would be to leverage the `name` field passed into the `serialize_unit_variant` like mentioned https://github.com/apache/avro-rs/pull/591#discussion_r3605830254 by @Kriskras99. I'd be much more okay with a stricter check that lets serialization fail if you don't follow the "secret handshake" over a "silent failure" that looks like I successfully serialized but actually serialized something unexpected, which is why my initial stance on this issue was > My preference would just to disallow serializing Non union rust representations using a union schema that contains that representation, but that's also a breaking change because I believe previously it was supported, although also sometimes unreliable). because it adds complexity to the "secret handshake" users need to understand if they want potentially more ambiguous use cases (serializing an enum with a union schema) rather than more explicit mappings (serialize an enum with a enum schema). But I'm going to leave the 2 of you up for the final say, because obviously the two of you have much more experience on this project and potentially understand more use cases users need than me, as I may have a narrow perspective thats biased towards my use cases. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
