Skip to content

Fix case sensitivity #285

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 12, 2018
Merged

Fix case sensitivity #285

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
129 changes: 128 additions & 1 deletion api_tests/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@ package test

import (
"encoding/json"
"testing"

"github.com/json-iterator/go"
"github.com/stretchr/testify/require"
"testing"
)

func Test_use_number_for_unmarshal(t *testing.T) {
Expand Down Expand Up @@ -45,3 +46,129 @@ func Test_read_large_number_as_interface(t *testing.T) {
should.Nil(err)
should.Equal(`123456789123456789123456789`, output)
}

type caseSensitiveStruct struct {
A string `json:"a"`
B string `json:"b,omitempty"`
C *C `json:"C,omitempty"`
}

type C struct {
D int64 `json:"D,omitempty"`
E *E `json:"e,omitempty"`
}

type E struct {
F string `json:"F,omitempty"`
}

func Test_CaseSensitive(t *testing.T) {
should := require.New(t)

testCases := []struct {
input string
expectedOutput string
caseSensitive bool
}{
{
input: `{"A":"foo","B":"bar"}`,
expectedOutput: `{"a":"foo","b":"bar"}`,
caseSensitive: false,
},
{
input: `{"a":"foo","b":"bar"}`,
expectedOutput: `{"a":"foo","b":"bar"}`,
caseSensitive: true,
},
{
input: `{"a":"foo","b":"bar","C":{"D":10}}`,
expectedOutput: `{"a":"foo","b":"bar","C":{"D":10}}`,
caseSensitive: true,
},
{
input: `{"a":"foo","B":"bar","c":{"d":10}}`,
expectedOutput: `{"a":"foo"}`,
caseSensitive: true,
},
{
input: `{"a":"foo","C":{"d":10}}`,
expectedOutput: `{"a":"foo","C":{}}`,
caseSensitive: true,
},
{
input: `{"a":"foo","C":{"D":10,"e":{"f":"baz"}}}`,
expectedOutput: `{"a":"foo","C":{"D":10,"e":{}}}`,
caseSensitive: true,
},
{
input: `{"a":"foo","C":{"D":10,"e":{"F":"baz"}}}`,
expectedOutput: `{"a":"foo","C":{"D":10,"e":{"F":"baz"}}}`,
caseSensitive: true,
},
{
input: `{"A":"foo","c":{"d":10,"E":{"f":"baz"}}}`,
expectedOutput: `{"a":"foo","C":{"D":10,"e":{"F":"baz"}}}`,
caseSensitive: false,
},
}

for _, tc := range testCases {
val := caseSensitiveStruct{}
err := jsoniter.Config{CaseSensitive: tc.caseSensitive}.Froze().UnmarshalFromString(tc.input, &val)
should.Nil(err)

output, err := jsoniter.MarshalToString(val)
should.Nil(err)
should.Equal(tc.expectedOutput, output)
}
}

type structWithElevenFields struct {
A string `json:"A,omitempty"`
B string `json:"B,omitempty"`
C string `json:"C,omitempty"`
D string `json:"d,omitempty"`
E string `json:"e,omitempty"`
F string `json:"f,omitempty"`
G string `json:"g,omitempty"`
H string `json:"h,omitempty"`
I string `json:"i,omitempty"`
J string `json:"j,omitempty"`
K string `json:"k,omitempty"`
}

func Test_CaseSensitive_MoreThanTenFields(t *testing.T) {
should := require.New(t)

testCases := []struct {
input string
expectedOutput string
caseSensitive bool
}{
{
input: `{"A":"1","B":"2","C":"3","d":"4","e":"5","f":"6","g":"7","h":"8","i":"9","j":"10","k":"11"}`,
expectedOutput: `{"A":"1","B":"2","C":"3","d":"4","e":"5","f":"6","g":"7","h":"8","i":"9","j":"10","k":"11"}`,
caseSensitive: true,
},
{
input: `{"a":"1","b":"2","c":"3","D":"4","E":"5","F":"6"}`,
expectedOutput: `{"A":"1","B":"2","C":"3","d":"4","e":"5","f":"6"}`,
caseSensitive: false,
},
{
input: `{"A":"1","b":"2","d":"4","E":"5"}`,
expectedOutput: `{"A":"1","d":"4"}`,
caseSensitive: true,
},
}

for _, tc := range testCases {
val := structWithElevenFields{}
err := jsoniter.Config{CaseSensitive: tc.caseSensitive}.Froze().UnmarshalFromString(tc.input, &val)
should.Nil(err)

output, err := jsoniter.MarshalToString(val)
should.Nil(err)
should.Equal(tc.expectedOutput, output)
}
}
15 changes: 10 additions & 5 deletions reflect_struct_decoder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,11 +32,15 @@ func decoderOfStruct(ctx *ctx, typ reflect2.Type) ValDecoder {
for k, binding := range bindings {
fields[k] = binding.Decoder.(*structFieldDecoder)
}
for k, binding := range bindings {
if _, found := fields[strings.ToLower(k)]; !found {
fields[strings.ToLower(k)] = binding.Decoder.(*structFieldDecoder)

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

@liggitt liggitt Jun 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@nikhita nikhita Jun 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@nikhita nikhita Jun 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@nikhita nikhita Jun 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Removed. (tests were failing, investigating why)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

+1

}
}
}

return createStructDecoder(ctx, typ, fields)
}

Expand All @@ -47,6 +51,7 @@ func createStructDecoder(ctx *ctx, typ reflect2.Type, fields map[string]*structF
knownHash := map[int64]struct{}{
0: {},
}

switch len(fields) {
case 0:
return &skipObjectDecoder{typ}
Expand Down Expand Up @@ -514,13 +519,13 @@ func (decoder *generalStructDecoder) decodeOneField(ptr unsafe.Pointer, iter *It
fieldBytes := iter.ReadStringAsSlice()
field = *(*string)(unsafe.Pointer(&fieldBytes))
fieldDecoder = decoder.fields[field]
if fieldDecoder == nil {
if fieldDecoder == nil && !iter.cfg.caseSensitive {
fieldDecoder = decoder.fields[strings.ToLower(field)]
}
} else {
field = iter.ReadString()
fieldDecoder = decoder.fields[field]
if fieldDecoder == nil {
if fieldDecoder == nil && !iter.cfg.caseSensitive {
fieldDecoder = decoder.fields[strings.ToLower(field)]
}
}
Expand Down