Fix case sensitivity

[nikhita]

Fixes kubernetes/kubernetes#64612 (comment)
Follow-up for #282

Also see https://github.com/json-iterator/go/pull/282/files#r194283181

[nikhita]

/cc @thockin @caesarxuchao @liggitt @sttts

[codecov]

Codecov Report

Merging #285 into master will increase coverage by 0.08%.
The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master     #285      +/-   ##
==========================================
+ Coverage   81.38%   81.46%   +0.08%     
==========================================
  Files          41       41              
  Lines        4979     4980       +1     
==========================================
+ Hits         4052     4057       +5     
  Misses        807      807              
+ Partials      120      116       -4

Impacted Files	Coverage Δ
reflect_struct_decoder.go	45.68% <83.33%> (-0.63%)	⬇️
reflect_native.go	88.12% <0%> (+0.76%)	⬆️
iter_int.go	90.37% <0%> (+1.67%)	⬆️
iter_object.go	66.66% <0%> (+1.69%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7cceb6c...3830516. Read the comment docs.

[liggitt]

Do the toLower calls on lines 521 and 527 need conditionalizing as well?

[liggitt]

And if so, is there a test case that could exercise that path?

[nikhita]

Do the toLower calls on lines 521 and 527 need conditionalizing as well?

I don't think so. In fact, I don't think we even need to check for the nil condition there.

go/reflect_struct_decoder.go

Lines 516 to 519 in 7cceb6c

	fieldDecoder = decoder.fields[field]
	if fieldDecoder == nil {
	fieldDecoder = decoder.fields[strings.ToLower(field)]
	}

Here decoder is a generalStructDecoder, which is created here:

go/reflect_struct_decoder.go

Line 485 in 7cceb6c

return &generalStructDecoder{typ, fields, false}

The fields in this struct comes from:

go/reflect_struct_decoder.go

Lines 31 to 34 in 7cceb6c

	fields := map[string]*structFieldDecoder{}
	for k, binding := range bindings {
	fields[k] = binding.Decoder.(*structFieldDecoder)
	}

The *structFieldDecoder is a *StructDescriptor which is created from:

go/reflect_struct_decoder.go

Line 14 in 7cceb6c

structDescriptor := describeStruct(ctx, typ)

go/reflect_extension.go

Line 324 in 7cceb6c

func describeStruct(ctx *ctx, typ reflect2.Type) *StructDescriptor {

which creates from:

go/reflect_extension.go

Lines 388 to 391 in 7cceb6c

	func createStructDescriptor(ctx *ctx, typ reflect2.Type, bindings []*Binding, embeddedBindings []*Binding) *StructDescriptor {
	structDescriptor := &StructDescriptor{
	Type: typ,
	Fields: bindings,

From the above snippet ^, *StructDescriptor can never be nil.

Also, if the decoder is supposed to be case sensitive, the conversion to lower case is taken care of while constructing the struct decoder:

go/reflect_struct_decoder.go

Lines 35 to 39 in 7cceb6c

	for k, binding := range bindings {
	if _, found := fields[strings.ToLower(k)]; !found {
	fields[strings.ToLower(k)] = binding.Decoder.(*structFieldDecoder)
	}
	}

so the toLower() which is used below in lines 521 and 527 is not called and not needed.

[nikhita]

If this makes sense, I'll remove the lines. But I want to first double check if this is indeed correct.

[caesarxuchao]

Also, if the decoder is supposed to be case sensitive, the conversion to lower case is take care of while constructing the struct decoder:

That's my understanding as well. I agree that we should remove the lines.

[nikhita]

That's my understanding as well. I agree that we should remove the lines.

Removed. (tests were failing, investigating why)

[caesarxuchao]

Wait, if the decoder is case insensitive, we need to keep the lines, because decoder.fields[field] might return empty, and we can only find the decoder via decoder.fields[strings.ToLower(field)].

[nikhita]

Yup, added back the lines along with a new test case.

And if so, is there a test case that could exercise that path?

@liggitt the case insensitive ones are the ones that exercise the path.

[liggitt]

Yup, added back the lines along with a new test case.

+1

[caesarxuchao]

@nikhita I found the root cause of not ignoring spec.reliCAs. It's because the DeploymentSpec struct has 11 fields (field whose json name has camelCase counts as 2), so it gets the generalStructDecoder at https://github.com/json-iterator/go/blob/master/reflect_struct_decoder.go#L485. Before your PR, the generalStructDecoder always does decoder.fields[strings.ToLower(field)], and thus decodes spec.repliCAs.

I think in the test cases, we need to add a case where a struct has more than 11 fields, to exercise the generalStructDecoder path.

Btw, we might have accidentally improved the performance of jsoniter. Before our fixes, as long as a struct has camelCase field names, the decoder will always be the generalStructDecoder because it always hits

go/reflect_struct_decoder.go

Lines 368 to 372 in 7cceb6c

	fieldHash := calcHash(fieldName, ctx.caseSensitive())
	_, known := knownHash[fieldHash]
	if known {
	return &generalStructDecoder{typ, fields, false}
	}

[nikhita]

@caesarxuchao updated.

Btw, we might have accidentally improved the performance of jsoniter.

🎉

[caesarxuchao]

lgtm

[thockin]

We should add an equivalent case-sensitivity test to k8s, so that any future changes in JSON parsing catch it.

[thockin]

LGTM

By the power vested in me, I hereby merge your fix.