Union fix #8
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->rakeconversion).