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

Bug: incorrect handling of minProperties and maxProperties in optional attribute #215

Closed
rtucek opened this issue Jan 13, 2016 · 4 comments
Labels

Comments

@rtucek
Copy link
Contributor

rtucek commented Jan 13, 2016

Given schema:

{
  "type": "object",
  "additionalProperties": false,
  "required": [
    "session",
    "include_hotel",
    "arrival",
    "departure",
    "ip",
    "travellers",
    "language_iso_code"
  ],
  "properties": {
    "session": {
      "type": "string"
    },
    "campaign": {
      "type": "integer"
    },
    "service_provider": {
      "type": "integer"
    },
    "include_hotel": {
      "type": "boolean"
    },
    "arrival": {
      "type": "string",
      "pattern": "^[0-9]{4}-[0-9]{2}-[0-9]{2}$"
    },
    "departure": {
      "type": "string",
      "pattern": "^[0-9]{4}-[0-9]{2}-[0-9]{2}$"
    },
    "ip": {
      "type": "string"
    },
    "offerId": {
      "type": "integer",
      "minimum": 1
    },
    "quiet": {
      "type": "boolean"
    },
    "language_iso_code": {
      "type": "string"
    },
    "travellers": {
      "type": "array",
      "minItems": 1,
      "maxItems": 16,
      "items": {
        "type": "integer"
      }
    },
    "rooms": {
      "type": "object",
      "additionalProperties": false,
      "minProperties": 1,
      "maxProperties": 2,
      "properties": {
        "size_1": {
          "type": "number"
        },
        "size_2": {
          "type": "number"
        }
      }
    },
    "offset": {
      "type": "integer"
    }
  }
}

And this data:

{
  "campaign": 1,
  "offset": 0,
  "service_provider": 3,
  "include_hotel": false,
  "arrival": "2016-01-20",
  "departure": "2016-01-27",
  "session": "foo",
  "ip": "bar",
  "language_iso_code": "de",
  "travellers": [
    18,
    18,
    18
  ]
}

Proofed by http://jsonschemalint.com/draft4/ the json is valid.
However the validator is returning the following error:

[
    0 =>  [
        "property" => "rooms",
        "message" => "Must contain no more than 2 properties",
        "constraint" => "maxProperties",
        "maxProperties" => 2
    ]
];

This should not happen because the property rooms is not given.
Thus their is no reason for a check on maxProperties constraint.

@rtucek rtucek changed the title Bug: incorrect handling of minProperties and maxProperties in optional object Bug: incorrect handling of minProperties and maxProperties in optional attribute Jan 17, 2016
@bighappyface
Copy link
Collaborator

@rtucek good find. Pull requests welcome 😉

rtucek added a commit to rtucek/json-schema that referenced this issue Feb 1, 2016
…an object but also not an instance of itself.
rtucek added a commit to rtucek/json-schema that referenced this issue Feb 1, 2016
rtucek added a commit to rtucek/json-schema that referenced this issue Feb 1, 2016
* fix-issue-215:
  Added a regression test for jsonrainbow#215.
  This patch fixes jsonrainbow#215 by ensuring that $value is not just an object but also not an instance of itself.
@rtucek rtucek mentioned this issue Feb 1, 2016
@rtucek
Copy link
Contributor Author

rtucek commented Feb 1, 2016

Here we got #224:
Fix + regression test + build passed.

rtucek added a commit to rtucek/json-schema that referenced this issue Feb 2, 2016
* fix-issue-215:
  This patch fixes jsonrainbow#215 by ensuring that $value is not just an object but also not an instance of itself. Added a regression test.
  Rebased for jsonrainbow#224
@rtucek
Copy link
Contributor Author

rtucek commented Feb 15, 2016

Ok, so after my little PR mess, I've provided 2 different approaches for fixing the issue.

#232 as you suggested
#233 as my original and restored proposal

Note that this time I haven't submitted a test.
Yes, I've noticed the json-schema/JSON-Schema-Test-Suite repo and I'm aware of that the phpunit is successfully testing against them.

So far, so good, but it's false-failing against my document which is perfectly valid, but I little more complex compared the standard tests (and that's how you trigger the bug in this library, having some nested constraints).

And this was the reason why I used this document as a regression test - just for having a pre-post comparison and for demonstrating the code change.
I'm sry that I'm failing simplifying the test any further without demonstrating anything xD
May you give me a concrete example of what you're thinking of?

@bighappyface
Copy link
Collaborator

Closed with #232

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

No branches or pull requests

2 participants