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: should try to convert strings<->numbers without errors if it's possible #22463

Closed
alexbyk opened this issue Oct 27, 2017 · 6 comments
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone

Comments

@alexbyk
Copy link

alexbyk commented Oct 27, 2017

Trying to combine api/JSON/databases in Go is a pain. The problem is it's very hard with JS applications/frameworks to be sure about the correctness of JSON output. Most probably everyone ends up with something like this:

// let product= {id: "1", price: "100"}
product.id = +product.id;
product.price = +product.price;
// .. send to API

Go could help a lot in this cases and the code above could be avoided, because Go has all the information needed to correctly convert, for example, this JSON {"id": "1", "price": "100"} into this structure

type Product struct {
	ID    int64   `json:"id"`
	Price float64 `json:"price"`
}

... but instead of that it panics

Even more, there is a json.Number type which partially solves the problem, but still you should do a lot of checks and conversions (like product.id.Float64()) because it's not a basic type and also database/sql doesn't support it.

The only way I found to deal with this stuff is to create a new type, based on int64, and implement json.Unmarshaler interface. But it's still a lot of typing and error prone

The full example about what time-saving encoding/json could do out of the box (maybe with some flags to not break an existing code)

package main

import (
	"encoding/json"
	"fmt"
)

type Product struct {
	ID    int64   `json:"id"`
	Price float64 `json:"price"`
}

func parseJSON(p *Product, str string) error {
	err := json.Unmarshal([]byte(str), p)
	if err != nil {
		return err
	}
	return nil
}

func ExampleJson() {
	strs := []string{
		// works
		`{"id": 1, "price": 100}`,
		`{"id": 1, "price": 100.0}`,

		// this should work too because Go has enough information
		// to convert these strings into int64/float64, but it doesn't :(
		`{"id": 1, "price": "100"}`,
		`{"id": 1, "price": "100.00"}`,
		`{"id": "1", "price": 100}`,
	}
	for _, str := range strs {
		p := Product{}
		err := parseJSON(&p, str)
		if err != nil {
			fmt.Println(err)
		}
		fmt.Printf("%+v, discount: %v\n", p, p.Price*0.1)

	}
	// Output:
	// {ID:1 Price:100}, discount: 10
	// {ID:1 Price:100}, discount: 10
	// {ID:1 Price:100}, discount: 10
	// {ID:1 Price:100}, discount: 10
	// {ID:1 Price:100}, discount: 10
}

In my opinion, err shouldn't be nil only in case when Product.id can't be converted to int64, Product.price to float64 and so on. The client code shouldn't be aware of what internal types are we using

Also the same should be true about this case:

var id string
err := json.Unmarshal([]byte(`123`), &id)
if err != nil {
	panic(err)
}
fmt.Println(id)

Go has enough information to assume that we want to convert JSON number 123 to Go's string, because it's the only reasonable case and JS client code shouldn't be bothered what's our id's internal type. But instead it panics

If encoding/json could behave like this, it would make working with JSON in Go much more pleasant and concise


UPD: Actually it's not a cool feature, It's more like an obvious expectation. For example, database/sql works as expected with this

var idStr string
db.QueryRow("select id from categories limit 1").Scan(&idStr)
var idInt int64
db.QueryRow("select id from categories limit 1").Scan(&idInt)

fmt.Printf("%+v\n", idStr)
fmt.Printf("%+v\n", idInt)

idStr will be "1" and idInt will be 1 (for example). So json should follow the same logic

Writing custom types with custom MarshalJSON/UnmarshalJSON isn't trivial and working with custom types isn't a pleasure at all. Also casting js code (id = +id) or writing strconv.Atoi() for every json input is annoying.

As a +1 for this proposal, there is a json.Number type and a string tag. I assume they were created exactly for such kind of cases. But they don't work and look like a hack. With the behavior, explained above, there will be no need of them. At least when dealing with JS api clients

As a +2, this behavior can save a lot of time for backend and frontend developers. Golang is a typed language, why not to use it at full strength?

@cznic
Copy link
Contributor

cznic commented Oct 27, 2017

In JSON, 6 is the number six. "6" is a string containing the digit 6. So the answer to the question "Can json numbers be quoted?" is basically "no," because if you put them in quotes, they're not numbers anymore.

src

@alexbyk
Copy link
Author

alexbyk commented Oct 27, 2017

@cznic how is it relevant to my proposal? I guess you read this issue too briefly and missed a point.

My point is json.Unmarshal should be able to convert json string "6" as well as json number 6 into var id int64 or into var id string because it's obvious what result are we expecting.

To be clear, database/sql works already the same way

var idStr string
db.QueryRow("select id from categories limit 1").Scan(&idStr)
fmt.Printf("%+v\n", idStr)

var idInt int64
db.QueryRow("select id from categories limit 1").Scan(&idInt)
fmt.Printf("%+v\n", idInt)

Both idInt and IdStr are acceptable for .Scan because it knows exactly what type we're expecting and how to convert Database's int/string into Go's int64 and string. So there is no reason to not do the same for encoding/json

@mvdan
Copy link
Member

mvdan commented Oct 27, 2017

I'm not convinced that this has a place in encoding/json. Like you said, you can always implement the Unmarshaler interface. I don't understand why that would be more error-prone, as long as you get it right once, test it well and re-use the code.

Many features and enhancements to encoding/json have been discarded in the past. The reason is that the package should be simple and have few knobs, while still being reasonably extensible. If you need any convenience or extra features on top of that, you can always use an external JSON package, and there are many.

This issue is also proposing a non-trivial design change to a package, so I would strongly recommend writing a proposal: https://github.com/golang/proposal

@cznic
Copy link
Contributor

cznic commented Oct 27, 2017

My point is json.Unmarshal should be able to convert json string "6" as well as json number 6 into var id int64 or into var id string because it's obvious what result are we expecting.

My point is that it should better not do that. What numbers are the obviously expected results for strings like "10101", "0666" or "deadbeef", for example?

@alexbyk
Copy link
Author

alexbyk commented Oct 27, 2017

My point is that it should better not do that. What numbers are the obviously expected results for strings like "10101", "0666" or "deadbeef", for example?

For these particular examples and Go's int64 it's obvious: the result of the invocation:
n, err := strconv.ParseInt(str, 10, 64).

  • "10101" - 10101
  • "0666" - 666
  • "deadbeef" - 0, error

That's because "...octal and hexadecimal formats are not used" in JSON

For float64, the result of the invocation: strconv.ParseFloat

@ianlancetaylor ianlancetaylor added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Oct 27, 2017
@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Oct 27, 2017
@ianlancetaylor ianlancetaylor changed the title encoding/json should try to convert strings<->numbers without errors if it's possible encoding/json: should try to convert strings<->numbers without errors if it's possible Oct 27, 2017
@rsc
Copy link
Contributor

rsc commented Oct 30, 2017

Being precise about whether a value is a number or a string is a feature, not a bug.

But if you find yourself in this situation, it should be possible to write your own value types for these ambiguous fields, have them implement json.Unmarshaler, and add whatever semantics make sense for your application. That's the extensibility hook that encoding/json expects users to make. Extending the app itself scales much better than putting every customization like this into the main package.

Please try that.

@rsc rsc closed this as completed Oct 30, 2017
@golang golang locked and limited conversation to collaborators Oct 30, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Development

No branches or pull requests

6 participants