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

StringsSchema has not implemented 'not'. #116

Closed
kazie opened this issue Oct 31, 2017 · 2 comments
Closed

StringsSchema has not implemented 'not'. #116

kazie opened this issue Oct 31, 2017 · 2 comments

Comments

@kazie
Copy link

kazie commented Oct 31, 2017

Whoever it might concern.

While using the latest Everit Json-schema validator, I happen to notice that it didn't let me run though on strings where I had implemented "not enum" blocks, or other "not" blocks for that matter.

Examples Schema:

{
  "type": "object",
  "properties": {
    "name": {
      "type": "string",
      "not": {
        "type": "string",
        "pattern": "^([0-9a-fA-F][0-9a-fA-F])+$"
      }
    }
  },
  "required": [
    "name"
  ]
}

Didn't invalidate

{
 "name" : "DEAD"
}



Or in the enum case.
```json
{
  "type": "object",
  "properties": {
    "name": {
      "type": "string",
      "not": {
        "enum": [
          "A",
          "B",
          "C"
        ]
      }
    }
  },
  "required": [
    "name"
  ]
}

Didn't invalidate:

{
   "name": "A"
}

As one can test one self, should be valid in other validators, such as the online validator on:
https://jsonschemalint.com/#/version/draft-06/markup/json


I will soon propose suggested fix, which also validates the changes by:

  1. Not breaking old tests.
  2. Validating new tests cases as described above.

Please if any comments, you can either "fix-it-yourself" or tell me if I need some changes in the code for a proposed pull request that will come with this ticket.

Best Regards,

kazie pushed a commit to kazie/json-schema that referenced this issue Oct 31, 2017
    This fix will clear the issue that I got when using "not" blocks for validation of string.
    Example would be to add forbidden values (not enum) or anti-patterns (not pattern).
    Implementation should be quite straight forwards, and be implementing "not" as a complete schema of its own - using the existing "NotSchema" class to validate the schema.
@erosb
Copy link
Contributor

erosb commented Oct 31, 2017

Hello @kazie ,

first, if you rewrite the schema to this form, then it will work:

{
  "type": "object",
  "properties": {
    "name": {
      "allOf" : [
         {
            "type": "string"
         },
         {
            "not": {"enum": ["A","B","C"]}
         },

      ]
    }
  },
  "required": [
    "name"
  ]
}

Second, please don't fix it as-is.

The library makes the false assumption that the keywords are / can be "grouped" in a specific type-related way. When the schema gets loaded, the loader makes some investigation about the possible type of the schema, then it loads the schema by taking only the schema-specific keywords into account.

In your case, when the loader finds "type":"string" , it checks further only the string-related keywords (like "pattern", "format", "minLength", etc..). It does not look for "not" because it isn't considered as a string-related keyword, and it is implemented in NotSchema (so "not" works only if the schema has only a single "not" key).

This has been the most common reason why issues are raised for this library, so probably it would make sense to attempt to fix it.

The best thing I can imagine is

  • locate the place where the type of the schema is decided (I think it will be near to SchemaLoader#load() and SchemaLoader#sniffSchemaByProps() )
  • make this mechanism smarter, by
    • collecting the possible matching types, instead of just considering the first one that looks correct
    • if there seem to be multiple different Schema objects to be created, then create all of them, and wrap them into a CombinedSchema with allOf().

In other words the plan is to do the same rewrite automatically that I showed an example of above.

I would welcome any efforts to it. Otherwise I'm not sure if I will find time to fix it before the end of year. Of course it affects fundamental parts of the library so this change should be tested thoroughly.

If you have any further questions on it then please don't hesitate to ask.

@kazie
Copy link
Author

kazie commented Oct 31, 2017

Thank you for your through comment @erosb.

I can confirm that this also works good in my other project which is using multiple different json parsers, and that no other ones disliked the allOf() solution.

Too bad to hear that this is a common issue!

Best Regards,

@kazie kazie closed this as completed Oct 31, 2017
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

No branches or pull requests

2 participants