Conditionally validate names of Record, Enums and Fixed. #415
Conversation
Absolutely no worries.
Ha! Yeah I thought of that, but I couldn't spoil another person's repo without their consent 😄 😛 I agree, these changes barely justify the cause, and a global solution could easily fit, if we are sure that there won't be another similar change in the future. Arguably though, if something else comes up, we can refactor the global into a DecoderConfig or something similar. I'll do an attempt so we can see if it looks ok-ish |
papanikge
force-pushed
the
conditional-validate-names
branch
from
58b6f09 to
baeea99
Compare
|
@nrwiersma I just pushed. Please let me know what you think. I also added a unit test with an invalid file |
ocf/ocf.go
Outdated
| type DecoderFunc func(*decoderConfig) | ||
|
|
||
| // WithSkipNameValidation sets whether to skip validating of field names when decoding. | ||
| func WithSkipNameValidation(skip bool) DecoderFunc { |
There was a problem hiding this comment.
I would urge caution here. It is unclear that this is setting a global value, so creating 2 decoders, one with this option and one without, it would be expected that they behave differently, however they will not. I would suggest leaving this out.
There was a problem hiding this comment.
Ah, good callout. I didn't think of that :(
How would the usage would look like though?
Do you prefer the callers setting the var from outside the package?
I find this a bit weird, although I might be just me.
There was a problem hiding this comment.
I see 2 options here:
- Expose the global var (it is currently exposed) and let people set it directly.
- Do not expose the global var and provide a function to set it, like
SkipNameValidation(true).
They both largely equate to the same thing, so either way is fine by me.
There was a problem hiding this comment.
Perhaps in the second option the name could be something like GlobalSkipNameValidation, but yeah, a global var setting would do the same semantically.
I'll remove it
There was a problem hiding this comment.
Done. Please take another look
|
Perhaps it would be good to document this in the README as well, to make it more discoverable. |
Makes sense. I added it in the |
|
Thank you. Can we get a new version tag too? 😇 |
|
Yes, will tag in a day or 2 to see there are no more changes coming. |
Apache Avro spec has some naming guidelines for Record, enums and fixed. This library correctly checks that and fails when a file has a badly named field.
However, the official Avro library does not 😢.
avro-tools tojson <badly-named-file.ocf>happily reads the file (same goes for AWS - I think that's because they use Java)There is a discussion about it here in the mailing list, and furthermore Python seems to have apache/avro#1995
Relates and closes issue #414