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

Duplicate keys in mappings don't cause parse error #226

Closed
rhysd opened this issue May 24, 2021 · 4 comments · Fixed by #531
Closed

Duplicate keys in mappings don't cause parse error #226

rhysd opened this issue May 24, 2021 · 4 comments · Fixed by #531
Labels
bug Something isn't working parser

Comments

@rhysd
Copy link

rhysd commented May 24, 2021

YAML spec says at 3.2.1.1:

The content of a mapping node is an unordered set of key: value node pairs, with the restriction that each of the keys is unique.

So keys in mappings MUST be unique. This is different from JSON spec where keys SHOULD be unique.

To conform the YAML spec, any YAML parser should cause parse error on duplicate keys. However this library does not cause a parse error.

package main

import (
	"github.com/goccy/go-yaml/parser"
)

func main() {
	s := "foo:\nfoo:\n"
	_, err := parser.ParseBytes([]byte(s), 0)
	if err == nil {
		panic("should have caused parse error")
	}
}

Here is a link to playground: https://play.golang.org/p/xog5p3vkQFj

@marco-m
Copy link
Contributor

marco-m commented Nov 9, 2024

Hello @goccy , I stumbled on the same problem. For my better understanding of the suitability of go-yaml, could you please give us your idea? That is, do you consider this a bug or expected behavior? Thanks!

@goccy goccy added bug Something isn't working parser labels Nov 12, 2024
@goccy
Copy link
Owner

goccy commented Nov 12, 2024

@marco-m @rhysd Thank you for your reports. I plan to fix this.

@rhysd
Copy link
Author

rhysd commented Nov 12, 2024

Checking duplicates affects performance (because the parser needs to remember the previous keys). So I'm not sure this check is needed as the default behavior. In the case, adding some opt-in/opt-out check may be suitable to control the trade off.

@goccy
Copy link
Owner

goccy commented Nov 13, 2024

I've fixed this behavior with #531 . I decided to make an error as the default behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working parser
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants