code review 6202068: encoding/json: Adding json.NumberConverter for cust...

encoding/json: Adding json.NumberConverter for custom number conversion.

The default behavior of the JSON library is to convert numbers to float64
when the decode target is of type interface{}. This makes it difficult to
have any control over the handling of JSON numbers in some cases.

One such example would be a JSON-based interchange format in which the
caller has no knowledge of the structure of documents yet still wants to
store/forward documents in a lossless but efficient way, perhaps passing the
number literals through unchanged, or decoding them differently based on
whether they are floating point or integral.

Some discussion around this issue exists. In this thread:

    https://groups.google.com/d/msg/golang-nuts/oXbGCPh1e0Y/3j9tB6Eka8oJ

Russ points out that the JSON spec nor the JavaScript language don't
distinguish between "3" and "3.0", which is of course valid. That being
said, not all applications working with JSON are destined to have anything
to do with JavaScript, and having a way to opt-in to a different
interpretation than that dictated by JavaScript implementations seems
reasonable. His response indicated a suggested fix but no implementation
plan. Hopefully the implementation of NumberConverter in this change allows
for a similar, though slightly lower-level, approach to the same end result
(nor does it preclude his suggested fix at a later time).

In this discussion:

    https://groups.google.com/d/msg/golang-nuts/HGuVS-DJVh4/L_Wnat9R57AJ

Russ also suggests another way to get access to handling of JSON number
literals, which is to tap in to (and presumably expose somehow)
scanner.step. This isn't something I'd feel comfortable tinkering around
with myself, though I'm aware that it might also suffice (though would also
require me to take over control of the rest of the document parse, I
believe).

I hope that this can find its way in to the codebase, and appreciate your
time.

[jgold.bg, 2012-05-11 18:08:06]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go/

[rsc, 2012-05-15 15:07:18]

Introducing number, as I suggested in 2010, was probably the best
answer, but it never happened, and now Go 1 has fixed the current
behavior.  We could plausibly introduce NumberConverter without
breaking Go 1 compatibility, but it's kind of ugly as an API goes.

I think it might work better to introduce Number, but not used by
default, and then have UseNumber be the method instead of having
arbitrary converters.  I'd like to think about this a little more.  I
created issue 3625 to track this.

Thanks.
Russ

[jgold.bg, 2012-05-15 16:30:42]

Thanks for checking back in. I'm happy with either, and am happy to modify my
approach to use json.Number in combination with the user calling
Decoder.UseNumber().

Let me know and I'm happy to push this forward.

jonathan

[rsc, 2012-05-22 02:48:06]

Okay, let's add a type Number like in my 2010 mail and enable its use
instead of float64 as the default type using a new method UseNumber()
on *Decoder.

Russ

[jgold.bg, 2012-05-23 19:03:54]

Hello golang-dev@googlegroups.com, rsc@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[jgold.bg, 2012-05-29 20:05:05]

ping

On Wed, May 23, 2012 at 07:03:54PM +0000, jgold.bg@gmail.com wrote:
> Hello golang-dev@googlegroups.com, rsc@golang.org (cc:
> golang-dev@googlegroups.com),
> 
> Please take another look.
> 
> 
> http://codereview.appspot.com/6202068/

[rsc, 2012-05-29 21:19:23]

Thanks for working on this. The one-sentence summary of my comments would be
"write less". If you choose your words and your code carefully, then you can
make the documentation easier for users to skim and the code (especially the
tests) easier to read and maintain. Concrete suggestions below.

Note that Marshal needs to know how to marshal Numbers too, so encode.go's case
reflect.String should check for numberType too, like the comment below adds a
check for numberType in the unmarshaler.

You should probably wait until CL 6258061 has been submitted before revising the
tests, to avoid merge conflicts.

http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode.go
File src/pkg/encoding/json/decode.go (right):

http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode....
src/pkg/encoding/json/decode.go:42: //	float64, for JSON numbers
add after this line:
// Number, for JSON numbers, if UseNumber has been called

http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode....
src/pkg/encoding/json/decode.go:140: // Holder for an uninterpreted JSON number.
By default, this library decodes
See http://golang.org/doc/effective_go.html#commentary

// A Number represents a JSON number literal.
type Number string

should be fine.  The rest of the details are more concerned with methods or
marshalling or unmarshalling.

// String returns the literal text of the number.
func (n Number) String() string { return string(n) }

// Float64 returns the number as a float64.
func (n Number) Float64() (float64, error)

// Int64 returns the number as an int64.
func (n Number) Int64() (int64, error)

http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode....
src/pkg/encoding/json/decode.go:631: // Util method for invoking either d's
custom NumberConverter or the default
// convertNumber converts the number literal s to a float64 or a Number
// depending on the setting of d.useNumber.

http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode....
src/pkg/encoding/json/decode.go:636: return strconv.ParseFloat(s, 64)
The call sites will be simplified if you make the UnmarshalTypeError here.

f, err := strconv.ParseFloat(s, 64)
if err != nil {
    return nil, &UnmarshalTypeError{"number " + s, reflect.TypeOf(0.0)}
}
return f, nil

http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode....
src/pkg/encoding/json/decode.go:727: if fromQuoted {
Add at top level:

var numberType = reflect.TypeOf(Number(""))

Add here, before this line:

if v.Kind() == reflect.String && v.Type() == numberType {
    v.SetString(s)
    break
}

http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode....
src/pkg/encoding/json/decode.go:735: errType := v.Type()
With above rewrite, shortens to

d.saveError(err)
break

http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode....
src/pkg/encoding/json/decode.go:895: var errType reflect.Type
Same.

http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode_...
File src/pkg/encoding/json/decode_test.go (right):

http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode_...
src/pkg/encoding/json/decode_test.go:196: func assertNumParsePanic(f func(),
errExpct string, t *testing.T) {
http://golang.org/doc/go_faq.html#assertions

http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode_...
src/pkg/encoding/json/decode_test.go:211: // Independent of Decode, basic
coverage of the accessors in Number
A more idiomatic way to write this test would be to use a table:

const convError = -42  // otherwise unused value

var numberTests = []struct{
    in string
    f float64
    i int64
}{
    {"12", 12, 12},
    {"1.2e1", 12, 12},
    {"1.23e1", 12.3, convError},
    ...
}

Then the test iterates over the table.  When a failure happens it can print a
good message instead of dying in an assert:

func TestNumber(t *testing.T) {
    for _, tt := range numberTests {
        n := Number(tt.in)

        if n.String() != tt.in {
            t.Errorf("Number(%q).String() = %q, want %q", tt.in, n.String(),
tt.in)
        }

        f, err := n.Float64()
        if tt.f == convError {
            if err == nil {
                t.Errorf("Number(%q).Float64() = %v, nil, want error", f)
            }
        } else {
            if f != tt.f || err != nil {
                t.Errorf("Number(%q).Float64() = %v, %v, want %v, nil", f, err,
tt.f)
            }
        }

         ... same for tt.i and Int64 method
}

Then when the test fails you get much more information about the state of the
code.  It also makes adding a new test case a simple matter of writing a line in
the data table instead of copying and pasting however many lines of code

http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode_...
src/pkg/encoding/json/decode_test.go:241: // test struct to hold an unmarshal,
providing sufficient fields for us to test
Please test Unmarshal by adding to the unmarshalTests table.
I will send out a CL addings tags to the table literal so that you can add
a useNumber bool field to the unmarshalTest struct.

http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode_...
src/pkg/encoding/json/decode_test.go:279: func TestDecodeUseNumberParseFailure(t
*testing.T) {
The unmarshalTests table can accommodate error checking too.

http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/stream.go
File src/pkg/encoding/json/stream.go (right):

http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/stream....
src/pkg/encoding/json/stream.go:29: // Instructs this Decoder to convert JSON
number strings to instances of Number
http://golang.org/doc/effective_go.html#commentary

// UseNumber causes the Decoder to unmarshal a number into an interface{} as 
// a Number instead of as a float64.

[jgold.bg, 2012-05-30 03:57:22]

On Tue, May 29, 2012 at 09:19:24PM +0000, rsc@golang.org wrote:
> Thanks for working on this. The one-sentence summary of my comments

Thanks for taking the time to help me with it!

I've made the changes you set out and updated with the new patch set. Two
things I thought were worth pointing out more specifically are inline below.

> 
>
http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode....
> src/pkg/encoding/json/decode.go:42: //	float64, for JSON numbers
> add after this line:
> // Number, for JSON numbers, if UseNumber has been called

I ended up not putting this in, because there's actually no way that a caller
can tell Unmarshal to UseNumber, since that requires access to decodeState. Am
I
not understanding what you'd like?

> 
>
http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode_...
> src/pkg/encoding/json/decode_test.go:196: func assertNumParsePanic(f
> func(), errExpct string, t *testing.T) {
> http://golang.org/doc/go_faq.html#assertions
> 
>
http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode_...
> src/pkg/encoding/json/decode_test.go:211: // Independent of Decode,
> basic coverage of the accessors in Number
> A more idiomatic way to write this test would be to use a table:
> 
[SNIPPED]
> 
> Then when the test fails you get much more information about the state
> of the code.  It also makes adding a new test case a simple matter of
> writing a line in the data table instead of copying and pasting however
> many lines of code

I've adopted the table approach for the most part, based on your comments.  I'm
not sure I'm understanding how to test the failures from Number.Int64() and
Number.Float64() better than I have though, and I apologize for any
aggravation.

The thing that makes it tough is that the design of Number is such that Int64()
and Float64() do not return errors, which I think makes sense since we assume
that bad numbers will never make it past the JSON parser in the first place.
But, strconv returns errors, and I feel like it's better to panic if we
actually
get one, and I just want code coverage of that codepath. I'm finding it hard to
concisely test panics in a table loop though without calling a specialized
method (with its own defer() for making the checks), which is why you'll see
what you see.

Thanks again for your help, and my apologies if I'm just not seeing something
obvious.

[jgold.bg, 2012-06-01 15:47:04]

Ping?

Sorry if I'm being pesky or too pushy -- I'm new to the team's time frames 
and code review process/tools, so please let me know if I'm not 
participating correctly.

jonathan

On Tuesday, May 29, 2012 8:57:19 PM UTC-7, Jonathan Gold wrote:
>
> On Tue, May 29, 2012 at 09:19:24PM +0000, rsc@golang.org wrote: 
> > Thanks for working on this. The one-sentence summary of my comments 
>
> Thanks for taking the time to help me with it! 
>
> I've made the changes you set out and updated with the new patch set. Two 
> things I thought were worth pointing out more specifically are inline 
> below. 
>
> > 
> > 
>
http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode....

> > src/pkg/encoding/json/decode.go:42: //        float64, for JSON numbers 
> > add after this line: 
> > // Number, for JSON numbers, if UseNumber has been called 
>
> I ended up not putting this in, because there's actually no way that a 
> caller 
> can tell Unmarshal to UseNumber, since that requires access to 
> decodeState. Am I 
> not understanding what you'd like? 
>
> > 
> > 
>
http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode_...

> > src/pkg/encoding/json/decode_test.go:196: func assertNumParsePanic(f 
> > func(), errExpct string, t *testing.T) { 
> > http://golang.org/doc/go_faq.html#assertions 
> > 
> > 
>
http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode_...

> > src/pkg/encoding/json/decode_test.go:211: // Independent of Decode, 
> > basic coverage of the accessors in Number 
> > A more idiomatic way to write this test would be to use a table: 
> > 
> [SNIPPED] 
> > 
> > Then when the test fails you get much more information about the state 
> > of the code.  It also makes adding a new test case a simple matter of 
> > writing a line in the data table instead of copying and pasting however 
> > many lines of code 
>
> I've adopted the table approach for the most part, based on your comments. 
>  I'm 
> not sure I'm understanding how to test the failures from Number.Int64() 
> and 
> Number.Float64() better than I have though, and I apologize for any 
> aggravation. 
>
> The thing that makes it tough is that the design of Number is such that 
> Int64() 
> and Float64() do not return errors, which I think makes sense since we 
> assume 
> that bad numbers will never make it past the JSON parser in the first 
> place. 
> But, strconv returns errors, and I feel like it's better to panic if we 
> actually 
> get one, and I just want code coverage of that codepath. I'm finding it 
> hard to 
> concisely test panics in a table loop though without calling a specialized 
> method (with its own defer() for making the checks), which is why you'll 
> see 
> what you see. 
>
> Thanks again for your help, and my apologies if I'm just not seeing 
> something 
> obvious. 
>

[iant2, 2012-06-02 00:19:45]

Jonathan Gold <jgold.bg@gmail.com> writes:

> Ping?
>
> Sorry if I'm being pesky or too pushy -- I'm new to the team's time frames 
> and code review process/tools, so please let me know if I'm not 
> participating correctly.

Russ is out for a couple of days.  He will probably get back to this
next week.

Ian

> On Tuesday, May 29, 2012 8:57:19 PM UTC-7, Jonathan Gold wrote:
>>
>> On Tue, May 29, 2012 at 09:19:24PM +0000, rsc@golang.org wrote: 
>> > Thanks for working on this. The one-sentence summary of my comments 
>>
>> Thanks for taking the time to help me with it! 
>>
>> I've made the changes you set out and updated with the new patch set. Two 
>> things I thought were worth pointing out more specifically are inline 
>> below. 
>>
>> > 
>> > 
>>
http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode....

>> > src/pkg/encoding/json/decode.go:42: //        float64, for JSON numbers 
>> > add after this line: 
>> > // Number, for JSON numbers, if UseNumber has been called 
>>
>> I ended up not putting this in, because there's actually no way that a 
>> caller 
>> can tell Unmarshal to UseNumber, since that requires access to 
>> decodeState. Am I 
>> not understanding what you'd like? 
>>
>> > 
>> > 
>>
http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode_...

>> > src/pkg/encoding/json/decode_test.go:196: func assertNumParsePanic(f 
>> > func(), errExpct string, t *testing.T) { 
>> > http://golang.org/doc/go_faq.html#assertions 
>> > 
>> > 
>>
http://codereview.appspot.com/6202068/diff/8001/src/pkg/encoding/json/decode_...

>> > src/pkg/encoding/json/decode_test.go:211: // Independent of Decode, 
>> > basic coverage of the accessors in Number 
>> > A more idiomatic way to write this test would be to use a table: 
>> > 
>> [SNIPPED] 
>> > 
>> > Then when the test fails you get much more information about the state 
>> > of the code.  It also makes adding a new test case a simple matter of 
>> > writing a line in the data table instead of copying and pasting however 
>> > many lines of code 
>>
>> I've adopted the table approach for the most part, based on your comments. 
>>  I'm 
>> not sure I'm understanding how to test the failures from Number.Int64() 
>> and 
>> Number.Float64() better than I have though, and I apologize for any 
>> aggravation. 
>>
>> The thing that makes it tough is that the design of Number is such that 
>> Int64() 
>> and Float64() do not return errors, which I think makes sense since we 
>> assume 
>> that bad numbers will never make it past the JSON parser in the first 
>> place. 
>> But, strconv returns errors, and I feel like it's better to panic if we 
>> actually 
>> get one, and I just want code coverage of that codepath. I'm finding it 
>> hard to 
>> concisely test panics in a table loop though without calling a specialized 
>> method (with its own defer() for making the checks), which is why you'll 
>> see 
>> what you see. 
>>
>> Thanks again for your help, and my apologies if I'm just not seeing 
>> something 
>> obvious. 
>>

[rsc, 2012-06-06 21:12:03]

Sorry for the delay. I have been busy with some other things.

http://codereview.appspot.com/6202068/diff/21001/src/pkg/encoding/json/decode.go
File src/pkg/encoding/json/decode.go (right):

http://codereview.appspot.com/6202068/diff/21001/src/pkg/encoding/json/decode...
src/pkg/encoding/json/decode.go:150: panic(err)
Panicking is not appropriate here. You know that the number has the right
syntax, but you do not know that the number is in range for a float64. 1e1000 is
valid JSON but will make ParseFloat fail. I think the error should be returned
here, making the signature (float64, error).

http://codereview.appspot.com/6202068/diff/21001/src/pkg/encoding/json/decode...
src/pkg/encoding/json/decode.go:157: if idx := strings.IndexAny(string(n),
".eE"); idx >= 0 {
If the number is 2.5, returning 2 does not seem correct here. If the number is
1e50, there is no correct answer at all. Again I think the thing to do is to
make the return values (int64, error) and just return ParseInt directly, not
trying to handle decimal points or exponents.

http://codereview.appspot.com/6202068/diff/21001/src/pkg/encoding/json/decode...
File src/pkg/encoding/json/decode_test.go (right):

http://codereview.appspot.com/6202068/diff/21001/src/pkg/encoding/json/decode...
src/pkg/encoding/json/decode_test.go:92: {in: `2`, ptr: new(Number), out:
Number("2"), useNumber: true},
Please duplicate this line and delete useNumber: true on the copy. If the
concrete data type is a Number, it should be used no matter what. useNumber
should only have an effect when unmarshalling into an interface{}.

Please also add tests

{in: `2`, ptr: new(interface{}), out: 2.0},
{in: `2`, ptr: new(interface{}), out: Number("2"), useNumber: true},

http://codereview.appspot.com/6202068/diff/21001/src/pkg/encoding/json/encode.go
File src/pkg/encoding/json/encode.go (right):

http://codereview.appspot.com/6202068/diff/21001/src/pkg/encoding/json/encode...
src/pkg/encoding/json/encode.go:39: // Floating point and integer values encode
as JSON numbers.
// Floating point, integer, and Number values encode as JSON numbers.

[jgold.bg, 2012-06-07 01:53:24]

PTAL.

Thanks for the suggestion about letting Int64()/Float64() return the strconv
errors. I originally assumed I was constrained to your 2010 interfaces, but am
much happier being able to just pass the error up to the caller:
    
    http://codereview.appspot.com/6202068/

jonathan

[jgold.bg, 2012-06-14 02:15:56]

Ping.

On 2012/06/07 01:53:24, jgold.bg wrote:
> PTAL.
> 
> Thanks for the suggestion about letting Int64()/Float64() return the strconv
> errors. I originally assumed I was constrained to your 2010 interfaces, but am
> much happier being able to just pass the error up to the caller:
>     
>     http://codereview.appspot.com/6202068/
> 
> jonathan

[rsc, 2012-06-25 15:31:10]

Looks good, sorry for the delay. Please fix these two doc nits and I'll submit.

http://codereview.appspot.com/6202068/diff/25006/src/pkg/encoding/json/decode.go
File src/pkg/encoding/json/decode.go (right):

http://codereview.appspot.com/6202068/diff/25006/src/pkg/encoding/json/decode...
src/pkg/encoding/json/decode.go:146: // Float64 returns the number as a float64
as processed by strconv.ParseFloat().
s/ as processed by strconv.ParseFloat()//

http://codereview.appspot.com/6202068/diff/25006/src/pkg/encoding/json/decode...
src/pkg/encoding/json/decode.go:151: // Int64 returns the number as an int64 as
processed by strconv.ParseInt().
s/ as processed by strconv.ParseInt()//

[jgold.bg, 2012-06-25 16:33:13]

Done (assuming your comments below were sed-style edits).

jonathan

On Mon, Jun 25, 2012 at 03:31:10PM +0000, rsc@golang.org wrote:
> Looks good, sorry for the delay. Please fix these two doc nits and I'll
> submit.
> 
> 
> 
>
http://codereview.appspot.com/6202068/diff/25006/src/pkg/encoding/json/decode.go
> File src/pkg/encoding/json/decode.go (right):
> 
>
http://codereview.appspot.com/6202068/diff/25006/src/pkg/encoding/json/decode...
> src/pkg/encoding/json/decode.go:146: // Float64 returns the number as a
> float64 as processed by strconv.ParseFloat().
> s/ as processed by strconv.ParseFloat()//
> 
>
http://codereview.appspot.com/6202068/diff/25006/src/pkg/encoding/json/decode...
> src/pkg/encoding/json/decode.go:151: // Int64 returns the number as an
> int64 as processed by strconv.ParseInt().
> s/ as processed by strconv.ParseInt()//
> 
> http://codereview.appspot.com/6202068/

[rsc, 2012-06-25 21:36:14]

*** Submitted as http://code.google.com/p/go/source/detail?r=73dfe1ecc9ac ***

encoding/json: add Number type

Number represents the actual JSON text,
preserving the precision and
formatting of the original input.

R=rsc, iant
CC=golang-dev
http://codereview.appspot.com/6202068

Committer: Russ Cox <rsc@golang.org>