code review 161060: This patch adds an ability to convert JSON-encoded data into

Decode JSON into native Go data structures

This patch adds an ability to convert JSON-encoded data into
a hierarchy of Go's native data types.

[SnakE, 2009-11-26 16:07:38]

Hello golang-dev@googlegroups.com,

I'd like you to review the following change.

[SnakE, 2009-11-26 19:43:33]

There is a cosmetic bug in nativebuilder_test.go,
TestKeySetterMustAddMapElements(): the Fatal strings mention elements 1, 2 and 3
instead of "a", "b" and "c".

Also, in the same file, both TestKeySetterMustAddMapElements() and
TestFlushMustWriteValueIntoAnUnderlyingMap() do not check elements existence
before checking their values.

I will submit these fixes for review after I hear from reviewers on other
possible problematic areas.

[agl1, 2009-11-26 22:15:21]

rsc knows the JSON code the best, but here's my drive by.

http://codereview.appspot.com/161060/diff/1001/1003
File src/pkg/json/error.go (right):

http://codereview.appspot.com/161060/diff/1001/1003#newcode17
src/pkg/json/error.go:17: func (pe *ParseError) String() string {
If you made this a method on ParseError, you wouldn't have to deal with the nil
case.

http://codereview.appspot.com/161060/diff/1001/1005
File src/pkg/json/nativebuilder.go (right):

http://codereview.appspot.com/161060/diff/1001/1005#newcode22
src/pkg/json/nativebuilder.go:22: func Decode(jsonStr string) (data interface{},
err *ParseError) {
Typically, os.Error is returned. Callers can cast to the specific type if they
need.

http://codereview.appspot.com/161060/diff/1001/1005#newcode23
src/pkg/json/nativebuilder.go:23: var jb *nativeJsonBuilder =
newNativeJsonBuilder(nil, nil);
Can use := here.

http://codereview.appspot.com/161060/diff/1001/1005#newcode28
src/pkg/json/nativebuilder.go:28: err = &ParseError{Index: errPos, Token:
errTok}
(note: if you changed to ParseError, you need to drop the & here)

[SnakE, 2009-11-26 22:53:53]

http://codereview.appspot.com/161060/diff/1001/1003
File src/pkg/json/error.go (right):

http://codereview.appspot.com/161060/diff/1001/1003#newcode17
src/pkg/json/error.go:17: func (pe *ParseError) String() string {
On 2009/11/26 22:15:21, agl1 wrote:
> If you made this a method on ParseError, you wouldn't have to deal with the
nil
> case.

Done.

http://codereview.appspot.com/161060/diff/1001/1005
File src/pkg/json/nativebuilder.go (right):

http://codereview.appspot.com/161060/diff/1001/1005#newcode22
src/pkg/json/nativebuilder.go:22: func Decode(jsonStr string) (data interface{},
err *ParseError) {
On 2009/11/26 22:15:21, agl1 wrote:
> Typically, os.Error is returned. Callers can cast to the specific type if they
> need.

Should I document the actual returned error then?  What is the standard
practice?  Right now the code is self-documenting in this respect.

http://codereview.appspot.com/161060/diff/1001/1005#newcode23
src/pkg/json/nativebuilder.go:23: var jb *nativeJsonBuilder =
newNativeJsonBuilder(nil, nil);
On 2009/11/26 22:15:21, agl1 wrote:
> Can use := here.

Done.

http://codereview.appspot.com/161060/diff/1001/1005#newcode28
src/pkg/json/nativebuilder.go:28: err = &ParseError{Index: errPos, Token:
errTok}
On 2009/11/26 22:15:21, agl1 wrote:
> (note: if you changed to ParseError, you need to drop the & here)

Thanks for the pointer.  I still don't quite understand how values are coerced
to interfaces and passed around under the hood.

[rsc, 2009-11-30 00:41:18]

Thanks for writing this.  I like this interface
much better than the current StringToJson.
This is a great start.  There are a few nits below.

In addition to the specific notes below, please remove
generic_test.go and generic.go from the package in 
this CL; clients should switch to using Decode instead.

http://codereview.appspot.com/161060/diff/1001/1003
File src/pkg/json/error.go (right):

http://codereview.appspot.com/161060/diff/1001/1003#newcode17
src/pkg/json/error.go:17: func (pe *ParseError) String() string {
On 2009/11/26 22:15:21, agl1 wrote:
> If you made this a method on ParseError, you wouldn't have to deal with the
nil
> case.

This is true but most errors typically get big enough
that it makes sense to use *ParseError instead of ParseError.
Also, it is harder for the client to mess up if you are using
*ParseError.

I apologize for the back and forth, but *ParseError is
much more common.

http://codereview.appspot.com/161060/diff/1001/1004
File src/pkg/json/error_test.go (right):

http://codereview.appspot.com/161060/diff/1001/1004#newcode9
src/pkg/json/error_test.go:9: func TestNilParseErrorToStringConversion(t
*testing.T) {
This is a weird thing to test.
nil *ParseErrors shouldn't happen anyway.

http://codereview.appspot.com/161060/diff/1001/1004#newcode13
src/pkg/json/error_test.go:13: t.Fatalf(`Error is "%s" instead of an empty
string.`, s)
s/"%s"/%q/

http://codereview.appspot.com/161060/diff/1001/1005
File src/pkg/json/nativebuilder.go (right):

http://codereview.appspot.com/161060/diff/1001/1005#newcode16
src/pkg/json/nativebuilder.go:16: // of Go data types.  The json return value
may be one of float, string,
float is definitely not good enough for the integers involved:
since it has only 24 bits of mantissa you'll lose too much
information converting everything to float.

float64 is probably good enough for this generic
representation.  That gets you 53 bits; the extra 11
you'd get from adding int64 and uint64 are probably
not worth the complexity.

http://codereview.appspot.com/161060/diff/1001/1005#newcode22
src/pkg/json/nativebuilder.go:22: func Decode(jsonStr string) (data interface{},
err *ParseError) {
On 2009/11/26 22:53:53, SnakE wrote:
> On 2009/11/26 22:15:21, agl1 wrote:
> > Typically, os.Error is returned. Callers can cast to the specific type if
they
> > need.
> 
> Should I document the actual returned error then?  What is the standard
> practice?  Right now the code is self-documenting in this respect.

It's self documenting but contains a latent bug: if a caller does something like

func MyFunc(jsonStr string) (data interface{}, err os.Error) {
    data, err = json.Decode(jsonStr);
    return;
}

then err will never be == nil.  A nil *ParseError assigned to an
interface{} (or os.Error) produces a non-nil interface value,
because for interface values, non-nil means there is some
value of some type in the interface.

See http://golang.org/doc/effective_go.html#errors for more.

http://codereview.appspot.com/161060/diff/1001/1005#newcode22
src/pkg/json/nativebuilder.go:22: func Decode(jsonStr string) (data interface{},
err *ParseError) {
There's no need for the word json in any variable
or function name in package json.  This should be
just Decode(s string).

I like the name Decode; can we rename this file to
decode.go?

http://codereview.appspot.com/161060/diff/1001/1005#newcode33
src/pkg/json/nativebuilder.go:33: type nativeJsonBuilder struct {
I suggest s/nativeJsonBuilder/decoder/

http://codereview.appspot.com/161060/diff/1001/1005#newcode39
src/pkg/json/nativebuilder.go:39: // The key into the container interface. 
Either int or string.
The general term used in the Go spec for both
of these cases is index not key, so s/containerKey/index/
(The container prefix is redundant.)

http://codereview.appspot.com/161060/diff/1001/1005#newcode43
src/pkg/json/nativebuilder.go:43: func (j *nativeJsonBuilder) Int64(i int64)	{
j.value = float(i) }
Should probably be float64; see note above.

http://codereview.appspot.com/161060/diff/1001/1005#newcode45
src/pkg/json/nativebuilder.go:45: func (j *nativeJsonBuilder) Uint64(i uint64)	{
j.value = float(i) }
Should probably be float64; see note above.

http://codereview.appspot.com/161060/diff/1001/1005#newcode47
src/pkg/json/nativebuilder.go:47: func (j *nativeJsonBuilder) Float64(f float64)
{
Should probably be float64; see note above.

http://codereview.appspot.com/161060/diff/1001/1005#newcode61
src/pkg/json/nativebuilder.go:61: // Both Elem and Key assume that the Parser is
consistent and calls only one
If the parser is buggy, anything could happen.
Can delete this comment.

http://codereview.appspot.com/161060/diff/1001/1005#newcode67
src/pkg/json/nativebuilder.go:67: arr, ok := j.value.(*vector.Vector);
s/arr/v/ -- it's a vector, not an array.

http://codereview.appspot.com/161060/diff/1001/1005#newcode79
src/pkg/json/nativebuilder.go:79: dict, ok := j.value.(map[string]interface{});
s/dict/m/ - it's a map, not a dictionary.

http://codereview.appspot.com/161060/diff/1001/1005#newcode88
src/pkg/json/nativebuilder.go:88: if j.container == nil {
The type switch will handle this case fine
(i.e. not execute either of the cases listed)
so you can delete this if statement.

http://codereview.appspot.com/161060/diff/1001/1005#newcode111
src/pkg/json/nativebuilder.go:111: func newNativeJsonBuilder(container
interface{}, key interface{}) *nativeJsonBuilder {
I like to keep constructors near the definition
of the type they construct.  Please move this up
right after the struct definition.

http://codereview.appspot.com/161060/diff/1001/1006
File src/pkg/json/nativebuilder_test.go (right):

http://codereview.appspot.com/161060/diff/1001/1006#newcode12
src/pkg/json/nativebuilder_test.go:12: func
TestInt64SetterMustProduceFloatValue(t *testing.T) {
There's no call to Decode in this file.
Also, you're testing with code here but you can write
this more succinctly by testing with data.

For example,

type decodeTest struct {
    s string;
    v interface{};
    err os.Error;
}

var decodeTests = []decodeTest{
    decodeTest{`1.234`: float64(1.234), nil},
    decodeTest{`false`: false, nil},
    decodeTest{`[1,2,3.14]`: []interface{}{1, 2, 3.14}, nil},
    // more here, to cover all the cases in the
    // current test code
}

func TestDecode(t *testing.T) {
    for _, tt := range decodeTests {
        v, err := Decode(tt.s); 
        if !reflect.DeepEqual(v, tt.v) || tt.err != err {
            t.Errorf("Decode(%#q) = %v, %v want %v, %v", v, err, tt.v, tt.err);
        }
    }
}

You can exercise all the things below by passing
appropriate strings to Decode.  That gives a good
end-to-end test.

[SnakE, 2009-11-30 05:01:19]

PTAL.  I've updated this change to include other necessary files.

http://codereview.appspot.com/161060/diff/1001/1003
File src/pkg/json/error.go (right):

http://codereview.appspot.com/161060/diff/1001/1003#newcode17
src/pkg/json/error.go:17: func (pe *ParseError) String() string {
On 2009/11/30 00:41:18, rsc wrote:
> I apologize for the back and forth, but *ParseError is
> much more common.

This is not a problem.  Done.

http://codereview.appspot.com/161060/diff/1001/1004
File src/pkg/json/error_test.go (right):

http://codereview.appspot.com/161060/diff/1001/1004#newcode9
src/pkg/json/error_test.go:9: func TestNilParseErrorToStringConversion(t
*testing.T) {
On 2009/11/30 00:41:18, rsc wrote:
> This is a weird thing to test.
> nil *ParseErrors shouldn't happen anyway.

It happened when Decode returned success as a nil *ParseError and I naively
passed it to Printf.  Now with Decode returning os.Error this won't happen
indeed.  I'm removing the whole error_test.go file.

http://codereview.appspot.com/161060/diff/1001/1005
File src/pkg/json/nativebuilder.go (right):

http://codereview.appspot.com/161060/diff/1001/1005#newcode16
src/pkg/json/nativebuilder.go:16: // of Go data types.  The json return value
may be one of float, string,
On 2009/11/30 00:41:18, rsc wrote:
> float is definitely not good enough for the integers involved:
> since it has only 24 bits of mantissa you'll lose too much
> information converting everything to float.
> 
> float64 is probably good enough for this generic
> representation.  That gets you 53 bits; the extra 11
> you'd get from adding int64 and uint64 are probably
> not worth the complexity.

Done.

I thought that JSON defined only floats for some reason.  Now I see that it only
defines "numbers."  While parsing uint64 separately seems like an overkill, I
think that keeping int64 is probably a good idea.  I can add int64 to the list
of returned types if you think it's useful.

On the second thought, it really helps to be able to process all JSON numbers
uniformly.

http://codereview.appspot.com/161060/diff/1001/1005#newcode22
src/pkg/json/nativebuilder.go:22: func Decode(jsonStr string) (data interface{},
err *ParseError) {
On 2009/11/30 00:41:18, rsc wrote:
> There's no need for the word json in any variable
> or function name in package json.  This should be
> just Decode(s string).
> 
> I like the name Decode; can we rename this file to
> decode.go?

Done.

http://codereview.appspot.com/161060/diff/1001/1005#newcode33
src/pkg/json/nativebuilder.go:33: type nativeJsonBuilder struct {
On 2009/11/30 00:41:18, rsc wrote:
> I suggest s/nativeJsonBuilder/decoder/

Done.

http://codereview.appspot.com/161060/diff/1001/1005#newcode39
src/pkg/json/nativebuilder.go:39: // The key into the container interface. 
Either int or string.
On 2009/11/30 00:41:18, rsc wrote:
> The general term used in the Go spec for both
> of these cases is index not key, so s/containerKey/index/
> (The container prefix is redundant.)

A field with the name "index" could be anything: some index of the decoder
itself for instance, or an index into the value etc.

Done, anyway.

http://codereview.appspot.com/161060/diff/1001/1005#newcode43
src/pkg/json/nativebuilder.go:43: func (j *nativeJsonBuilder) Int64(i int64)	{
j.value = float(i) }
On 2009/11/30 00:41:18, rsc wrote:
> Should probably be float64; see note above.

Done.

http://codereview.appspot.com/161060/diff/1001/1005#newcode45
src/pkg/json/nativebuilder.go:45: func (j *nativeJsonBuilder) Uint64(i uint64)	{
j.value = float(i) }
On 2009/11/30 00:41:18, rsc wrote:
> Should probably be float64; see note above.

Done.

http://codereview.appspot.com/161060/diff/1001/1005#newcode47
src/pkg/json/nativebuilder.go:47: func (j *nativeJsonBuilder) Float64(f float64)
{
On 2009/11/30 00:41:18, rsc wrote:
> Should probably be float64; see note above.

Done.

http://codereview.appspot.com/161060/diff/1001/1005#newcode61
src/pkg/json/nativebuilder.go:61: // Both Elem and Key assume that the Parser is
consistent and calls only one
On 2009/11/30 00:41:18, rsc wrote:
> If the parser is buggy, anything could happen.
> Can delete this comment.

Done.

http://codereview.appspot.com/161060/diff/1001/1005#newcode67
src/pkg/json/nativebuilder.go:67: arr, ok := j.value.(*vector.Vector);
On 2009/11/30 00:41:18, rsc wrote:
> s/arr/v/ -- it's a vector, not an array.

Done.

http://codereview.appspot.com/161060/diff/1001/1005#newcode79
src/pkg/json/nativebuilder.go:79: dict, ok := j.value.(map[string]interface{});
On 2009/11/30 00:41:18, rsc wrote:
> s/dict/m/ - it's a map, not a dictionary.

Done.

http://codereview.appspot.com/161060/diff/1001/1005#newcode88
src/pkg/json/nativebuilder.go:88: if j.container == nil {
On 2009/11/30 00:41:18, rsc wrote:
> The type switch will handle this case fine
> (i.e. not execute either of the cases listed)
> so you can delete this if statement.

Done.

http://codereview.appspot.com/161060/diff/1001/1005#newcode111
src/pkg/json/nativebuilder.go:111: func newNativeJsonBuilder(container
interface{}, key interface{}) *nativeJsonBuilder {
On 2009/11/30 00:41:18, rsc wrote:
> I like to keep constructors near the definition
> of the type they construct.  Please move this up
> right after the struct definition.

Done.

http://codereview.appspot.com/161060/diff/1001/1006
File src/pkg/json/nativebuilder_test.go (right):

http://codereview.appspot.com/161060/diff/1001/1006#newcode12
src/pkg/json/nativebuilder_test.go:12: func
TestInt64SetterMustProduceFloatValue(t *testing.T) {
On 2009/11/30 00:41:18, rsc wrote:
> There's no call to Decode in this file.
>
> [...] 
> 
> You can exercise all the things below by passing
> appropriate strings to Decode.  That gives a good
> end-to-end test.

These were supposed to be unit tests, to make sure that nativeJsonBuilder works
in isolation from Parser and other components.  I've used your tips to condense
them as much as possible.  I've also added a test for Decode which works exactly
as you suggest and runs all the tests which were in the original
generic_test.go.

I really believe that unit tests add value.  I can remove them if you think
they're redundant though.

[rsc, 2009-11-30 19:17:52]

a few more nits

http://andi.latest.codereview.appspot.com/161060/diff/27/2005
File src/pkg/expvar/expvar_test.go (right):

http://andi.latest.codereview.appspot.com/161060/diff/27/2005#newcode68
src/pkg/expvar/expvar_test.go:68: switch m := j.(type) {
Please put this back into an if so that the error case
can be picked off nearer the test, and do the same
in the other parts.  

m, ok := j.(map[string]interface{});
if !ok {
    t.Error("...");
}
red, ok := ...
if !ok {
    t.Error("...");
}

In rewriting this you've swapped all the errors
to the bottom, which makes them harder to
read -- you're 4 levels down the stack now and
have to remember all the things that might
have gone wrong as you unwind.
(It's telling that the rewrite turned 13 lines into 21.)

See also the second half of this section
http://golang.org/doc/effective_go.html#if

http://andi.latest.codereview.appspot.com/161060/diff/27/2006
File src/pkg/json/Makefile (right):

http://andi.latest.codereview.appspot.com/161060/diff/27/2006#newcode9
src/pkg/json/Makefile:9: error.go\
please move down 1 line
so that the list remains sorted

http://andi.latest.codereview.appspot.com/161060/diff/27/2008
File src/pkg/json/decode_test.go (right):

http://andi.latest.codereview.appspot.com/161060/diff/27/2008#newcode13
src/pkg/json/decode_test.go:13: func TestInt64SetterMustProduceFloat64Value(t
*testing.T) {
I'm willing to keep these tests but the 
convention in Go is to use names, not sentences.

TestDecodeInt64

http://andi.latest.codereview.appspot.com/161060/diff/27/2008#newcode19
src/pkg/json/decode_test.go:19: func TestUint64SetterMustProduceFloatValue(t
*testing.T) {
TestDecodeUint64 etc

http://andi.latest.codereview.appspot.com/161060/diff/27/2008#newcode49
src/pkg/json/decode_test.go:49: func TestArraySetterMustProduceEmptyArray(t
*testing.T) {
TestDecodeEmptyArray

http://andi.latest.codereview.appspot.com/161060/diff/27/2008#newcode55
src/pkg/json/decode_test.go:55: func TestMapSetterMustProduceEmptyMap(t
*testing.T) {
TestDecodeEmptyMap

http://andi.latest.codereview.appspot.com/161060/diff/27/2008#newcode61
src/pkg/json/decode_test.go:61: func
TestFlushMustWriteValueIntoAnUnderlyingArray(t *testing.T) {
TestDecodeArray

http://andi.latest.codereview.appspot.com/161060/diff/27/2008#newcode69
src/pkg/json/decode_test.go:69: func
TestFlushMustWriteValueIntoAnUnderlyingMap(t *testing.T) {
TestDecodeMap

http://andi.latest.codereview.appspot.com/161060/diff/27/2008#newcode82
src/pkg/json/decode_test.go:82: func TestElemSetterMustAddArrayElements(t
*testing.T) {
TestDecodeElem

http://andi.latest.codereview.appspot.com/161060/diff/27/2008#newcode91
src/pkg/json/decode_test.go:91: func TestKeySetterMustAddMapElements(t
*testing.T) {
TestDecodeKey

http://andi.latest.codereview.appspot.com/161060/diff/27/2008#newcode102
src/pkg/json/decode_test.go:102: t.Fatalf("Got %#v of type %T instead of %#v of
type %T.", results, results, expected, expected)
the usual formatting is

t.Fatalf("have %T(%#v) want %T(%#v)", have, have, want, want)

http://andi.latest.codereview.appspot.com/161060/diff/27/2008#newcode127
src/pkg/json/decode_test.go:127: func TestDecodeEndToEnd(t *testing.T) {
TestDecode

http://andi.latest.codereview.appspot.com/161060/diff/27/2008#newcode130
src/pkg/json/decode_test.go:130: if err != nil {
it's worth printing both return values in both cases.
and the error message usually uses go syntax instead of
english:

if err != nil || !reflect.DeepEqual(val, test.r) {
    t.Errorf("Decode(%#q) = %v, %v want %v, nil", test.s, val, err, test.r);
}

[SnakE, 2009-11-30 21:46:35]

PTAL

http://andi.latest.codereview.appspot.com/161060/diff/27/2005
File src/pkg/expvar/expvar_test.go (right):

http://andi.latest.codereview.appspot.com/161060/diff/27/2005#newcode68
src/pkg/expvar/expvar_test.go:68: switch m := j.(type) {
On 2009/11/30 19:17:52, rsc wrote:
> Please put this back into an if so that the error case
> can be picked off nearer the test, and do the same
> in the other parts.  

Done.  I reworked my changes to be as close to the original test as possible.

> (It's telling that the rewrite turned 13 lines into 21.)

Please note that my 21-line test also checked for "red" presence in the map
while the original test would simply crash if it wasn't there.

http://andi.latest.codereview.appspot.com/161060/diff/27/2006
File src/pkg/json/Makefile (right):

http://andi.latest.codereview.appspot.com/161060/diff/27/2006#newcode9
src/pkg/json/Makefile:9: error.go\
On 2009/11/30 19:17:52, rsc wrote:
> please move down 1 line
> so that the list remains sorted

Done.

http://andi.latest.codereview.appspot.com/161060/diff/27/2008
File src/pkg/json/decode_test.go (right):

http://andi.latest.codereview.appspot.com/161060/diff/27/2008#newcode13
src/pkg/json/decode_test.go:13: func TestInt64SetterMustProduceFloat64Value(t
*testing.T) {
On 2009/11/30 19:17:52, rsc wrote:
> I'm willing to keep these tests but the 
> convention in Go is to use names, not sentences.
> 
> TestDecodeInt64

Done for all the test methods.

http://andi.latest.codereview.appspot.com/161060/diff/27/2008#newcode102
src/pkg/json/decode_test.go:102: t.Fatalf("Got %#v of type %T instead of %#v of
type %T.", results, results, expected, expected)
On 2009/11/30 19:17:52, rsc wrote:
> the usual formatting is
> 
> t.Fatalf("have %T(%#v) want %T(%#v)", have, have, want, want)

I had this before.  This occasionally led to values like "[]interface {
}([]interface { }{})" and "map[string] interface { }(map[string] interface {
}{})".

Done, anyway.

http://andi.latest.codereview.appspot.com/161060/diff/27/2008#newcode130
src/pkg/json/decode_test.go:130: if err != nil {
On 2009/11/30 19:17:52, rsc wrote:
> it's worth printing both return values in both cases.
> and the error message usually uses go syntax instead of
> english:
> 
> if err != nil || !reflect.DeepEqual(val, test.r) {
>     t.Errorf("Decode(%#q) = %v, %v want %v, nil", test.s, val, err, test.r);
> }

I had sort of this before as well.  Occasionally it gave errors looking like
"Decode(`1`) = 1, nil want 1, nil" with the reason of the error being wrong type
of "1".  Seems like right now there is no way in Go to print a value and its
type consistently and good-looking.

Done, anyway.

[rsc, 2009-11-30 21:50:17]

> I had sort of this before as well.  Occasionally it gave errors looking
> like "Decode(`1`) = 1, nil want 1, nil" with the reason of the error
> being wrong type of "1".  Seems like right now there is no way in Go to
> print a value and its type consistently and good-looking.

I think it would be fine (in a new CL) to work on %#v so that
it does the right thing.

[rsc, 2009-11-30 21:53:28]

LGTM
thanks

[rsc, 2009-11-30 21:55:13]

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

json: Decode into native Go data structures

This patch adds an ability to convert JSON-encoded data into
a hierarchy of Go's native data types.

R=rsc
CC=golang-dev
http://codereview.appspot.com/161060

Committer: Russ Cox <rsc@golang.org>