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

Jump to bottom

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
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Backlog

Comments

@dsnet
Copy link
Member

dsnet commented Dec 10, 2020 •
edited

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.
Also, it uses GoStruct3, when it makes little sense.
At minimum, it should at least used GoStruct1.

\cc @mvdan
@dsnet
Copy link
Member Author

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 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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 11, 2020
@toothrot toothrot added this to the Backlog milestone Dec 11, 2020
@dsnet
Copy link
Member Author

dsnet commented Dec 11, 2020 •
edited

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 commented Dec 16, 2020 •
edited

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 commented Dec 20, 2020 •
edited

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

@dsnet dsnet mentioned this issue May 22, 2021
Open
@rsc rsc mentioned this issue Jun 2, 2021
Open
@dsnet dsnet mentioned this issue Mar 1, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Backlog
Development

No branches or pull requests
4 participants
@toothrot @mvdan @fl0cke @dsnet