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

Block specific, error prone, yaml key names #694

Open
AvnerCohen opened this issue Oct 22, 2024 · 8 comments
Open

Block specific, error prone, yaml key names #694

AvnerCohen opened this issue Oct 22, 2024 · 8 comments

Comments

@AvnerCohen
Copy link

null:
  code: XK
  value1: true
  value2: true

This "null" is valid, but will fail in many cases, depends on the programming language reading this yaml.

Is there any yamllint rule I could use to protect such "error prone key names" ? (I can think of none, false and true as additional candidates)

@adrienverge
Copy link
Owner

Hello Avner,

Indeed, although confusing in most contexts, this code is valid YAML!

Yamllint doesn't currently provide a way to forbid specific types of keys.
We could imagine a new rule like key-types like this:

rules:
  key-types:
    # types from https://perlpunk.github.io/YAML-PP-p5/schemas.html
    forbid-null: true|false  # tag:[yaml.org](http://yaml.org/),2002:null
    forbid-bool: true|false  # tag:[yaml.org](http://yaml.org/),2002:bool
    forbid-int: true|false   # tag:[yaml.org](http://yaml.org/),2002:int
    forbid-float: true|false # tag:[yaml.org](http://yaml.org/),2002:float
    forbid-str: true|false   # tag:[yaml.org](http://yaml.org/),2002:str

@AvnerCohen
Copy link
Author

This would be amazing!
I am not at familiar with the YAML vocabulary, I am wondering if the fact the "forbid" here, should be explicitly to "key names" ? (or should it be "tag names") ?
and so the rule would be:

rules:
  key-types:
    forbid-as-key-null: true|false

etc..

But this is cosmetic, and I suspect you know what you are doing :)

Would be amazing to see something like that implemented.

Also, important to consider case here (Null, None, null, NULL etc).

@adrienverge
Copy link
Owner

I am not at familiar with the YAML vocabulary, I am wondering if the fact the "forbid" here, should be explicitly to "key names" ? (or should it be "tag names") ?

I'm not sure, because this rule would be about forbidding the use of specific types (null, boolean, integers, for instance values null, true or 1.2e3), not about names (e.g. "null" or "string"). (Maybe I didn't understand your question well, in which case feel free to rephrase.)

Also, important to consider case here (Null, None, null, NULL etc).

I think in YAML 1.2 only the following values are null: null, Null, NULL and ~ (my source is @perlpunk's https://perlpunk.github.io/YAML-PP-p5/schemas.html).

Would be amazing to see something like that implemented.

Contributions are welcome :)
They need to follow guidelines and come with tests and documentation.

@AvnerCohen
Copy link
Author

Thanks @adrienverge So, to be sure I get your suggestion here:

dict_of_values:
   null: 'value'

In the above example, if I get this right, "null" is used as "Key" in the dict_of_values dictionary.

So I think I am asking about how to block "keys" with certain and specific "string values" that might be ok in YAML, but as soon as you are being "parsed" by python/javascript/etc the name chosen might cause issues down the line..

@adrienverge
Copy link
Owner

adrienverge commented Oct 29, 2024

Hello Avner,

I'm not sure I get you right, maybe there's a misunderstanding about the type of null in YAML?

In your example YAML snippet, null is not a string, it the null value. Hence, it's not a key with a name, it's a reserved keyword in YAML.
Said differently, the 2 following dictionaries are different:

# Example 1: null value
dict_of_values:
   null: 'value'
# Example 2: string value, with value "null"
dict_of_values:
   'null': 'value'

Unlike JSON, using the null value as a key in a mapping is allowed in YAML (see examples at the end of https://yaml.org/type/null.html).

If your proposal is to create a rule to avoid the use of dict: {null: 'value'}, I find it meaningful. (And we could allow forbidding similar cases like dict: {4: 'value'} or dict: {false: 'value'}).

However if it's about forbidding specific key names (= string values like dict: {'null': 'value'}) I'm against it, because yamllint doesn't aim to check the values / content of YAML data (only its form).

@AvnerCohen
Copy link
Author

Thanks @adrienverge thanks for sticking with me on this :) I imagine this is a combination of my language barrier (yaml) and my language barrier (not native english).. But I feel we are making progress :)

We are aligned on the goal here, it's on "null value" case. To illustrate the issue I am seeing:

>>> import yaml
>>> yaml_string = """
... dict_of_values:
...   null: 'value'
... """
>>> yaml.safe_load(yaml_string)
{'dict_of_values': {None: 'value'}}
>>>

The resulting "None" is going to cause issues down the road and also creates interoperability issues because here is how it looks in a Javascript Yaml parser:

{
  "dict_of_values": {
    "null": "value"
  }
}

Going back to your original suggestion:

rules:
  key-types:
    # types from https://perlpunk.github.io/YAML-PP-p5/schemas.html
    forbid-null: true|false  # tag:[yaml.org](http://yaml.org/),2002:null
    forbid-bool: true|false  # tag:[yaml.org](http://yaml.org/),2002:bool
    forbid-int: true|false   # tag:[yaml.org](http://yaml.org/),2002:int
    forbid-float: true|false # tag:[yaml.org](http://yaml.org/),2002:float
    forbid-str: true|false   # tag:[yaml.org](http://yaml.org/),2002:str

Are you able to provide any pointers on how to progress this with a contribution ?

@adrienverge
Copy link
Owner

To illustrate the issue I am seeing:

{'dict_of_values': {None: 'value'}}

Agreed 👍 then we're talking about the same thing 🙂

Are you able to provide any pointers on how to progress this with a contribution ?

I suggest checking out CONTRIBUTING.rst first, then for a good start you can look at the code of other rules, in particular read some commits that introduced new rules into yamllint (you'll see they must come with tests and documentation). Good luck!

@AvnerCohen
Copy link
Author

@adrienverge Please see the initial suggested direction at #695
There is more way to go of course, but wanted to see if this make sense as the general direction or I am completely off, which is not impossible..:)

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