fix bug when applying defaults #405

mathroc · 2017-03-23T10:36:12Z

Warning: First parameter must either be an object or the name of an existing class in vendor/justinrainbow/json-schema/src/JsonSchema/Constraints/UndefinedConstraint.php line 261

to reproduce:

<?php

use JsonSchema\Constraints\Constraint;

include_once __DIR__."/../vendor/autoload.php";

$validator = new \JsonSchema\Validator();

$data = [];
$schema = (object)[
    "type" => "array",
    "items" => (object)[
        "required" => [],
        "type" => "object",
    ]
];

$validator->validate($data, $schema, Constraint::CHECK_MODE_APPLY_DEFAULTS);

according to the spec:

The value of "items" MUST be either a schema or array of schemas.

but here the code supposes it's an array of schemas and it fails when it's trying to use [] (the value of required) as a schema

erayd · 2017-03-23T13:15:26Z

Nice catch! Appreciate you finding / fixing this :-).

It's 2:15am here, but your bug report has made me think - if minItems is set, and items is a schema, then we should probably set the default as many times as needed to fulfil that. Thoughts?

The comment on test #21 needs fixing, it says the same thing twice.

I'll review this properly in the morning when I'm not half asleep ;-).

mathroc · 2017-03-23T13:32:59Z

It's 2:15am here, but your bug report has made me think - if minItems is set, and items is a schema, then we should probably set the default as many times as needed to fulfil that. Thoughts?

yes we could :)

The comment on test #21 needs fixing, it says the same thing twice.

fixed

I'll review this properly in the morning when I'm not half asleep ;-).

👍

erayd · 2017-03-23T20:25:53Z

tests/Constraints/DefaultPropertiesTest.php

@@ -149,6 +149,16 @@ public function getValidTests()
                '{"items":[{"default":null}]}',
                '[null]'
            ),
+            array(// #21 items might be an array of schema (instead of an array of schema)


This comment needs clarifying - it says the same thing twice.

erayd · 2017-03-23T20:28:38Z

tests/Constraints/DefaultPropertiesTest.php

@@ -149,6 +149,16 @@ public function getValidTests()
                '{"items":[{"default":null}]}',
                '[null]'
            ),
+            array(// #21 items might be an array of schema (instead of an array of schema)
+                '[{}]',
+                '{"items":{"properties":{"propertyOne":{"default":"valueOne"}},"required":[]}}',


What is the purpose of required in this test?

erayd · 2017-03-23T20:28:54Z

tests/Constraints/DefaultPropertiesTest.php

+            ),
+            array(// #22 if items is not an array, it does not create new item
+                '[]',
+                '{"items":{"properties":{"propertyOne":{"default":"valueOne"}},"required":[]}}',


What is the purpose of required in this test?

they were not, it's just how I found out about this bug and I didn't realized I could remove it from the tests

erayd · 2017-03-23T20:46:20Z

Thanks for fixing this bug :-).

yes we could :)

Are you intending to do the work on minItems (as part of this PR, or in another PR), or would you like me to do it? I'm quite happy either way :-).

mathroc · 2017-03-24T10:00:25Z

I've added support for minItems, tell me if you think of something missing (I'll squash everything when it's ready to merge)

erayd · 2017-03-24T19:15:42Z

src/JsonSchema/Constraints/UndefinedConstraint.php

+            if (LooseTypeCheck::isArray($schema->items)) {
+                $items = $schema->items;
+            } elseif (isset($schema->minItems) && count($value) < $schema->minItems) {
+                $items = array_fill(count($value), $schema->minItems - 1, $schema->items);


Why the magic number? Shouldn't this be less count($value), rather than less 1?

absolutely! I thought the second argument of array_fill was $end_index but it's $num…

erayd · 2017-03-24T19:17:09Z

src/JsonSchema/Constraints/UndefinedConstraint.php

+            $items = array();
+            if (LooseTypeCheck::isArray($schema->items)) {
+                $items = $schema->items;
+            } elseif (isset($schema->minItems) && count($value) < $schema->minItems) {


Needs to check and handle the state of $schema->exclusiveMinimum.

I has no idea this existed, I'll add a test for exclusiveMinimum

mathroc · 2017-03-24T23:36:31Z

should be ok if I understood exclusiveMinimum correctly

erayd · 2017-03-25T00:07:41Z

Grrr.... I've sent you on a wild goose chase sorry, exclusiveMinimum applies to minimum only, not to minItems (and therefore should not be in this PR). I should not have tried to review this when I was half asleep; this is my mistake.

Eveything else looks fine; once the ExclusiveMinimum stuff goes away (sorry!) this should be merge-ready.

mathroc · 2017-03-25T00:19:29Z

don’t worry, I read its definition too quickly too, it’s gone

erayd

Looks good - thanks :-)

all items and add support for minItems when applying defaults

mathroc · 2017-03-25T01:12:13Z

rebased

bighappyface

+1

…jsonrainbow#405) all items and add support for minItems when applying defaults

* Split $objectDefinition into $schema and $properties (#411) Object validation attempts to use a single variable to store both the object definition, and its properties. This causes validation to be incomplete where "properties" is not set, but "additionalProperties" is. This commit fixes both bugs in issue #353. * Issue-414: Allow The Option of T or space for Date time. (#415) * Testcase for minProperties with properties defined (#416) + Fix Test * Tweak phpdocumentor dependency to avoid install conflicts (#421) * [BUGFIX] Cast empty schema arrays to object (#409) * Cast root to object * Use function_exists to allow polyfill compatibility * Move array->object conversion to SchemaConstraint & SchemaStorage Fixes issue #408 * fix bug when applying defaults for array items when the schema is for (#405) all items and add support for minItems when applying defaults * [BUGFIX] Split "uri" format into "uri" & "uri-reference", fix meta-schema bug (#419) * Split "uri" format into "uri" and "uri-reference" * Correct format for id & $ref in draft-03/04 meta-schemas See json-schema-org/JSON-Schema-Test-Suite#177 (comment)

mathroc force-pushed the fix/defaults-items branch 3 times, most recently from 6dce921 to 0771bff Compare March 23, 2017 13:00

mathroc changed the title ~~[wip] fix bug when applying defaults~~ fix bug when applying defaults Mar 23, 2017

erayd requested changes Mar 23, 2017

View reviewed changes

mathroc force-pushed the fix/defaults-items branch from 0771bff to ea6bf9f Compare March 24, 2017 09:57

mathroc force-pushed the fix/defaults-items branch 2 times, most recently from 03a09cd to b854caa Compare March 24, 2017 10:51

erayd requested changes Mar 24, 2017

View reviewed changes

erayd approved these changes Mar 25, 2017

View reviewed changes

fix bug when applying defaults for array items when the schema is for

eef0954

all items and add support for minItems when applying defaults

mathroc force-pushed the fix/defaults-items branch from a631e70 to eef0954 Compare March 25, 2017 01:11

erayd mentioned this pull request Apr 12, 2017

Backports for 5.2.1 #413

Merged

bighappyface approved these changes May 5, 2017

View reviewed changes

bighappyface merged commit 331d41b into jsonrainbow:6.0.0-dev May 5, 2017

mathroc deleted the fix/defaults-items branch May 5, 2017 15:01

erayd pushed a commit to erayd/json-schema that referenced this pull request May 5, 2017

fix bug when applying defaults for array items when the schema is for (…

e231775

…jsonrainbow#405) all items and add support for minItems when applying defaults

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix bug when applying defaults #405

fix bug when applying defaults #405

mathroc commented Mar 23, 2017

erayd commented Mar 23, 2017

mathroc commented Mar 23, 2017

erayd Mar 23, 2017

erayd Mar 23, 2017

erayd Mar 23, 2017 •

edited

Loading

mathroc Mar 24, 2017

erayd commented Mar 23, 2017

mathroc commented Mar 24, 2017

erayd Mar 24, 2017

mathroc Mar 24, 2017

erayd Mar 24, 2017

mathroc Mar 24, 2017

mathroc commented Mar 24, 2017

erayd commented Mar 25, 2017

mathroc commented Mar 25, 2017 •

edited

Loading

erayd left a comment

mathroc commented Mar 25, 2017

bighappyface left a comment

fix bug when applying defaults #405

fix bug when applying defaults #405

Conversation

mathroc commented Mar 23, 2017

erayd commented Mar 23, 2017

mathroc commented Mar 23, 2017

erayd Mar 23, 2017

Choose a reason for hiding this comment

erayd Mar 23, 2017

Choose a reason for hiding this comment

erayd Mar 23, 2017 • edited Loading

Choose a reason for hiding this comment

mathroc Mar 24, 2017

Choose a reason for hiding this comment

erayd commented Mar 23, 2017

mathroc commented Mar 24, 2017

erayd Mar 24, 2017

Choose a reason for hiding this comment

mathroc Mar 24, 2017

Choose a reason for hiding this comment

erayd Mar 24, 2017

Choose a reason for hiding this comment

mathroc Mar 24, 2017

Choose a reason for hiding this comment

mathroc commented Mar 24, 2017

erayd commented Mar 25, 2017

mathroc commented Mar 25, 2017 • edited Loading

erayd left a comment

Choose a reason for hiding this comment

mathroc commented Mar 25, 2017

bighappyface left a comment

Choose a reason for hiding this comment

erayd Mar 23, 2017 •

edited

Loading

mathroc commented Mar 25, 2017 •

edited

Loading