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

Type errors inform wrong type when parent schema has keyword "title" #845

Closed
1 of 4 tasks
nieomylnieja opened this issue Feb 14, 2023 · 5 comments · Fixed by #853
Closed
1 of 4 tasks

Type errors inform wrong type when parent schema has keyword "title" #845

nieomylnieja opened this issue Feb 14, 2023 · 5 comments · Fixed by #853
Assignees
Labels
Milestone

Comments

@nieomylnieja
Copy link

Describe the bug

It seems that using the title keyword propagates the element name to all it's properties which also have a type of object.
I verified the behaviour against another JSON Schema implementation: https://www.jsonschemavalidator.net/.
Given the following schema:

{
  "$schema": "http://json-schema.org/draft-07/schema",
  "$id": "file:///home/mh/schema/test.schema.json",
  "type": "object",
  "title": "THE OBJECT",
  "properties": {
    "foo": {
      "type": "object"
    },
    "bar": {
      "type": "string"
    }
  }
}

It produced the following error:
2023-02-14_17-23
Which makes absolute sense, since the type of the property foo is object.
However that's not the case with yaml-language-server, which instead treats all type errors for object properties of a schema with set title keyword as If they had the same type as the parent schema, i.e. THE OBJECT.
2023-02-14_17-24
It only happens when the property is of type object, If we now change bar to an invalid type, let's say object it will produce the right error, telling us it should be a string:
2023-02-14_17-34

Expected Behavior

Displayed error should be referring to the property type, not the parent schema type.

Current Behavior

Displayed error informs about parent schema type, instead of the property type mismatch.

Steps to Reproduce

Copy the schema provided in the description and play around with it as described.

Environment

  • Windows
  • Mac
  • Linux
  • other (please specify)
@nieomylnieja
Copy link
Author

I did some preliminary code digging and it looks like validate function in jsonParser07.ts walks over it's properties propagating the parent schema, when _validateNode is called, it does this:

//get more specific name than just object
const schemaType = schema.type === 'object' ? getSchemaTypeName(schema) : schema.type;

getSchemaTypeName accepts the parent schema and eventually returns it's title as the schemaType which is used to construct the error:

if (schema.title) {
  return schema.title;
}

It's fine for parent schema, but why would we want to propagate that to all it's properties which are objects? Am I missing sth (decision wise) here?

@rgrunber
Copy link
Member

rgrunber commented Mar 1, 2023

Ok, so this issue should be fixed but I think I understand the original intent. Spitting out a warning that tells users the type should be object is not very helpful. My guess is that by using title (or the title of the parent), it gives users a better sense of exactly what the format is, and where to find it. The problem is, a user is expecting a type, not a reference to an object in the schema.

Maybe an error message like 'Incorrect type. Expected "object" (THE OBJECT). would have been nicer when the expected type is just object.

@msivasubramaniaan
Copy link
Contributor

Ok, so this issue should be fixed but I think I understand the original intent. Spitting out a warning that tells users the type should be object is not very helpful. My guess is that by using title (or the title of the parent), it gives users a better sense of exactly what the format is, and where to find it. The problem is, a user is expecting a type, not a reference to an object in the schema.

Maybe an error message like 'Incorrect type. Expected "object" (THE OBJECT). would have been nicer when the expected type is just object.

Ya It may be useful. I will raise another PR on top of this

@nieomylnieja
Copy link
Author

@rgrunber this makes even more sense, having both seems like a win win. I would even argue it might be a good approach for both scenarios (having title set in the current node too), it's always good to know there's an object expected instead of a string, if the users are only presented with the title they will always have to resort to checking it with the schema.

@msivasubramaniaan
Copy link
Contributor

The fix will be available on the YAML 1.12.0

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

Successfully merging a pull request may close this issue.

3 participants