-
Notifications
You must be signed in to change notification settings - Fork 2
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
Union fix #8
Open
anujdas
wants to merge
6
commits into
Saltside:master
Choose a base branch
from
anujdas:wip/union_fix
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Union fix #8
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We already need `rake` to work with this gem, and it's capable of basically everything that `make` is; unify build tools to prevent confusion. Move the test thrift files into the test/ directory to avoid clutter, plus add a `clean` target for `rake` that wipes built definitions. Also, add minitest as a dependency, since it’s not built into newer Ruby versions.
Some things passed/returned to Thrift endpoints are not themselves Thrift objects, e.g., lists, primitives, or nil. These can and should still be validated. Since we're anyway not making use of type hints in the schema and relying on Thrift structs to self-validate, we can just recurse the struct tree and let what can be validated be validated, greatly cleaning up implementation while adding functionality. For example, union validation was broken for a very specific edge case: when a field that was a container around structs is not set, validation produces a highly confusing error message indicating that the unset field is, in fact, not set. Consider the Thrift definition union UnionExample { 1: list<SomeStruct> primary, 2: string alternate, } Setting the `alternate` field caused validation to fail, claiming that the `primary` field was not the set_field. This was due to the validator accessing the unset field, because the code meant to check this case only ran for specific field types. Now it performs correctly regardless of field type.
Union validation used to raise a StandardError, which doesn't make sense when the rest of the validation failures cause ProtocolExceptions. Make that uniform.
The library's error changes and new functionality seem like enough to bump the minor version. In particular, union validation now raises a different error (ProtocolException) than it used to (StandardError).
@ahawkins sorry for the commit churn. This PR should be final. |
Validator#validate is an instance method; reflect this in the docs.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Union validation was broken for a very specific edge case: when a field
that was a container around structs is not set, validation produces a
highly confusing error message indicating that the unset field was, in
fact, not set. As an example, consider the definition
Setting the alternate field caused validation to fail, claiming that
the primary field was not the set_field. This was due to the validator
accessing the unset field, because the code meant to check this case
only ran for specific field types. Now it performs correctly regardless
of field type.
Also adds a bunch of other changes and fixes (error unification, README
fixes,
make
->rake
conversion).