Fix inclusion to correctly disallow outside values #1135
Closed
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
houndci-bot
reviewed
Sep 12, 2018
| expect_not_to_match_on_values(builder, possible_values) | ||
| end | ||
|
|
||
| it 'fails when used in the positive with an appropriate failure message' do |
mcmire
force-pushed
the
fix-validate-inclusion-of
branch
from
60eb2d0
to
cd094f2
Compare
The inclusion matcher, when qualified with `in_array`, was using
AllowValueMatcher to check that values outside the array were disallowed
by the model (and then inverting its result). However, it should have
been using DisallowValueMatcher all this time. This commit fixes that.
Without this fix, the following error is raised when using the inclusion
matcher against a model which does not have the proper inclusion
validation on it:
undefined method `attribute_setter' for nil:NilClass
This happens because the inclusion matcher is a complex matcher, i.e.,
it runs a series of submatchers internally and the result of those
submatchers contributes to whether or not the matcher matches. If one of
those submatchers fails, the inclusion matcher immediately fails and
spits out the failure message associated with that submatcher.
However, there is a fundamental difference between AllowValueMatcher and
DisallowValueMatcher as it relates to how they function:
* AllowValueMatcher sets an attribute to a value on a record and expects
the record not to fail validation.
* DisallowValueMatcher sets an attribute to a value on a record, but
expects the record *to* fail validation.
The issue in this case is that, because AllowValueMatcher was used
instead of DisallowValueMatcher, the inclusion matcher thought that the
AllowValueMatcher failed, when in fact it passed (this result was just
inverted). So it tried to generate a failure message for a matcher that
didn't fail in the first place. By using DisallowValueMatcher, we set
the proper expectations.
mcmire
force-pushed
the
fix-validate-inclusion-of
branch
from
cd094f2
to
ad9b112
Compare
guialbuk
approved these changes
Sep 15, 2018
|
Fixed by #1136 |
|
Oop, okay, I was going to merge this after I rebased the other branch, as it contained some other non-related stuff. Just give me a thumbs-up next time and I can make sure to sort them out :) |
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.
The inclusion matcher, when qualified with
in_array, was usingAllowValueMatcher to check that values outside the array were disallowed
by the model (and then inverting its result). However, it should have
been using DisallowValueMatcher all this time. This commit fixes that.
Without this fix, the following error is raised when using the inclusion
matcher against a model which does not have the proper inclusion
validation on it:
This happens because the inclusion matcher is a complex matcher, i.e.,
it runs a series of submatchers internally and the result of those
submatchers contributes to whether or not the matcher matches. If one of
those submatchers fails, the inclusion matcher immediately fails and
spits out the failure message associated with that submatcher.
However, there is a fundamental difference between AllowValueMatcher and
DisallowValueMatcher as it relates to how they function:
the record not to fail validation.
expects the record to fail validation.
The issue in this case is that, because AllowValueMatcher was used
instead of DisallowValueMatcher, the inclusion matcher thought that the
AllowValueMatcher failed, when in fact it passed (this result was just
inverted). So it tried to generate a failure message for a matcher that
didn't fail in the first place. By using DisallowValueMatcher, we set
the proper expectations.
Related issue: #904