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

Default values don't get applied to objects inside of an array #417

Open
pcjacobse opened this issue Apr 21, 2017 · 7 comments
Open

Default values don't get applied to objects inside of an array #417

pcjacobse opened this issue Apr 21, 2017 · 7 comments
Projects

Comments

@pcjacobse
Copy link

Hi,

It seems that the default values don't get applied to objects inside of an array.
It looks like that only the top level object gets their default values applied.

The validation in the sub-object does work (like type checking and required fields).

I'm running version 5.2.0

Example:
I expect the 'test' property to be filled with the default value if not present.

$data = [
    (object)[
        "key" => 1,
        "test" => 1
    ],(object)[
        "key" => 1,
    ]
];
$validator = new \JsonSchema\Validator();

var_dump($validator->validate(
    $data,
    json_decode('
    {
        "type": "array",
        "items": [
            {
                "type": "object",
                "properties": {
                    "key": {
                        "type": "integer"
                    },
                    "test": {
                        "type": "integer",
                        "default": 0
                    }
                },
                "required": [
                    "key"
                ]
            }
        ]
    }
    '), \JsonSchema\Constraints\Constraint::CHECK_MODE_APPLY_DEFAULTS
));
var_dump($data);

Gives:

int(0)

array(2) {
  [0]=>
  object(stdClass)#3 (2) {
    ["key"]=>
    int(1)
    ["test"]=>
    int(1)
  }
  [1]=>
  object(stdClass)#2 (1) {
    ["key"]=>
    int(1)
  }
}
@erayd
Copy link
Collaborator

erayd commented Apr 21, 2017

@pcjacobse Thanks for reporting this.

I've found the root cause - the logic that prevents recursive application of defaults is being incorrectly triggered for arrays, with the result that the library thinks it has already applied the default value for "test" and skips it.

Currently thinking of the best way to address this.

@erayd
Copy link
Collaborator

erayd commented Apr 21, 2017

@mathroc Do you have an opinion?

The problem is the call to $path->setFromDefault() in the array section of UndefinedConstraint::applyDefaultValues(). It needs to be called only for each element that has been set from a default, and not for the entire array the way it is now. However... we don't, at that point, actually have a path object for the array members - $path refers to the array itself.

Obviously this needs fixing, but it may require changing the recursion logic for defaults.

@erayd erayd mentioned this issue May 15, 2017
@mathroc
Copy link
Contributor

mathroc commented May 15, 2017

@erayd sorry I missed this, I’ll take a look at this in the next few days

@mathroc
Copy link
Contributor

mathroc commented May 19, 2017

@erayd this seems a bit tricky 😕 one think though, shouldn't $path->setFromDefault(); be called at the end of the previous if (one line up) ?

it will still cause problems when default items are defined inside arrays but that's not the case in @pcjacobse exemple

@krismckay
Copy link

@mathroc @erayd @pcjacobse
I'm not sure if you guys have made any progress on this, but I made the following change and it appears to be working for me.
UndefinedConstraint.php:Line273
Add the following after the $path->setFromDefault();
$this->applyDefaultValues($value[$currentItem], $schema->items[$currentItem], $path);
Which is just recursively passing the array items through the same function. If this ends up blowing up on me I'll repost.

@erayd
Copy link
Collaborator

erayd commented Aug 30, 2017

@krismckay Line 273 of UndefinedConstraint.php (branch 6.0.0-dev) is } else {. Could you please clarify exactly where you're proposing that line be inserted? If you could include some of the surrounding code in your post for context, that would be helpful. I'm assuming that you probably meant to insert after L277.

Overall, I like the fundamental approach of your idea. The implementation as you've posted it isn't quite suitable (the path passed for recursion needs to be correct) - however, recursing within this block is a good idea, and seems like an ideal solution to the problem.

If you are intending to submit a PR for this, please base it on 6.0.0-dev, and address the issues above. If not, let me know, and I'll do a PR when I have the time.

@krismckay
Copy link

krismckay commented Aug 30, 2017

False alarm! ..

Digging into this a little more I found it is all working fine, the issue was in my formatting of the schema. Rather then making the array definition an object, I had it as an array of objects which was causing the validation to miss the array items.
In my own version, I've corrected the schema and reverted the code back to default.

To illustrate, I had this (which breaks):

      "logics": {
        "type": "array",
        "items": [
           {
            "type": "object",
            "properties": {
              "logic_id": {
                "title": "Logic ID",
                "type": "integer"
              },
              "enabled": {
                "title": "Enabled",
                "type": "boolean",
                "default": true
              },
              "priority": {
                "title": "Priority",
                "type": "integer",
                "default": 1
              }
            }
          }
        ]
      },

When I should have had this:

      "logics": {
        "type": "array",
        "items": {
          "type": "object",
          "properties": {
            "logic_id": {
              "title": "Logic ID",
              "type": "integer"
            },
            "enabled": {
              "title": "Enabled",
              "type": "boolean",
              "default": true
            },
            "priority": {
              "title": "Priority",
              "type": "integer",
              "default": 1
            }
          }
        }
      },

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
v6.0.0
To do
Development

No branches or pull requests

4 participants