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

panic: json: cannot unmarshal object into Go struct field Payload.Data of type main.Key #342

Closed
aexvir opened this issue Mar 8, 2022 · 5 comments · Fixed by #353
Closed

Comments

@aexvir
Copy link
Sponsor

aexvir commented Mar 8, 2022

we're still evaluating go-json as replacement for jsoniter on our setup, and during our testing I've found another issue where go-json wouldn't be able to parse valid json that both encoding/json and jsoniter parse just fine

this time it seems to be caused by using custom types with custom unmarshal functions as map keys
when attaching the debugger with a breakpoint on the UnmarshalJSON function I can't see the function being called at all, so it seems to be panicking somewhere earlier

reproducible by:

package main

import (
	"bytes"
	"fmt"

	"github.com/goccy/go-json"
)

const (
	First  = "first item"
	Second = "second item"
)

type Key string

func (key *Key) UnmarshalJSON(b []byte) error {
	var s string
	if err := json.Unmarshal(b, &s); err != nil {
		return err
	}

	switch s {
	case "first":
		*key = First
	case "second":
		*key = Second
	default:
		return fmt.Errorf("option not recognized: %s", s)
	}

	return nil
}

type Value string

func (val *Value) UnmarshalJSON(b []byte) error {
	var s string
	if err := json.Unmarshal(b, &s); err != nil {
		return err
	}

	*val = Value("letter " + s)
	return nil
}

type Payload struct {
	Data map[Key]Value `json:"data"`
}

const jsondata = `
{
	"data": {
		"first": "a",
		"second": "d"
	}
}
`

func main() {
	var pld Payload

	reader := bytes.NewReader([]byte(jsondata))
	err := json.NewDecoder(reader).Decode(&pld)
	if err != nil {
		panic(err)
	}

	data, err := json.Marshal(pld)

	fmt.Println(string(data))
}

note: just by removing (or commenting out) the func (key *Key) UnmarshalJSON(b []byte) function will make the parsing successful, so it doesn't look like a specific issue just because of custom types as map keys

@orisano
Copy link
Sponsor Contributor

orisano commented Mar 8, 2022

There is an incompatible behavior.
but encoding/json does not support UnmarshalJSON on map key type.
https://go.dev/play/p/gtPW1rnJBuG

@orisano
Copy link
Sponsor Contributor

orisano commented Mar 8, 2022

encoding/json directly converts the string to KeyType. it does not call UnmarshalJSON.
https://github.com/golang/go/blob/6e63be7b69aab25ac66029e7dfec47303d3b7505/src/encoding/json/decode.go#L792

@aexvir
Copy link
Sponsor Author

aexvir commented Mar 8, 2022

oh, yep, you're right! good catch
this seems to be something from jsoniter, and I incorrectly assumed encoding/json was behaving the same

@aexvir
Copy link
Sponsor Author

aexvir commented Mar 8, 2022

but in that case I don't think that go-json should return and error; it should just ignore it as encoding/json, no?
is go-json trying to do something with the unmarshal function? or why is it erroring?

and... will this feature be supported? 😅 should I file another issue as feature request?
I think it's a nice feature, and while we can work around it by validating the strings after the unmarshaling, it would be much nicer to have it done on a single step

@aexvir
Copy link
Sponsor Author

aexvir commented Mar 21, 2022

someone smarter than me pointed out today that encoding/json actually does allow for custom unmarshaling on map keys, as long as the custom type implements the encoding.TextMarshaler interface 😅
it's documented on the official docs

I verified that the UnmarshalText function indeed gets called when using encoding/json: https://goplay.space/#FA41T64uoGy

so adding support for custom unmarshalers on map keys is needed for this library to be compliant with the std lib; allowing to use it like jsoniter would be just additional sugarcoat

edit: I just realized after providing the goplay.space link that actually that works just fine in go-json 😅
so the only issue here is that it shouldn't return an error when UnmarshalJSON is present, or just add support for that

orisano added a commit to orisano/go-json that referenced this issue Mar 24, 2022
fix goccy#342
The map key decoder has an incompatible behavior when the type kind is string and the type has UnmarshalJSON.
goccy added a commit that referenced this issue Mar 24, 2022
fix: an incompatible behavior on map key decoder
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

Successfully merging a pull request may close this issue.

2 participants