Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix bug when applying defaults #405

Merged
merged 1 commit into from
May 5, 2017

Conversation

mathroc
Copy link
Contributor

@mathroc mathroc commented Mar 23, 2017

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

@mathroc mathroc force-pushed the fix/defaults-items branch 3 times, most recently from 6dce921 to 0771bff Compare March 23, 2017 13:00
@mathroc mathroc changed the title [wip] fix bug when applying defaults fix bug when applying defaults Mar 23, 2017
@erayd
Copy link
Contributor

erayd commented Mar 23, 2017

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
Copy link
Contributor Author

mathroc commented Mar 23, 2017

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 ;-).

👍

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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":[]}}',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of required in this test?

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

@erayd erayd Mar 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the purpose of required in this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

erayd commented Mar 23, 2017

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
Copy link
Contributor Author

mathroc commented Mar 24, 2017

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

@mathroc mathroc force-pushed the fix/defaults-items branch 2 times, most recently from 03a09cd to b854caa Compare March 24, 2017 10:51
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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@mathroc
Copy link
Contributor Author

mathroc commented Mar 24, 2017

should be ok if I understood exclusiveMinimum correctly

@erayd
Copy link
Contributor

erayd commented Mar 25, 2017

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
Copy link
Contributor Author

mathroc commented Mar 25, 2017

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

Copy link
Contributor

@erayd erayd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - thanks :-)

all items and add support for minItems when applying defaults
@mathroc
Copy link
Contributor Author

mathroc commented Mar 25, 2017

rebased

@erayd erayd mentioned this pull request Apr 12, 2017
Copy link
Collaborator

@bighappyface bighappyface left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@bighappyface bighappyface merged commit 331d41b into jsonrainbow:6.0.0-dev May 5, 2017
@mathroc 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
…jsonrainbow#405)

all items and add support for minItems when applying defaults
bighappyface pushed a commit that referenced this pull request May 16, 2017
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants