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

Commas seem optional in flow sequences - is this intended? #170

Open
ae-govau opened this issue Sep 18, 2023 · 2 comments
Open

Commas seem optional in flow sequences - is this intended? #170

ae-govau opened this issue Sep 18, 2023 · 2 comments

Comments

@ae-govau
Copy link

We noticed that:

new YamlReader("['a'b]").read();

would parse without error, returning a list with 2 strings, "a" and "b".

Other parsers (pyyaml, SnakeYAML) fail on parsing this, due to expecting a comma between entries.

This YAML specification https://yaml.org/spec/1.2.2/#flow-sequences states:

Flow sequence content is denoted by surrounding “[” and “]” characters.
...
Sequence entries are separated by a “,” character.

I found it surpising that this library allowed it and thought it worth an issue for discussion.

Running a few more tests, it doesn't seem to always work - so I expect allowing the above is a subtle bug. e.g.

        System.err.println(new YamlReader("['a'b]").read());
        System.err.println(new YamlReader("[a b c d]").read());
        System.err.println(new YamlReader("['a,b' c d]").read());
        System.err.println(new YamlReader("['a'b'c'd]").read());

outputs the following:

[a, b]
[a b c d]
[a,b, c d]
[a, b'c'd]
@NathanSweet
Copy link
Member

NathanSweet commented Sep 20, 2023

I don't remember if that was done purposefully. In my other project, JsonBeans, I made commas optional purposefully. For YamlBeans it could be bug a or a feature, depending on perspective.

TBH the entire YAML format is a pretty big mess. I stopped using it in my projects.

In your 4 examples it appears to behave reasonably: delimiting a value with quotes separates it from subsequent values. If we can't find a scenario where the behavior is problematic, I'm inclined to leave it.

@ae-govau
Copy link
Author

Thanks for responding.

It's not a big deal for us - we only noticed as GitHub was warning us about 2 vulns in this library (GHSA-jm7r-4pg6-gf26 and GHSA-vj49-j7rc-h54f), and as we migrated away from it to another library that was already in our stack, we found a couple of unit-tests in our code that began failing with the new library. When we investigated we found it was due to us inadvertently using invalid YAML in our unit-test, that this library allowed.

Since it seemed against the spec (and all other parsers I'd seen) and that we'd already spent the time investigating I thought I'd drop a drive-by issue - but we don't feel strongly about whether you change the behaviour or not.

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

No branches or pull requests

2 participants