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

unknown secondary tag name "!!cat" specified #567

Closed
jandubois opened this issue Dec 3, 2024 · 4 comments · Fixed by #572
Closed

unknown secondary tag name "!!cat" specified #567

jandubois opened this issue Dec 3, 2024 · 4 comments · Fixed by #572
Labels
bug Something isn't working

Comments

@jandubois
Copy link

The library no longer accepts custom tag names

Using this program to reproduce:

package main

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

func main() {
	doc := "mike: !!cat 3"
	var v interface{}
	if err := yaml.Unmarshal([]byte(doc), &v); err != nil {
		fmt.Print(err)
	} else {
		fmt.Printf("%+v\n", v)
	}
}

Up to v1.13.8 this was accepted by the parser:

$ go get github.com/goccy/[email protected]
go: downloading github.com/goccy/go-yaml v1.13.8
go: upgraded github.com/goccy/go-yaml v1.13.5 => v1.13.8
$ go run test.go
map[mike:<nil>]

While it did not throw an error, it did produce the wrong result; it should have been:

map[mike:3]

Starting with v1.13.9 and continuing to the latest release it throws an error:

$ go get github.com/goccy/[email protected]
go: downloading github.com/goccy/go-yaml v1.13.9
go: upgraded github.com/goccy/go-yaml v1.13.8 => v1.13.9
$ go run test.go
[1:7] unknown secondary tag name "!!cat" specified
>  1 | mike: !!cat 3
             ^

This change breaks the test suite of yq, so dependabot cannot update the github.com/goccy/go-yaml dependency, see e.g. https://github.com/mikefarah/yq/actions/runs/12061918796/job/33634760899?pr=2205

@goccy
Copy link
Owner

goccy commented Dec 3, 2024

@jandubois I couldn't imagine a case where you would want to specify something other than the reserved secondary tags in YAML. What was the original intent behind the YAML in question? While it's easy to make this not throw an error, I'm not sure if it's appropriate to allow any secondary tags

@jandubois
Copy link
Author

jandubois commented Dec 4, 2024

I guess you can argue that the tag cat does not exist in the tag:yaml.org,2002: schema because it is not included in the YAML spec.

But in general you can't know because I can use an application-specific schema with the secondary tag handle:

package main

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

func main() {
	doc := `
%TAG !! tag:jandubois.com,2024:
---
mike: !!cat 3
`
	var v interface{}
	if err := yaml.Unmarshal([]byte(doc), &v); err != nil {
		fmt.Print(err)
	} else {
		fmt.Printf("%+v\n", v)
	}
}

And it still fails the same way (because AFAICT you did not implement the %TAG directive).

So I'm not sure how useful it is to restrict the tags to the available set from the tag:yaml.org,2002: schema.

And even then, if you look at the PyYAML Documentation under the "YAML tags and Python types" heading, you will see that PyYAML is defining a bunch of custom tags in the default tag:yaml.org,2002: schema, like !!python/str etc.

I would argue that this was not correct, and they should have created their own schema, which could then inherit from the default schema.

But it is what it is, so applications extending the default schema do exist in the wild.

What was the original intent behind the YAML in question?

I don't actually know; it just appears in a test where they test the compatibility of the library: https://github.com/mikefarah/yq/blob/80310ea/pkg/yqlib/goccy_yaml_test.go#L46-L51

@goccy
Copy link
Owner

goccy commented Dec 5, 2024

Thank you for your reports. I've decided to fix this problem with #572 . Thank you !

@jandubois
Copy link
Author

Thank you! 🙏

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

Successfully merging a pull request may close this issue.

2 participants