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

JSON schema validation passes when it shouldn't #465

Closed
DannyBen opened this issue Dec 19, 2023 · 12 comments
Closed

JSON schema validation passes when it shouldn't #465

DannyBen opened this issue Dec 19, 2023 · 12 comments
Assignees
Labels
bug Something isn't working

Comments

@DannyBen
Copy link
Owner

DannyBen commented Dec 19, 2023

It appears that our bashly.json schema allows invalid value types to pass as valid.

The below is how it should behave:

Schema

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "type": "object",
  "properties": {
    "default": {
      "type": "string"
    }
  },
  "required": ["default"],
  "additionalProperties": false
}

Test file

# default: some string
default:
  - one
  - two

Result

Fails as expected when running this command:

$ check-jsonschema --schemafile schema.json test.yml
Schema validation errors were encountered.
  test.yml::$.default: ['one', 'two'] is not of type 'string'

(this test was done to rule out check-jsonschema as the culprit)


However, when using disallowed value types in bashly.yml, the validation passes:

bashly.yml

# bashly.yml
default:
  key1: value
  key2: value

Result

$ check-jsonschema --schemafile bashly.json bashly.yml
ok -- validation done

In fact, anything I use seem to pass validation, even this bashly.yml

asd: asd
foo: bar
@DannyBen DannyBen added the bug Something isn't working label Dec 19, 2023
@DannyBen DannyBen changed the title JSON schema passes as valid when it shouldn't JSON schema validation passes when it shouldn't Dec 19, 2023
@DannyBen
Copy link
Owner Author

DannyBen commented Dec 19, 2023

The problem lies within the $ref call to custom-properties.

This schema will pass validation on any given file.

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "definitions": {
    "custom-properties": {
      "patternProperties": {
        "^x_.": {
          "title": "custom property"
        }
      }
    }
  },
  "type": "object",
  "properties": {
    "default": {
      "type": "string"
    }
  },
  "required": ["default"],
  "additionalProperties": false,
  "$ref": "#/definitions/custom-properties"
}

when tested with check-jsonschema. I wonder if it is the same when tested with other validators.

However, this schema works as expected:

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "type": "object",
  "properties": {
    "default": {
      "type": "string"
    }
  },
  "required": ["default"],
  "additionalProperties": false,
  "patternProperties": {
    "^x_.": {
      "title": "custom property"
    }
  }
}

@DannyBen
Copy link
Owner Author

Quoting from this page:

In Draft 4-7, $ref behaves a little differently. When an object contains a $ref property, the object is considered a reference, not a schema. Therefore, any other properties you put in that object will not be treated as JSON Schema keywords and will be ignored by the validator. $ref can only be used where a schema is expected.

If I am reading this right, it means we cannot use $ref to include a partial property, no?

@DannyBen
Copy link
Owner Author

Wow, this JSON schema thing is so ugly......

I believe we should be using allOf:

      "items": {
        "allOf": [
          { "$ref": "#/$defs/argument" },
          { "$ref": "#/$defs/custom-properties" }
        ]
      }

as hinted by this SO page, but I was unable to make it work both for inner elements (argument) and root.

If needed, I can make a much simpler YAML template that can be converted to JSON without using $ref at all - but I am done for today, will wait for your input.

@DannyBen
Copy link
Owner Author

One thing that I found out that is working, is to remove the definition of the custom-properties and instead, use the patternProperties snippet as is multiple times. Then, the schema seems to be working as intended.

Should I do it?

@EmilyGraceSeville7cf
Copy link
Collaborator

Wow, this JSON schema thing is so ugly......

I believe we should be using allOf:

      "items": {
        "allOf": [
          { "$ref": "#/$defs/argument" },
          { "$ref": "#/$defs/custom-properties" }
        ]
      }

as hinted by this SO page, but I was unable to make it work both for inner elements (argument) and root.

If needed, I can make a much simpler YAML template that can be converted to JSON without using $ref at all - but I am done for today, will wait for your input.

I was thinking about creating something like a JSON schema generator from YAML. I guess it's a good take, but it should be a separate project that can be incorporated in this one.

@EmilyGraceSeville7cf
Copy link
Collaborator

One thing that I found out that is working, is to remove the definition of the custom-properties and instead, use the patternProperties snippet as is multiple times. Then, the schema seems to be working as intended.

Should I do it?

Or we can use allOf. The problem is that VS Code tells nothing about this issue for instance and the schema seems to work inside it...

@DannyBen
Copy link
Owner Author

I was thinking about creating something like a JSON schema generator from YAML.

This is easily done. I have already tried it with yq for converting the JSON schema to YAML, replacing the $ref with YAML anchor, and then regenerating it back. Should I post it in a branch for review?

Or we can use allOf.

I was unable to make it work for the root element.

The problem is that VS Code tells nothing about this issue

The quote I pasted about $ref overriding everything else seems to be the definitive guide as I understand.

@EmilyGraceSeville7cf
Copy link
Collaborator

YAML Language support uses JSON Schemas to understand the shape of a YAML file, including its value sets, defaults and descriptions. The schema support is shipped with JSON Schema Draft 7.

Quote from this page. But the following YAML for some reason is still not valid:

aa: 1
default: a

for:

{
  "$schema": "http://json-schema.org/draft-07/schema#",
  "definitions": {
    "custom-properties": {
      "patternProperties": {
        "^x_.": {
          "title": "custom property"
        }
      }
    }
  },
  "type": "object",
  "properties": {
    "default": {
      "type": "string"
    }
  },
  "required": ["default"],
  "additionalProperties": false,
  "$ref": "#/definitions/custom-properties"
}

@EmilyGraceSeville7cf
Copy link
Collaborator

EmilyGraceSeville7cf commented Dec 20, 2023

Should I do it?

I am not sure, we can try to use this validator to match VS Code editor behavior as much as possible. Maybe I am wrong, but I guess VS Code extension supports more than draft 7 according to its behavior.

P.S. To set up Go, this action can be used.

This is easily done. I have already tried it with yq for converting the JSON schema to YAML, replacing the $ref with YAML anchor, and then regenerating it back. Should I post it in a branch for review?

Yeah, I wanna see it. ;)

@DannyBen
Copy link
Owner Author

👉 #468

@DannyBen
Copy link
Owner Author

DannyBen commented Dec 20, 2023

we can try to use this validator to match VS Code editor behavior

Matching to VS Code validation is in no way a priority for me. Our schema should simply be standards compliant. If VS code validation works as intended (i.e., it FAILS when there are arbitrary values that do not start with x_), then it means that either VS Code is wrong, or check-jsonschema is wrong.

I tested jv and judging just by the 10 minutes of test, it is not better:

  1. It also does not complain when testing the below YAML against our currently live bashly schema
  2. It does not allow reading YAML with anchors (like I have with almost all of my bashly.yml files).

Test YAML:

default: asd
x_root: asd
asd: asd

args:
- name: asd
  x_asd: 1

@DannyBen
Copy link
Owner Author

Fixed in #468 + #467

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants