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

Schema validator doesn't recognize Null literals #118

Closed
SupSuper opened this issue Jan 4, 2019 · 4 comments
Closed

Schema validator doesn't recognize Null literals #118

SupSuper opened this issue Jan 4, 2019 · 4 comments
Labels
Milestone

Comments

@SupSuper
Copy link

SupSuper commented Jan 4, 2019

Simple test case, make a schema with "type": "null"
Only the empty value is accepted as valid. The YAML null literals like ~ and null are rejected: https://yaml.org/type/null.html
This should be correctly supported by the parser: https://github.com/mulesoft-labs/yaml-ast-parser/blob/master/src/scalarInference.ts#L71

However, "type": "string" does accept Null literals, so this might be related to this fix: #72

@JPinkney JPinkney added the bug label Jan 4, 2019
@JPinkney JPinkney added this to the 0.5.0 milestone Mar 12, 2019
@zallek
Copy link

zallek commented Feb 14, 2020

@JPinkney Any updates on that?
As @SupSuper said, the problem might be due to #72 as null token are expected to be parsed as null values and not string according to the YAML spec https://yaml.org/type/null.html
I would suggest reverting this commit

config
Screen Shot 2020-02-14 at 12 21 57 PM

unexpected error:
Screen Shot 2020-02-14 at 12 21 26 PM

@timo-klarshift
Copy link

I am coming from here: redhat-developer/vscode-yaml#277

I forked the yaml-language-server now and added tests for this issue. I also provided a fix which implements this: https://yaml.org/type/null.html

But I am in very doubt, if this will be accepted, as it could be that a lot of implementations actually rely on receiving the Null literals as strings.

So I think the best solution would be to enable some configuration parameter to switch between those behaviors. But recording to the spec, I think the current implementation is just wrong here.

@timo-klarshift
Copy link

I created a PR with tests also.
I am not sure if that behavior will break something for the k8s folks.
But in any case, we should stick to the YAML spec :)

#241

@JPinkney
Copy link
Contributor

Yeah, I think this should be switched back to the old behavior. I think in this case the kubernetes schema is actually wrong. If null is allowed in that case then the accepted types should be "string"/"null" not just "string".

Even other validators online that I've found don't allow null when the type is string. When I implemented it originally I just thought they knew something that I didn't so I just figured they were right 🤷‍♂️

timo-klarshift pushed a commit to timo-klarshift/yaml-language-server that referenced this issue Feb 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants