Improvements to type coercion #384

erayd · 2017-03-13T02:00:17Z

What

Don't coerce already-valid types
Add CHECK_MODE_EARLY_COERCE to enable previous coercion behavior
Don't test further types if we've already validated one successfully
Fix reversed arguments from centralize errors #364
Implement every remaining coercion case from ajv's matrix
Rewrite coercion tests to improve readability and simplify adding future tests.

Why

See discussion in #379 regarding coercion of already-valid types and CHECK_MODE_EARLY_COERCE. Other items are performance / bugfix, or are to improve testing.

shmax · 2017-03-13T02:34:45Z

src/JsonSchema/Constraints/TypeConstraint.php

@@ -185,6 +185,10 @@ protected function validateType(&$value, $type, $coerce = false)
        }

        if ('array' === $type) {
+            if ($coerce) {
+                $value = $this->toArray($value);


Looks good, but I notice that ajv only does this conversion if coerceTypes is set to "array". Should this be controlled by another flag?

Hmm, good point. I'm happy either way - would you like a flag?

Honestly don't know. We can try without for a while, and see if the rationale for having one ever emerges.

Good suggestion - let's do that then. And if someone asks for it, then we can add it at that point.

I don't supposed you know whether @epoberezkin has stated his rationale for doing it that way in ajv?

Well, I think this is the issue that spawned the feature: ajv-validator/ajv#158

They discuss it a bit, then come up with the coerceTypes:'array' idea

Hmm... will have to think on this a bit. At a minimum I should write some tests to check that post-coercion validation is happening properly.

shmax · 2017-03-13T03:46:44Z

src/JsonSchema/Constraints/TypeConstraint.php

@@ -255,10 +269,27 @@ protected function toNumber($value)

    protected function toInteger($value)
    {
-        if (is_numeric($value) && (int) $value == $value) {
+        $numberValue = $this->toNumber($value);


Little risky, isn't it?

echo toInteger("5f"); // 5

Ooh, good catch - this is why we have code reviews! Thanks :-)

Should be sorted now.

shmax · 2017-03-13T04:39:26Z

src/JsonSchema/Constraints/TypeConstraint.php

+            return (int) $value;
+        }
+        if ($this->getTypeCheck()->isArray($value) && count($value) === 1) {
+            reset($value);


reset returns the first element in the array, so you can skip a step and pass its results directly on line 272

Good point - thanks :-).

shmax · 2017-03-13T04:40:59Z

src/JsonSchema/Constraints/TypeConstraint.php

    protected function toInteger($value)
    {
-        if (is_numeric($value) && (int) $value == $value) {
-            return (int) $value; // cast to number
+        $numberValue = $this->toNumber($value);


what does the call to toNumber get you?

All the other type coersion cases. It means no copy pasta, because other than integer-ness, the two methods are identical.

is there anything actually functionally different? Because now we're calling is_numeric twice whereas before it was only called once

Yes - the second is_numeric call catches cases that fall through toNumber without being successfully coerced.

that's what I mean, though--if you put the code back the way it was then there is only one call to is_numeric. So I guess I'm still not clear on what is functionally different about the new code. Are additional values now being coerced to integer (eg. boolean) with this code?

Yes. The new code also coerces from bool, null, and array, via the call to toNumber.

Basically the logic is to apply all the coercion rules for number, and then see if the result can be further coerced to an int. If it is, then return the cast int - if not, return the original value.

shmax · 2017-03-13T04:46:03Z

src/JsonSchema/Constraints/TypeConstraint.php

+     *
+     * @param mixed $value
+     *
+     * @return array


I believe the return type should be array|mixed

Is the version of PHPDoc in this project OK with that? I've had issues with types like that in the past.

(If it's not OK, it should be mixed)

Never mind, apparently this project doesn't use PHPDoc. I'll change to array|mixed.

just mixed is fine, too. I suggested array|mixed to emphasize that array is primarily what we're interested in

I like your original suggestion better. If we're not using PHPdoc, then there's no reason to avoid it IMO. If such a reason crops up later, it's a pretty quick / easy change to downgrade the return type to mixed.

erayd · 2017-03-13T05:31:21Z

@shmax I think this PR now contains everything we intended it to contain. Is there anything you'd like me to add / change / remove / hide-in-the-corner-and-cry-from-shame-about?

shmax · 2017-03-13T06:03:32Z

Wow, very impressive, as always! I'll take another look tomorrow when I'm fresh but for now I say LGTM

erayd · 2017-03-13T06:04:19Z

Thanks :-).

erayd · 2017-03-13T06:08:22Z

Hmm, just thought of one other thing - I need to ensure that the double-coercion for array cases is covered.

erayd · 2017-03-13T06:20:03Z

Double-coercion tests done.

erayd · 2017-03-14T00:34:09Z

@shmax How's this?

shmax · 2017-03-14T02:26:32Z

README.md

+`CHECK_MODE_EARLY_COERCE` has no effect unless used in combination with `CHECK_MODE_COERCE_TYPES`. If
+enabled, type coercion will occur as soon as possible, even if the value may already be of another valid
+type.
+


Could you maybe expand on this a bit to make it more clear? I know what you're talking about, but I'm not sure that someone else coming to the project for the first time would.

shmax · 2017-03-14T02:28:58Z

tests/Constraints/CoerciveTest.php

+            // toType
+            'string' => array(
+                //    fromType      fromValue       toValue         valid   Test Number
+                array('string',     '"string"',     'string',       true),  // #0


Wow, I love the new grid, much easier to read!

I usually find it confusing when a code sample uses values that mimic or mirror the name of the type they are meant to be demonstrating. Instead of 'string', can we use something else?

shmax · 2017-03-14T02:34:02Z

tests/Constraints/CoerciveTest.php

+        if ($valid) {
+            $this->assertTrue(
+                $validator->isValid(),
+                'Validation failed: ' . json_encode($validator->getErrors(), \JSON_PRETTY_PRINT)


shmax · 2017-03-14T02:48:13Z

tests/Constraints/CoerciveTest.php

-     */
-    public function testInvalidCoerceCasesUsingAssoc($input, $schema, $errors = array())
+    /** @dataProvider dataCoerceCases **/
+    public function testCoerceCases($schema, $data, $startType, $endType, $endValue, $valid, $early = false, $assoc = false)


What if the 7th argument was just something like $flags? Then instead of passing in true you could pass in CHECK_MODE_EARLY_COERCE, which would be a little easier to read, and you don't have to keep adding more arguments to this method signature as more flags are added.

Good idea - will do.

shmax · 2017-03-14T02:52:40Z

tests/Constraints/CoerciveTest.php

+                array('boolean',    'true',         true,           true),  // #17
+                array('NULL',       'null',         false,          true),  // #18
+                array('array',      '["true"]',     true,           true),  // #19
+                array('object',     '{"a":"b"}',     null,          false), // #20


extra space?

Yup - well spotted!

shmax · 2017-03-14T02:59:56Z

tests/Constraints/CoerciveTest.php

+                array('integer',    '45',           '45',           true),  // #1
+                array('boolean',    'true',         'true',         true),  // #2
+                array('boolean',    'false',        'false',        true),  // #3
+                array('NULL',       'null',         '',             true),  // #4


What's with the counting numbers in the comments? They look very nice, but it seems like the kind of thing that would be a pain to maintain going forward. What are they for?

They are the test number, starting from zero. PHPUnit reports those numbers when anything fails - it makes tracking down a failing test case much faster / easier when you don't have to manually count them all every time, particularly when there are this many tests.

Right, that's what I was originally thinking about when I suggested breaking things up into smaller tests. testEarlyCoerce, testArray, testNoCoerceNeeded, etc. Maybe not for the vanilla stuff, but at least the complex ones. This is okay, too, though.

Would you prefer them as separate methods? I like them better all in one place like this, because it's easier to compare them - but I can certainly break to one method per case if you'd rather.

Up to you. I can live with the numbers.

Then I'd like to keep them the way they are please - I find it much easier to debug this way :-).

shmax · 2017-03-14T03:02:20Z

tests/Constraints/CoerciveTest.php

-        $this->assertTrue(gettype($value->negativeInteger) == 'integer');
-        $this->assertTrue(gettype($value->boolean) == 'boolean');
+        // #38 check post-coercion validation (to array)
+        $tests[] = array(


I think it's okay to break out these more complicated ones like this, but since even the complex ones have only a single property, can they be setup before you iterate over them on 83?

They can. The reason I did it this way is because complex tests are more likely to be added than simple ones, and adding things to the beginning or midway through requires manually renumbering all the test cases - whereas adding something to the end is easy, and doesn't require renumbering anything.

I wasn't suggesting adding them to the big grid, only adding them after the grid but before the loop. That way you don't have to do the brain work of trying to figure out what the json string will look like.

No problem - done.

Oh wait, I get it--you can't handle the complex tests the same way you do the others because they have arrays of types, so you can't slot them in the same way as the others.

Or at least a few of them do. It seems like some of them could still be handled the same way as the ones in the big grid, eg:

before

// #39 check post-coercion validation (from array) $tests[] = array( '{"properties":{"propertyOne":{"type":"number"}}}', '{"propertyOne":["ABC"]}', 'array', null, null, false );

after

// #39 check post-coercion validation (from array) $tests['number'][] = array( 'array', ["ABC"]', null, null, false );

I'm sure I have all that in the wrong order, but hopefully you know what I mean

Good point - have moved test 39 to the grid. I don't think any of the others can move though - if you spot any, please let me know, but I think they all have reasons for being where they are.

shmax · 2017-03-14T04:04:15Z

LGTM Fantastic work! Take a victory lap

erayd · 2017-03-14T04:06:53Z

Thanks :-). Really appreciate the work you've done reviewing all this too, and the various suggestions you've made - the result is better because of that.

erayd · 2017-03-17T04:20:52Z

Rebased.

bighappyface · 2017-03-17T04:37:59Z

@erayd conflict?

erayd · 2017-03-17T04:45:19Z

@bighappyface I think we overlapped, and I rebased before you merged the conflict. I have rebased it again.

erayd · 2017-03-21T18:13:30Z

Rebased.

bighappyface · 2017-03-21T18:28:37Z

Once more, please :)

* Add all remaining coercion cases from ajv matrix * Rewrite the coercion tests to tidy things up a bit

If set, falls back to the old behavior of coercing to the first compatible type, regardless of whether another already-valid type might be available.

* Fix whitespace * Turn $early into $extraFlags * Change "string" to "ABC" in string test * Update README.md description of CHECK_MODE_EARLY_COERCE

erayd · 2017-03-21T18:43:42Z

Done.

bighappyface

+1

* Improve performance - don't loop over everything if already valid * Don't coerce already-valid types (fixes jsonrainbow#379) * Add remaining coercion cases & rewrite tests * Add all remaining coercion cases from ajv matrix * Rewrite the coercion tests to tidy things up a bit * Add CHECK_MODE_EARLY_COERCE If set, falls back to the old behavior of coercing to the first compatible type, regardless of whether another already-valid type might be available. * Add multiple-type test that requires coercion * \JSON_PRETTY_PRINT doesn't exist in PHP-5.3, so work around this * Various PR cleanup stuff * Fix whitespace * Turn $early into $extraFlags * Change "string" to "ABC" in string test * Update README.md description of CHECK_MODE_EARLY_COERCE * Move loop after complex tests definition * Move test jsonrainbow#39 to grid jsonrainbow#15

erayd mentioned this pull request Mar 13, 2017

Two possible improvements to type coercion #379

Closed

erayd changed the title ~~[BUGFIX] Don't coerce already-valid values (fixes #379 pt. 2)~~ [BUGFIX] Don't coerce already-valid types (fixes #379 pt. 2) Mar 13, 2017

erayd changed the title ~~[BUGFIX] Don't coerce already-valid types (fixes #379 pt. 2)~~ Don't coerce already-valid types (implements #379 pt. 2) Mar 13, 2017

shmax reviewed Mar 13, 2017

View reviewed changes

erayd force-pushed the bugfix-379-coercion branch from 584d05f to c9e1276 Compare March 13, 2017 05:22

erayd force-pushed the bugfix-379-coercion branch from c67f9fd to 42411d8 Compare March 13, 2017 23:56

erayd changed the title ~~Don't coerce already-valid types (implements #379 pt. 2)~~ Improvements to type coercion Mar 13, 2017

erayd mentioned this pull request Mar 14, 2017

Coerce empty string to null #381

Closed

shmax reviewed Mar 14, 2017

View reviewed changes

bighappyface added the Needs rebase on target branch label Mar 17, 2017

erayd force-pushed the bugfix-379-coercion branch from ab501e8 to 7df164c Compare March 17, 2017 04:20

erayd force-pushed the bugfix-379-coercion branch from 7df164c to 98b6516 Compare March 17, 2017 04:45

bighappyface removed the Needs rebase on target branch label Mar 17, 2017

erayd force-pushed the bugfix-379-coercion branch from 98b6516 to e64fd9e Compare March 21, 2017 18:13

bighappyface added the Needs rebase on target branch label Mar 21, 2017

erayd added 9 commits March 22, 2017 07:37

Improve performance - don't loop over everything if already valid

ba33726

Don't coerce already-valid types (fixes jsonrainbow#379)

7f7a2b1

Add remaining coercion cases & rewrite tests

d70c8d6

* Add all remaining coercion cases from ajv matrix * Rewrite the coercion tests to tidy things up a bit

Add CHECK_MODE_EARLY_COERCE

a775d4d

If set, falls back to the old behavior of coercing to the first compatible type, regardless of whether another already-valid type might be available.

Add multiple-type test that requires coercion

2ea432b

\JSON_PRETTY_PRINT doesn't exist in PHP-5.3, so work around this

4ca0541

Various PR cleanup stuff

ab0e8ba

* Fix whitespace * Turn $early into $extraFlags * Change "string" to "ABC" in string test * Update README.md description of CHECK_MODE_EARLY_COERCE

Move loop after complex tests definition

c8f4b50

Move test jsonrainbow#39 to grid jsonrainbow#15

a21741d

erayd force-pushed the bugfix-379-coercion branch from e64fd9e to a21741d Compare March 21, 2017 18:43

bighappyface removed the Needs rebase on target branch label Mar 21, 2017

bighappyface approved these changes Mar 21, 2017

View reviewed changes

bighappyface merged commit f3bfb47 into jsonrainbow:6.0.0-dev Mar 21, 2017

erayd mentioned this pull request Mar 21, 2017

Backports for 5.2.0 #368

Merged

erayd deleted the bugfix-379-coercion branch March 22, 2017 21:36

kb05 mentioned this pull request Nov 11, 2020

TypeConstraint: validateType fails with stringable objects. #645

Open

Improvements to type coercion #384

Improvements to type coercion #384

Conversation

erayd commented Mar 13, 2017 • edited Loading

What

Why

shmax Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erayd Mar 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erayd commented Mar 13, 2017 • edited Loading

shmax commented Mar 13, 2017

erayd commented Mar 13, 2017

erayd commented Mar 13, 2017

erayd commented Mar 13, 2017

erayd commented Mar 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmax Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shmax Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

shmax Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

erayd Mar 14, 2017 • edited Loading

Choose a reason for hiding this comment

shmax commented Mar 14, 2017

erayd commented Mar 14, 2017

erayd commented Mar 17, 2017

bighappyface commented Mar 17, 2017

erayd commented Mar 17, 2017

erayd commented Mar 21, 2017

bighappyface commented Mar 21, 2017

erayd commented Mar 21, 2017

bighappyface left a comment

Choose a reason for hiding this comment

erayd commented Mar 13, 2017 •

edited

Loading

shmax Mar 13, 2017 •

edited

Loading

erayd Mar 13, 2017 •

edited

Loading

erayd commented Mar 13, 2017 •

edited

Loading

shmax Mar 14, 2017 •

edited

Loading

shmax Mar 14, 2017 •

edited

Loading

shmax Mar 14, 2017 •

edited

Loading

erayd Mar 14, 2017 •

edited

Loading