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

Bugfix for #215 #224

Closed
wants to merge 1 commit into from
Closed

Bugfix for #215 #224

wants to merge 1 commit into from

Conversation

rtucek
Copy link
Contributor

@rtucek rtucek commented Feb 1, 2016

Fixed bug, discovered in #215.
Also added regression test.

@bighappyface
Copy link
Collaborator

@rtucek thanks for the contribution!

A couple of things:

  1. Would you please squash this PR down to a single commit?
  2. Would you please simplify the test schema and document to focus only on demonstrating the code change? Please review the other test cases for this constraint and/or tests in the official test suite (https://github.com/json-schema/JSON-Schema-Test-Suite)

…an object but also not an instance of itself.

Added a regression test.
rtucek added a commit to rtucek/json-schema that referenced this pull request 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 2, 2016

Hi @bighappyface,

thank your for your quick reply.

  1. Yeah, that should have been done in the first place - sry.
  2. Well, I personally don't agree with simplifying the test. It was a complex schema and JSON that uncovered the bug...

Anyhow, I did the rebase as requested and it passed the test.

@bighappyface
Copy link
Collaborator

@rtucek thanks for the update.

Upon deeper review of the changes, I think we have misplaced the minProperties/maxProperties validation logic in UndefinedConstraint when it should be within ObjectConstraint as those are validation keywords of objects (http://json-schema.org/latest/json-schema-validation.html#anchor53).

If the validation were located in the appropriate place then the negation added in this PR would probably be unnecessary. Further, this package already validates all of the minProperties/maxProperties testcases provided by the JSON Schema Test Suite:

https://github.com/json-schema/JSON-Schema-Test-Suite/blob/1.1.0/tests/draft4/minProperties.json
https://github.com/json-schema/JSON-Schema-Test-Suite/blob/1.1.0/tests/draft4/maxProperties.json

Those test cases demonstrate the constraint validation specifically. That is what I meant by " please simplify the test schema and document to focus only on demonstrating the code change." The latest update to the new test case seems a bit odd to me because it's redundant and the "document" is an empty object literal ({}) that doesn't demonstrate the schema provided afterward.

Would you please look at migrating the minProperties/maxProperties validation logic to the ObjectConstraint class to see if it works are expected with your original schema and JSON document?

@rtucek
Copy link
Contributor Author

rtucek commented Feb 2, 2016

Yeah, it would definitely be a true fix for the issue - rather than just a dirty one-line quicky 😄
Probably I get this done until the end of the week.

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

Successfully merging this pull request may close these issues.

2 participants