Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fail when extra fields are in the payload #15

Merged
merged 3 commits into from Jul 15, 2018

Conversation

dorner
Copy link
Contributor

@dorner dorner commented Jul 5, 2018

Currently, when passing a datum into the schema validator, the datum will pass even if the datum has more fields than the schema does. This can be dangerous because the client code is assuming that the schema has these values, but they are actually being silently swallowed when they are encoded.

This PR updates the validator so it will fail if any extra fields are passed into it.

@tjwp
Copy link
Collaborator

tjwp commented Jul 6, 2018

What is the behavior of schema validation in the other language implementations? Does it complain if additional data is present? I'm wary of making the behavior of the ruby implementation different to other languages.

It probably makes sense to open/discuss this on the official avro repo first and then add it here after that discussion.

@dorner
Copy link
Contributor Author

dorner commented Jul 6, 2018

I know that the node.js one has a toggle to allow or disallow extra fields: https://github.com/mtth/avsc/wiki/API#typeisvalidval-opts. Looks like the default is to allow them.
I can implement something like this as an option if it works. Might be a bit more work to also add that to avro_turf though. I'm concerned about how long it'll take for everything to percolate through. :(

@arron-green
Copy link

i verified with python's fastavro that adding an extra field that's not in the schema is also ignored

@tjwp
Copy link
Collaborator

tjwp commented Jul 9, 2018

I would prefer to have an option to disallow the extra fields, and allow them by default. For this gem, I don't think we want to change default behavior in ways that may not make it into the official gem. But I'm okay with additional, optional functionality

@arron-green
Copy link

+1 for optional functionality

@dorner
Copy link
Contributor Author

dorner commented Jul 9, 2018

All righty. I'll look into changing it this way.

@dorner
Copy link
Contributor Author

dorner commented Jul 10, 2018

@tjwp updated the code, can you take another look?

Copy link
Collaborator

@tjwp tjwp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's the only comment I have. What do you think @jkapell?

result.add_error(path,
"extra field provided: #{field_name} - not in schema!")
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be done more efficiently as something like the following:

(datum.keys.map(&:to_s) - expected_schema.fields.map(&:name)).each do |extra_field|
  result.add_error(path, "extra field '#{extra_field}' - not in schema")
end

@dorner
Copy link
Contributor Author

dorner commented Jul 13, 2018

@tjwp made the change as requested.

@dorner
Copy link
Contributor Author

dorner commented Jul 13, 2018

Nice! So for next steps, would you rather I make the PR against the avro gem before merging this in?

@tjwp
Copy link
Collaborator

tjwp commented Jul 13, 2018

I can run with getting this merged into this gem at this point. Feel free to open an official avro PR in parallel.

@tjwp tjwp merged commit f22205d into salsify:master Jul 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants