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

encoding/json: UnmarshalTypeError.Error message seems insensible #43126

Open
dsnet opened this issue Dec 10, 2020 · 5 comments
Open

encoding/json: UnmarshalTypeError.Error message seems insensible #43126

dsnet opened this issue Dec 10, 2020 · 5 comments

Comments

@dsnet
Copy link
Member

@dsnet dsnet commented Dec 10, 2020

Consider this snippet:

type GoStruct1 struct {
	AlphaField GoStruct2 `json:"alpha_field"`
}
type GoStruct2 struct {
	BravoField []GoStruct3 `json:"bravo_field"`
}
type GoStruct3 struct {
	StringField string `json:"string_field"`
}

func main() {
	fmt.Println(json.Unmarshal([]byte(`{"alpha_field":{"bravo_field":[{"string_field": 3.14159}]}}`), new(GoStruct1)))
}

On Go1.15, this prints:

json: cannot unmarshal number into Go struct field GoStruct3.alpha_field.bravo_field.string_field of type string

Notice that it combines two different namespaces where it is:

{{GoTypeName}}.{{JSONPath}}

and that the message says that it is a "Go struct field" when it isn't.

\cc @mvdan

@dsnet
Copy link
Member Author

@dsnet dsnet commented Dec 10, 2020

Related considerations:

  • Should the JSON path include array indexes?
  • Should the JSON path disambiguate the path if dots appear in the object name? Example.

Strawman proposal: the JSON path should be a valid expression in JavaScript to address the specific value from the root value.

  • For objects, we use .foo if the object name is a valid JavaScript identifier, otherwise we use ["fizz.buzz"] if it isn't.
  • For arrays, we use [5].

@mvdan
Copy link
Member

@mvdan mvdan commented Dec 11, 2020

I agree with the strawman proposal. In that proposal, what would you use in place of GoStruct3?

Also, is there a "json path" spec we can simply adhere to? or do we simply try to get as close to a JS expression as we reasonably can?

cc @cuonglm who worked on the original feature. I did think about proposing to make it more consistent with slices and such, but at the end of the day I thought the addition was already a big step forward.

@toothrot toothrot added this to the Backlog milestone Dec 11, 2020
@dsnet
Copy link
Member Author

@dsnet dsnet commented Dec 11, 2020

Also, is there a "json path" spec we can simply adhere to?

One option is RFC 6901, which uses / instead of . as the delimiter. This would be the visual difference between .fooName["quoted/name"].barName[5] and /fooName/quoted~1Name/barName/5. I kind of like the simplicity and visual aesthetics of this. It only requires escaping of the ~ and / as ~0 and ~1 respectively (note that ASCII control characters in U+0000 to U+0020 are left as is).

do we simply try to get as close to a JS expression as we reasonably can?

The grammar for a JS identifier is pretty complicated. Fortunately, the Go identifier grammar is a strict subset of the JS identifier grammar, so we could just use that as a shortcut.

@dsnet
Copy link
Member Author

@dsnet dsnet commented Dec 16, 2020

Another oddity. If the target type is a Go map, then the JSON object names for those are dropped: https://play.golang.org/p/DP-bAr_6TUy

@fl0cke
Copy link

@fl0cke fl0cke commented Dec 20, 2020

The GoStruct3 at the beginning is extremely confusing. I would either

  • replace it with the type name of the outermost struct, e.g. GoStruct1.alpha_field.bravo_field.string_field or nothing if the target is a map.
  • replace it with a $ sign, e.g. $.alpha_field.bravo_field.string_field. This is similar to how JSON path expressions are formatted in MariaDB. See https://mariadb.com/kb/en/jsonpath-expressions/.
  • omit it completely, e.g. alpha_field.bravo_field.string_field

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants