code review 4965049: json: fix decode bug with struct tag names with ,opts b...

json: fix decode bug with struct tag names with ,opts being ignored

When the encoder was updated to respect the ",omitempty"
struct tag options, the decoder half was never updated to know
about the new struct tag format. (the format is now an optional
name, followed by zero or more ",option" strings)

This only affected people who used ",omitempty" along with
a field name. In that case, the serialized JSON wouldn't
decode to the original value.

[bradfitz, 2011-08-26 07:05:40]

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

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

[dvyukov, 2011-08-26 07:41:56]

There can be no spaces before the comma in tags, right?

[dvyukov, 2011-08-26 07:43:23]

The decodes does case-insensitive comparison of field names, but does only
case-sensitive for tags. Is it intentional?

[dvyukov, 2011-08-26 07:54:14]

I am looking at the documentation "The "json" key in
struct field's tag value is the key name, followed by an optional comma
and options."

and at encoding code:
			if tv := f.Tag.Get("json"); tv != "" {
				ss := strings.SplitN(tv, ",", 2)
				if isValidTag(ss[0]) {
					tag = ss[0]
				}
				if len(ss) > 1 {
					// Currently the only option is omitempty,
					// so parsing is trivial.
					omitEmpty = ss[1] == "omitempty"
				}
			}

As far as I see it won't handle: 'json:"name,omitempty,letstestencoder"'
If we don't expect other options, why we do ss[1] == "omitempty"?

I just want to understand what we are supporting?

[bradfitz, 2011-08-26 07:56:41]

On Fri, Aug 26, 2011 at 11:54 AM, <dvyukov@google.com> wrote:

> I am looking at the documentation "The "json" key in
> struct field's tag value is the key name, followed by an optional comma
> and options."
>
> and at encoding code:
>                        if tv := f.Tag.Get("json"); tv != "" {
>                                ss := strings.SplitN(tv, ",", 2)
>                                if isValidTag(ss[0]) {
>                                        tag = ss[0]
>                                }
>                                if len(ss) > 1 {
>                                        // Currently the only option is
> omitempty,
>                                        // so parsing is trivial.
>                                        omitEmpty = ss[1] == "omitempty"
>                                }
>                        }
>
> As far as I see it won't handle: 'json:"name,omitempty,**letstestencoder"'
> If we don't expect other options, why we do ss[1] == "omitempty"?
>
> I just want to understand what we are supporting?


Look at the comment there.

It says:   "Currently the only option is omitempty, so parsing is trivial."

An upcoming CL (after this one), adds a new ",string" option, and then the
"parsing" does not use ==

[bradfitz, 2011-08-26 08:14:06]

On Fri, Aug 26, 2011 at 11:41 AM, <dvyukov@google.com> wrote:

> There can be no spaces before the comma in tags, right?


Correct. No spaces.

[dvyukov, 2011-08-26 08:15:18]

In either case, even if that things are issues, they are not issues with this CL
(spaces are not handled by encoding, case-sensitive comparison was here before),
so
LGTM
just want to make sure everything is under control

[bradfitz, 2011-08-26 08:15:36]

On Fri, Aug 26, 2011 at 11:43 AM, <dvyukov@google.com> wrote:

> The decodes does case-insensitive comparison of field names, but does
> only case-sensitive for tags. Is it intentional?


Yes, that's how it's always been.  The default case is automatic &
case-insensitive.  If you override a field and specify an exact name, that
name is then case-sensitive.

[bradfitz, 2011-08-26 08:27:38]

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

json: fix decode bug with struct tag names with ,opts being ignored

When the encoder was updated to respect the ",omitempty"
struct tag options, the decoder half was never updated to know
about the new struct tag format. (the format is now an optional
name, followed by zero or more ",option" strings)

This only affected people who used ",omitempty" along with
a field name. In that case, the serialized JSON wouldn't
decode to the original value.

R=golang-dev, dvyukov
CC=golang-dev
http://codereview.appspot.com/4965049

[rsc, 2011-08-26 20:42:04]

>> The decodes does case-insensitive comparison of field names, but does
>> only case-sensitive for tags. Is it intentional?
>
> Yes, that's how it's always been.  The default case is automatic &
> case-insensitive.  If you override a field and specify an exact name, that
> name is then case-sensitive.

that's probably a mistake we should fix.
it will break people's Unmarshal calls.
we should have changed it when we broke
people's Marshal calls by changing the
code to not strlwr the names before using them.

[rsc, 2011-08-26 20:42:18]

>> Yes, that's how it's always been.  The default case is automatic &
>> case-insensitive.  If you override a field and specify an exact name, that
>> name is then case-sensitive.
>
> that's probably a mistake we should fix.
> it will break people's Unmarshal calls.
> we should have changed it when we broke
> people's Marshal calls by changing the
> code to not strlwr the names before using them.

that is, the mistake is the case-insensitive default.