code review 4918051: json: add struct tag option to wrap literals in strings

json: add struct tag option to wrap literals in strings

Since JavaScript doesn't have [u]int64 types, some JSON APIs
encode such types as strings to avoid losing precision.

This adds a new struct tag option ",string" to cause
fields to be wrapped in JSON strings on encoding
and unwrapped from strings when decoding.

[bradfitz, 2011-08-24 09:42:37]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2011-08-24 09:47:03]

This one's for discussion.

I anticipate the JSON != JavaScript argument already, but let me preemptive
counter by noting that there are real systems that need to be interoperated
with here.

Custom integer type aliases to implement MarshalJSON and UnmarshalJSON are
unnatural and bleed into weird places.

I realize this CL doesn't address the marshaling case, which I also need. (I
need some integer fields, generally int64s, to always be encoded as strings,
I believe regardless of their size)

So maybe a structtag like `json:"strconv"` on int types would work?


On Wed, Aug 24, 2011 at 1:42 PM, <bradfitz@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> json: unmarshal numeric strings into integers types
>
> JSON APIs typically encode large integers as strings,
> since JavaScript has no [u]int64 types (just a float64)
>
> This attempts to strconv strings when decoding into
> an integer type.
>
> I started down the road of making this a new struct
> tag option (`json:"strconv"`) but seemed unnecessarily
> complicated (both in docs + code), when I considered
> this might be a sane default.
>
> Please review this at
http://codereview.appspot.com/**4918051/<http://codereview.appspot.com/4918051/>
>
> Affected files:
>  M src/pkg/json/decode.go
>  M src/pkg/json/decode_test.go
>
>
> Index: src/pkg/json/decode.go
> ==============================**==============================**=======
> --- a/src/pkg/json/decode.go
> +++ b/src/pkg/json/decode.go
> @@ -592,6 +592,20 @@
>                switch v.Kind() {
>                default:
>                        d.saveError(&**UnmarshalTypeError{"string",
> v.Type()})
> +               case reflect.Int, reflect.Int8, reflect.Int16,
> reflect.Int32, reflect.Int64:
> +                       n, err := strconv.Atoi64(string(s))
> +                       if err != nil || v.OverflowInt(n) {
> +                               d.saveError(&**UnmarshalTypeError{"string
> " + string(s), v.Type()})
> +                               break
> +                       }
> +                       v.SetInt(n)
> +               case reflect.Uint, reflect.Uint8, reflect.Uint16,
> reflect.Uint32, reflect.Uint64, reflect.Uintptr:
> +                       n, err := strconv.Atoui64(string(s))
> +                       if err != nil || v.OverflowUint(n) {
> +                               d.saveError(&**UnmarshalTypeError{"string
> " + string(s), v.Type()})
> +                               break
> +                       }
> +                       v.SetUint(n)
>                case reflect.Slice:
>                        if v.Type() != byteSliceType {
>                                d.saveError(&**UnmarshalTypeError{"string",
> v.Type()})
> Index: src/pkg/json/decode_test.go
> ==============================**==============================**=======
> --- a/src/pkg/json/decode_test.go
> +++ b/src/pkg/json/decode_test.go
> @@ -87,6 +87,29 @@
>        {`[{"T":false}]`, &umslice, umslice, nil},
>        {`[{"T":false}]`, &umslicep, &umslice, nil},
>        {`{"M":{"T":false}}`, &umstruct, umstruct, nil},
> +
> +       // strconv tests (strings unmarshalled into integer fields)
> +       {numStringIndent, new(numString), numStringValue, nil},
> +       {`{"int8s": "127"}`, new(numString), numString{Int8s: 127}, nil},
> +       {`{"int8s": "128"}`, new(numString), numString{},
> &UnmarshalTypeError{"string 128", reflect.TypeOf(int8(0))}},
> +       {`{"uints": "-1"}`, new(numString), numString{},
> &UnmarshalTypeError{"string -1", reflect.TypeOf(uint(0))}},
> +}
> +
> +var numStringIndent = `{
> +       "uint64s": "36028797018963968",
> +       "int64s": "2305843009213693952"
> +}`
> +
> +type numString struct {
> +       Uint64s uint64
> +       Int64s  int64
> +       Int8s   int8
> +       Uints   uint
> +}
> +
> +var numStringValue = numString{
> +       Uint64s: 36028797018963968,
> +       Int64s:  2305843009213693952,
>  }
>
>  func TestMarshal(t *testing.T) {
>
>
>

[rsc, 2011-08-24 15:06:12]

I would like to keep the json core clean
instead of full of heuristics.  There are lots
of things that come up when dealing with
real world json but those things are often
different in each case.  If at all possible,
I'd like to use the current MarshalJSON
and UnmarshalJSON callouts.

Here's an alternative for the case you're in.
The json package could define some helper types:

// An IntString is an int that uses a quoted decimal string
// as its JSON wire representation.
type IntString int

and so on for Int64String, UintString, Uint64String.

Then your code can just use those in the struct fields.

Russ

[gustavo_niemeyer.net, 2011-08-24 17:19:31]

> I'd like to use the current MarshalJSON
> and UnmarshalJSON callouts.

As a minor inconvenience, this forces the field to be declared as a
pointer right now.  This is fixable, though, now that reflect is able
to produce pointers out of values. I've addressed the problem in
gobson, and I hope to introduce similar logic in json in the future if
no one gets there first.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[rsc, 2011-08-24 17:31:52]

I thought I already fixed that.
On Aug 24, 2011 1:19 PM, "Gustavo Niemeyer" <gustavo@niemeyer.net> wrote:
>> I'd like to use the current MarshalJSON
>> and UnmarshalJSON callouts.
>
> As a minor inconvenience, this forces the field to be declared as a
> pointer right now. This is fixable, though, now that reflect is able
> to produce pointers out of values. I've addressed the problem in
> gobson, and I hope to introduce similar logic in json in the future if
> no one gets there first.
>
> --
> Gustavo Niemeyer
> http://niemeyer.net
> http://niemeyer.net/plus
> http://niemeyer.net/twitter
> http://niemeyer.net/blog
>
> -- I never filed a patent.

[gustavo_niemeyer.net, 2011-08-24 17:44:18]

> I thought I already fixed that.

changeset:   9370:9009fa15f80e
user:        Russ Cox <rsc@golang.org>
date:        Wed Aug 10 09:26:51 2011 -0400
summary:     json: fix []unmarshaler case

Brilliant, thanks! I missed what you meant by that sentence entirely.

-- 
Gustavo Niemeyer
http://niemeyer.net
http://niemeyer.net/plus
http://niemeyer.net/twitter
http://niemeyer.net/blog

-- I never filed a patent.

[bradfitz, 2011-08-24 22:05:37]

I could have defined those types in my own package, too.  My "bleed out to
weird places" reference was that I think it's a bit gross for callers to
have to cast the field selector expression back to a plain int64 everywhere
they access it. Feels like a weird internal detail to expose.

I'd rather give callers a pure struct with sane types and keep the
serialization metadata elsewhere... like in the json struct tag.

But if you're against using the struct tag for this, I could also just
define twice as many types, one with Int64String types for marshalling, and
one with int64, and convert between them myself.  Feels like an unnecessary
amount of work, though.

Is there a less offensive hook in the json package you could imagine adding?


On Wed, Aug 24, 2011 at 7:06 PM, Russ Cox <rsc@golang.org> wrote:

> I would like to keep the json core clean
> instead of full of heuristics.  There are lots
> of things that come up when dealing with
> real world json but those things are often
> different in each case.  If at all possible,
> I'd like to use the current MarshalJSON
> and UnmarshalJSON callouts.
>
> Here's an alternative for the case you're in.
> The json package could define some helper types:
>
> // An IntString is an int that uses a quoted decimal string
> // as its JSON wire representation.
> type IntString int
>
> and so on for Int64String, UintString, Uint64String.
>
> Then your code can just use those in the struct fields.
>
> Russ
>

[rsc, 2011-08-25 02:40:54]

> I'd rather give callers a pure struct with sane types and keep the
> serialization metadata elsewhere... like in the json struct tag.
> But if you're against using the struct tag for this, I could also just
> define twice as many types, one with Int64String types for marshalling, and
> one with int64, and convert between them myself.  Feels like an unnecessary
> amount of work, though.
> Is there a less offensive hook in the json package you could imagine adding?

I am just worried about adding a new tag every month.
At some point it stops being useful.

Let's say that ",string" means take the normal JSON representation
and then encode it as a JSON string, and similarly during
unmarshal.  That's without sniffing: if it says ",string" you
have to have "123" not 123 during unmarshal.
That seems okay.

Russ

[bradfitz, 2011-08-26 09:38:20]

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

Please take another look.

[bradfitz, 2011-08-26 09:42:50]

Russ,

Here's my first attempt at implementing your earlier counter-proposal of a
",string" option.

I wouldn't be surprised if you'd like it done differently, but this was the
least invasive method I could figure out.  Suggestions welcome.

- Brad


On Fri, Aug 26, 2011 at 1:38 PM, <bradfitz@golang.org> wrote:

> Hello golang-dev@googlegroups.com, rsc@golang.org, gustavo@niemeyer.net
> (cc: golang-dev@googlegroups.com),
>
> Please take another look.
>
>
>
http://codereview.appspot.com/**4918051/<http://codereview.appspot.com/4918051/>
>

[rsc, 2011-08-26 20:35:44]

LGTM

Nice how it fit in.

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/decode.go
File src/pkg/json/decode.go (right):

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/decode.go#newco...
src/pkg/json/decode.go:529: var s string
Please put this s in the d struct so that it
gets reused on each iteration.

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/encode.go
File src/pkg/json/encode.go (left):

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/encode.go#oldco...
src/pkg/json/encode.go:232: // copy JSON into buffer, checking validity.
Why did you delete the comment?  It does do that.

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/encode.go
File src/pkg/json/encode.go (right):

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/encode.go#newco...
src/pkg/json/encode.go:61: // In addition to "omitempty", the other recognized
option is
// The "string" option signals that a field is stored as JSON inside a
// JSON-encoded string.  This extra level of encoding is sometimes
// used when communicating with JavaScript programs:
//
//    Int64String int64 `json:",string"`

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/encode.go#newco...
src/pkg/json/encode.go:253: var writeString func(string)
writeString := (*encodeState).WriteString
if quoted {
    writeString = (*encodeState).string
}

Then you have to say writeString(e, ...) below but you
avoid the allocation.

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/tags.go
File src/pkg/json/tags.go (right):

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/tags.go#newcode28
src/pkg/json/tags.go:28: if len(o) == 0 {
This implementation will fail if you search for foo in "foobar,foo".
It won't come up today but it will be surprising when it does.

This is slower but handles that: 

s := string(o)
for s != "" {
    var next string
    i := strings.Index(s, ",")
    if i >= 0 {
        s, next = s[:i], s[i+1:]
    }
    if s == optionName {
        return true
    }
    s = next
}
return false

In practice it is unlikely to be noticeably slower
since normally s == "".

[bradfitz, 2011-08-29 19:06:12]

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/decode.go
File src/pkg/json/decode.go (right):

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/decode.go#newco...
src/pkg/json/decode.go:529: var s string
On 2011/08/26 20:35:44, rsc wrote:
> Please put this s in the d struct so that it
> gets reused on each iteration.

Ah, I had figured it didn't matter because it's an immutable string, but I guess
the StringHeader part can be re-used.

Done.

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/encode.go
File src/pkg/json/encode.go (left):

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/encode.go#oldco...
src/pkg/json/encode.go:232: // copy JSON into buffer, checking validity.
On 2011/08/26 20:35:44, rsc wrote:
> Why did you delete the comment?  It does do that.

Whoops, on accident?  Restored.

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/encode.go
File src/pkg/json/encode.go (right):

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/encode.go#newco...
src/pkg/json/encode.go:61: // In addition to "omitempty", the other recognized
option is
On 2011/08/26 20:35:44, rsc wrote:
> // The "string" option signals that a field is stored as JSON inside a
> // JSON-encoded string.  This extra level of encoding is sometimes
> // used when communicating with JavaScript programs:
> //
> //    Int64String int64 `json:",string"`
> 

Done.

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/encode.go#newco...
src/pkg/json/encode.go:253: var writeString func(string)
On 2011/08/26 20:35:44, rsc wrote:
> writeString := (*encodeState).WriteString
> if quoted {
>     writeString = (*encodeState).string
> }
> 
> Then you have to say writeString(e, ...) below but you
> avoid the allocation.

That's how I originally had it, but I hoped that the recent work on escape
analysis did or would make the two equivalent.

Not true?

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/tags.go
File src/pkg/json/tags.go (right):

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/tags.go#newcode28
src/pkg/json/tags.go:28: if len(o) == 0 {
On 2011/08/26 20:35:44, rsc wrote:
> This implementation will fail if you search for foo in "foobar,foo".
> It won't come up today but it will be surprising when it does.
> 
> This is slower but handles that: 
> 
> s := string(o)
> for s != "" {
>     var next string
>     i := strings.Index(s, ",")
>     if i >= 0 {
>         s, next = s[:i], s[i+1:]
>     }
>     if s == optionName {
>         return true
>     }
>     s = next
> }
> return false
> 
> In practice it is unlikely to be noticeably slower
> since normally s == "".
> 

Done.

Added some tests too.

[bradfitz, 2011-08-29 19:06:14]

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

Please take another look.

[rsc, 2011-08-29 19:23:37]

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/encode.go
File src/pkg/json/encode.go (right):

http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/encode.go#newco...
src/pkg/json/encode.go:253: var writeString func(string)
On 2011/08/29 19:06:12, bradfitz wrote:
> On 2011/08/26 20:35:44, rsc wrote:
> > writeString := (*encodeState).WriteString
> > if quoted {
> >     writeString = (*encodeState).string
> > }
> > 
> > Then you have to say writeString(e, ...) below but you
> > avoid the allocation.
> 
> That's how I originally had it, but I hoped that the recent work on escape
> analysis did or would make the two equivalent.
> 
> Not true?

Some day it might be true but it's still cleaner
not to have the func literals cluttering up the
code.  There's already a name for the function
you want: (*encodeState).WriteString.

[bradfitz, 2011-08-29 19:32:40]

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

Please take another look.

[bradfitz, 2011-08-29 19:32:59]

On Mon, Aug 29, 2011 at 12:23 PM, <rsc@golang.org> wrote:

>
> http://codereview.appspot.com/**4918051/diff/17001/src/pkg/**
>
json/encode.go<http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/encode.go>
> File src/pkg/json/encode.go (right):
>
> http://codereview.appspot.com/**4918051/diff/17001/src/pkg/**
>
json/encode.go#newcode253<http://codereview.appspot.com/4918051/diff/17001/src/pkg/json/encode.go#newcode253>
> src/pkg/json/encode.go:253: var writeString func(string)
> On 2011/08/29 19:06:12, bradfitz wrote:
>
>> On 2011/08/26 20:35:44, rsc wrote:
>> > writeString := (*encodeState).WriteString
>> > if quoted {
>> >     writeString = (*encodeState).string
>> > }
>> >
>> > Then you have to say writeString(e, ...) below but you
>> > avoid the allocation.
>>
>
>  That's how I originally had it, but I hoped that the recent work on
>>
> escape
>
>> analysis did or would make the two equivalent.
>>
>
>  Not true?
>>
>
> Some day it might be true but it's still cleaner
> not to have the func literals cluttering up the
> code.  There's already a name for the function
> you want: (*encodeState).WriteString.
>

Done.

Changed encodeState.string() to return a compatible return type.

[rsc, 2011-08-29 19:37:42]

LGTM

[bradfitz, 2011-08-29 19:46:37]

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

json: add struct tag option to wrap literals in strings

Since JavaScript doesn't have [u]int64 types, some JSON APIs
encode such types as strings to avoid losing precision.

This adds a new struct tag option ",string" to cause
fields to be wrapped in JSON strings on encoding
and unwrapped from strings when decoding.

R=rsc, gustavo
CC=golang-dev
http://codereview.appspot.com/4918051