Update NotEmpty validator to use bitmasking #5874

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
4 participants
@dstockto

This updates the NotEmpty validator to use bitmasks instead of adding and subtracting the values. While it may not seem like a big deal, there are bugs that can crop up if a user creates code that adds in the same value more than once. For instance, without this patch, setting the type to array('boolean', 'boolean') means that the validator will behave as though you'd set it to array('integer').

Additionally, there's no reason to subtract the value of the bit mask from the $type value since we can use bitwise math to determine which flags are set. Using |= instead of adding means that even if we set the same flag lots of times, the value still will be the same. I've added a new unit test which will fail with the current NotEmpty implementation but will work with the updated implementation.

The documentation needs to be updated as well since the NotEmpty docs talk about adding the values together which is not the correct way to use these flags.

The values for the flags have also been updated to hex as it makes it a little more clear what's actually happening. Each validation value is a number represented by a single bit (with the exception of the combined values like PHP and ALL). I did not updated the 493, but it should probably be commented or otherwise indicated the values that are being checked by default (boolean, float, string, empty array, null and space).

The only other test changed was to add 3 more checks to ensure the default setting treat 0.0 as invalid, 1.0 as valid and an empty stdClass object as valid.

David Stockton
Update NotEmpty validator to use bitmasking instead of addition and s…
…ubtraction to fix an issue when string types are duplicated
+ )
+ );
+
+ $this->assertFalse($filter->isValid(false));

This comment has been minimized.

Show comment Hide comment
@Maks3w

Maks3w Feb 26, 2014

Member

Please refactor this as a data provider

@Maks3w

Maks3w Feb 26, 2014

Member

Please refactor this as a data provider

This comment has been minimized.

Show comment Hide comment
@Maks3w

Maks3w Feb 26, 2014

Member

Forgive this. Whole tests will need to be refactored and remove the duplication

@Maks3w

Maks3w Feb 26, 2014

Member

Forgive this. Whole tests will need to be refactored and remove the duplication

This comment has been minimized.

Show comment Hide comment
@dstockto

dstockto Feb 26, 2014

I'd be happy to redo the entire test suite as data providers. I was following existing code as an example.

@dstockto

dstockto Feb 26, 2014

I'd be happy to redo the entire test suite as data providers. I was following existing code as an example.

This comment has been minimized.

Show comment Hide comment
@Maks3w

Maks3w Feb 26, 2014

Member

Yep, but not in this PR. Too noise.

@Maks3w

Maks3w Feb 26, 2014

Member

Yep, but not in this PR. Too noise.

This comment has been minimized.

Show comment Hide comment
@dstockto

dstockto Feb 26, 2014

I'll have it updated shortly. I'll do the test -> data provider refactor in a different PR

@dstockto

dstockto Feb 26, 2014

I'll have it updated shortly. I'll do the test -> data provider refactor in a different PR

@Maks3w

This comment has been minimized.

Show comment Hide comment
@Maks3w

Maks3w Feb 26, 2014

Member

Please check line 363 of test. May should be changed to bits

Member

Maks3w commented Feb 26, 2014

Please check line 363 of test. May should be changed to bits

@dstockto

This comment has been minimized.

Show comment Hide comment
@dstockto

dstockto Feb 26, 2014

I've posted an update to move the new test to a data validator and the test you indicated (line 363) moved to using bitwise math.

I've posted an update to move the new test to a data validator and the test you indicated (line 363) moved to using bitwise math.

@Maks3w

This comment has been minimized.

Show comment Hide comment
@Maks3w

Maks3w Feb 26, 2014

This don't test isValid method.

This don't test isValid method.

This comment has been minimized.

Show comment Hide comment
@dstockto

dstockto Feb 26, 2014

Owner

All the other tests do test that. The issue was that the type was being set incorrectly if a string type was set more than once.

Owner

dstockto replied Feb 26, 2014

All the other tests do test that. The issue was that the type was being set incorrectly if a string type was set more than once.

@dstockto

This comment has been minimized.

Show comment Hide comment
@dstockto

dstockto Feb 26, 2014

@Maks3w Do you want it updated to test isValid for all the various types if the validator is set with a double string of something? I can do that but I think it's overkill. We know the validators work based on setting the values appropriately because each of them is tested. This is testing that setting the type using duplicated string values ends up with the right type. As long as that's the case, we're in a known state (the type is right) and that's tested.

@Maks3w Do you want it updated to test isValid for all the various types if the validator is set with a double string of something? I can do that but I think it's overkill. We know the validators work based on setting the values appropriately because each of them is tested. This is testing that setting the type using duplicated string values ends up with the right type. As long as that's the case, we're in a known state (the type is right) and that's tested.

@dstockto

This comment has been minimized.

Show comment Hide comment
@dstockto

dstockto Feb 27, 2014

@Maks3w If you disagree and want additional tests for isValid when the validator is configured with a duplicated string type, I can do that, but I'd like to know how you envision those working or looking.

@Maks3w If you disagree and want additional tests for isValid when the validator is configured with a duplicated string type, I can do that, but I'd like to know how you envision those working or looking.

@weierophinney

This comment has been minimized.

Show comment Hide comment
@weierophinney

weierophinney Mar 3, 2014

Member

@dstockto Easiest way to do those tests is to use a data provider; that way you write the test case once, passing it many sets of data. Ping me on IRC if you want assistance.

Member

weierophinney commented Mar 3, 2014

@dstockto Easiest way to do those tests is to use a data provider; that way you write the test case once, passing it many sets of data. Ping me on IRC if you want assistance.

@weierophinney weierophinney added this to the 2.2.6 milestone Mar 3, 2014

@dstockto

This comment has been minimized.

Show comment Hide comment
@dstockto

dstockto Mar 4, 2014

@weierophinney I'm just trying to get an understanding of how much we want to do with that.

I've got PR #5879 posted which converts all the existing multi-assert tests to data providers.

(Pasted from the bottom of this, if you want to know my reason on it, it's below this section.)
TL;DR - I think the added tests are for and should be for checking that the type is set correctly. The other tests already validate isValid for examples of different data for each type. The bitmask problem is due to incorrect bitmask implementation so the tests test the setting of the type and leave the testing of the isValid method to the tests that are already doing that.

I guess the question on this one remains:

We've got tests for isValid for ~12 data types for each NotEmpty type. The issue I was originally fixing is adding the same type via a string more than once which results in the wrong internal type in the validator. The tests shows that the behavior is corrected. It seems to make sense that if the tests that assure validation works properly if the type is set properly work then we rely on those for isValid. The other test should determine if setting type works properly which is a different piece of functionality.

I guess where I'm getting stuck on the data provider tests that you and @Maks3w have asked for is that it seems to be "thorough" I'd need some way to do a dataProvider provider test -- in other words testing the duplication of the string setting for each type and then for each type testing that it's valid in the same way the existing tests check.

For instance, if an individual data set was

array('boolean', array( ... array of example types and expected validity value ...))

Then the way that test would run would be to loop over each of the values in the inner array to make sure that it's valid in the way we expect. Another option is essentially duplicate every data provider test from #5879 but have the test try to set up the validator with a duplicate string type, and then use the same data providers from those tests.

One final way I've thought of would be to have a single test, but with a massive 200+ line data provider that looks something like this:

public function bigOlDataProvider() 
{ 
    return array(
        array('boolean', true, true),
        array('boolean', false, false),
        array('boolean', 1, true),
        array('boolean', 0, true),
..<snip 8 more types for boolean>..

        array('integer', 0, false),
        array('integer', 1, true),
        array('integer', false, true),
       array('integer', true, true),
.. <snip 8 more types for integer> ..

<snip 12 lines (or more) for each of the different validator types>

   );
}

In any case, I feel like the tests there are just redundant. There are already redundant tests in the testcase now (multiple times we're looking for the 493 value for instance.

TL;DR - I think the added tests are for and should be for checking that the type is set correctly. The other tests already validate isValid for examples of different data for each type. The bitmask problem is due to incorrect bitmask implementation so the tests test the setting of the type and leave the testing of the isValid method to the tests that are already doing that.

dstockto commented Mar 4, 2014

@weierophinney I'm just trying to get an understanding of how much we want to do with that.

I've got PR #5879 posted which converts all the existing multi-assert tests to data providers.

(Pasted from the bottom of this, if you want to know my reason on it, it's below this section.)
TL;DR - I think the added tests are for and should be for checking that the type is set correctly. The other tests already validate isValid for examples of different data for each type. The bitmask problem is due to incorrect bitmask implementation so the tests test the setting of the type and leave the testing of the isValid method to the tests that are already doing that.

I guess the question on this one remains:

We've got tests for isValid for ~12 data types for each NotEmpty type. The issue I was originally fixing is adding the same type via a string more than once which results in the wrong internal type in the validator. The tests shows that the behavior is corrected. It seems to make sense that if the tests that assure validation works properly if the type is set properly work then we rely on those for isValid. The other test should determine if setting type works properly which is a different piece of functionality.

I guess where I'm getting stuck on the data provider tests that you and @Maks3w have asked for is that it seems to be "thorough" I'd need some way to do a dataProvider provider test -- in other words testing the duplication of the string setting for each type and then for each type testing that it's valid in the same way the existing tests check.

For instance, if an individual data set was

array('boolean', array( ... array of example types and expected validity value ...))

Then the way that test would run would be to loop over each of the values in the inner array to make sure that it's valid in the way we expect. Another option is essentially duplicate every data provider test from #5879 but have the test try to set up the validator with a duplicate string type, and then use the same data providers from those tests.

One final way I've thought of would be to have a single test, but with a massive 200+ line data provider that looks something like this:

public function bigOlDataProvider() 
{ 
    return array(
        array('boolean', true, true),
        array('boolean', false, false),
        array('boolean', 1, true),
        array('boolean', 0, true),
..<snip 8 more types for boolean>..

        array('integer', 0, false),
        array('integer', 1, true),
        array('integer', false, true),
       array('integer', true, true),
.. <snip 8 more types for integer> ..

<snip 12 lines (or more) for each of the different validator types>

   );
}

In any case, I feel like the tests there are just redundant. There are already redundant tests in the testcase now (multiple times we're looking for the 493 value for instance.

TL;DR - I think the added tests are for and should be for checking that the type is set correctly. The other tests already validate isValid for examples of different data for each type. The bitmask problem is due to incorrect bitmask implementation so the tests test the setting of the type and leave the testing of the isValid method to the tests that are already doing that.

@Ocramius

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Mar 4, 2014

Member

@dstockto could you rebase this? There are conflicts because of your recently merged PR about refactoring tests for this validator

Member

Ocramius commented Mar 4, 2014

@dstockto could you rebase this? There are conflicts because of your recently merged PR about refactoring tests for this validator

@@ -360,7 +363,7 @@ public function testArrayConfigNotation()
public function testMultiConstantNotation()
{
$filter = new NotEmpty(
- NotEmpty::ZERO + NotEmpty::STRING + NotEmpty::BOOLEAN

This comment has been minimized.

Show comment Hide comment
@Ocramius

Ocramius Mar 4, 2014

Member

This should probably not be changed, but a different test method with the bitwise OR could be used

@Ocramius

Ocramius Mar 4, 2014

Member

This should probably not be changed, but a different test method with the bitwise OR could be used

This comment has been minimized.

Show comment Hide comment
@dstockto

dstockto Mar 4, 2014

@Ocramius the reason I changed it was because it is the wrong way to set the flags. There's really nothing we can do to prevent someone from doing something like

new NotEmpty(NotEmpty::BOOLEAN + NotEmpty::BOOLEAN);

I can certainly put it back and add another test for the bitwise OR but I figured it was a bad example in the tests.

@dstockto

dstockto Mar 4, 2014

@Ocramius the reason I changed it was because it is the wrong way to set the flags. There's really nothing we can do to prevent someone from doing something like

new NotEmpty(NotEmpty::BOOLEAN + NotEmpty::BOOLEAN);

I can certainly put it back and add another test for the bitwise OR but I figured it was a bad example in the tests.

@Ocramius Ocramius self-assigned this Mar 4, 2014

@weierophinney

This comment has been minimized.

Show comment Hide comment
@weierophinney

weierophinney Mar 4, 2014

Member

@dstockto I don't think we need to test isValid any more than it already is -- as you correctly note, this is more about ensuring that the bitmask does not get corrupted. Test that, and you're good. (I think you may already have done that.)

Member

weierophinney commented Mar 4, 2014

@dstockto I don't think we need to test isValid any more than it already is -- as you correctly note, this is more about ensuring that the bitmask does not get corrupted. Test that, and you're good. (I think you may already have done that.)

David Stockton added some commits Feb 26, 2014

David Stockton
Update NotEmpty validator to use bitmasking instead of addition and s…
…ubtraction to fix an issue when string types are duplicated
David Stockton
Rebase onto master
Merge branch 'notEmptyBitMask' of github.com:dstockto/zf2 into notEmptyBitMask

Conflicts:
	tests/ZendTest/Validator/NotEmptyTest.php
@dstockto

This comment has been minimized.

Show comment Hide comment
@dstockto

dstockto Mar 4, 2014

@Ocramius I think it's rebased now. It did some odd things so if it is not how you would like it, please let me know. I may need some assistance with the rebase.

dstockto commented Mar 4, 2014

@Ocramius I think it's rebased now. It did some odd things so if it is not how you would like it, please let me know. I may need some assistance with the rebase.

@dstockto

This comment has been minimized.

Show comment Hide comment
@dstockto

dstockto Mar 4, 2014

@weierophinney The test for setting the bitmask is on line 734 and is a data provider for all type constants provided. It ensures that setting the string twice for each will result in the expected bitmast value.

dstockto commented Mar 4, 2014

@weierophinney The test for setting the bitmask is on line 734 and is a data provider for all type constants provided. It ensures that setting the string twice for each will result in the expected bitmast value.

@dstockto

This comment has been minimized.

Show comment Hide comment
@dstockto

dstockto Mar 4, 2014

I think I broke something with the rebase, looking at it now.

dstockto commented Mar 4, 2014

I think I broke something with the rebase, looking at it now.

@dstockto

This comment has been minimized.

Show comment Hide comment
@dstockto

dstockto Mar 4, 2014

Should be good now. Not sure how two whole methods ended up being duplicated.

dstockto commented Mar 4, 2014

Should be good now. Not sure how two whole methods ended up being duplicated.

weierophinney added a commit that referenced this pull request Mar 4, 2014

Merge pull request #5874 from dstockto/notEmptyBitMask
Update NotEmpty validator to use bitmasking

weierophinney added a commit that referenced this pull request Mar 4, 2014

weierophinney added a commit to zendframework/zend-validator that referenced this pull request May 15, 2015

Merge pull request zendframework/zendframework#5874 from dstockto/not…
…EmptyBitMask

Update NotEmpty validator to use bitmasking

weierophinney added a commit to zendframework/zend-validator that referenced this pull request May 15, 2015

weierophinney added a commit to zendframework/zend-validator that referenced this pull request May 15, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment