jsonnext / codemirror-json-schema

A JSONSchema enabled mode for codemirror 6, for json4 and json5, inspired by monaco-json
https://codemirror-json-schema.netlify.app
MIT License
57 stars 9 forks source link

"required" property not checked? #75

Closed alyjak closed 7 months ago

alyjak commented 8 months ago

Looking through https://github.com/acao/codemirror-json-schema/blob/main/src/__tests__/json-validation.spec.ts I don't see a test to ensure that required properties within an object are checked for. Moreover, in my application, I'm not seeing any lint errors when I include an object in my json that are missing required properties per my linked schema.

Is this expected behavior? If not, I'm not sure if this repo, https://www.npmjs.com/package/json-schema-library, or https://www.npmjs.com/package/@types/json-schema is the root of the issue.

acao commented 8 months ago

It is not expected behavior. Required properties work in some cases but not all.

to note, @types/json-schema is a type library, so it is not present at runtime. this is true of any library prefixed with @types or any typescript - when typescript is transpiled for browser compilation with bundlers, all the type annotation and type imports are removed.

the library that takes care of validation logic is json-schema-library, so we will need to report this bug to them!

our validation tests don't cover everything because it's bad practice to test the features of external libraries in unit testing, and if they fail, there's nothing we can do about it except open a PR to json-schema-library and ask them to fix the bug. the vast amount of the business logic for our library is autocomplete, however for validation and hover we just provide a thin presentation layer for json-schema-library results

can you supply an example of the schema that isn't working?

SilverDY commented 7 months ago

I am facing the same problem.

{
    "$schema": "https://json-schema.org/draft/2020-12/schema",
    "$id": "https://example.com/product.schema.json",
    "properties": {
        "ex": {
            "type": "number"
        },
        "in": {
            "type": "number"
        }
    },
    "required": [
        "ex",
        "in"
    ],
    "additionalProperties": false
}

required doesn't work in any case.

acao commented 7 months ago

Ok so I took a break from covid to check this out, and you're correct, it is indeed a bug with how we are generating the pointers from json-schema-library, and thus it is something we should test for!

I hope to have a patch fix released before the weekend

acao commented 7 months ago

asking my collaborator @imolorhe - do you agree that we should be including each array item in the parsed pointer map? this way, we can target specific array items for errors like these? image

update: The answer appears to be yes? I found a way to get the parser to handle this case, but the change breaks the autocomplete because the autocomplete doesn't expect the parser to have these pointers.

Otherwise, it restores error messages for required fields both at the root and in nested/declaration cases, at the cost of autocomplete. This will take a little while!

acao commented 7 months ago

Here it is working for the n index of an object definition! image

The only missing feature of this parser fix is, the parser only returns an array index pointer if it's an object. so an array like [{ foo: 1 }] works, but a validation error with foo: [1,2,null] does not return an array index to match the error that #/foo/2 should be an integer for example. so yay I've fixed this bug, but found more bugs 😆

imolorhe commented 7 months ago

do you agree that we should be including each array item in the parsed pointer map? this way, we can target specific array items for errors like these?

Yeah this sounds good to me!

acao commented 7 months ago

@alyjak @SilverDY 0.5.1 is the fix release! sorry for the delay

SilverDY commented 7 months ago

@acao thank you so much for that!