Skip to content
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.

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

Fix bug in function decodeReflectJSONStruct #306

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

NicolasMahe
Copy link

@NicolasMahe NicolasMahe commented Feb 16, 2020

This PR fixes a bug in the function decodeReflectJSONStruct.

To decode each field of the struct, this function is calling decodeReflectJSON. The issue was instead of using each field.FieldOptions, it was passing its parameter fopts FieldOptions:

err = cdc.decodeReflectJSON(valueBytes, finfo, frv, fopts)

The binary version decodeReflectBinaryStruct is already using each field.FieldOptions, so I just apply the same logic to the json version.

_n, err = cdc.decodeReflectBinary(bz, finfo, frv, field.FieldOptions, false)

I also add a new Unsafe struct to json_test.go to unit test the issue. Without the fix, the tests are failing.


Here is the link to the failed CI test before the fix: https://app.circleci.com/jobs/github/tendermint/go-amino/670

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

json-decode.go Outdated
@@ -362,7 +362,7 @@ func (cdc *Codec) decodeReflectJSONSlice(bz []byte, info *TypeInfo, rv reflect.V
}

// CONTRACT: rv.CanAddr() is true.
func (cdc *Codec) decodeReflectJSONStruct(bz []byte, info *TypeInfo, rv reflect.Value, fopts FieldOptions) (err error) {
func (cdc *Codec) decodeReflectJSONStruct(bz []byte, info *TypeInfo, rv reflect.Value, _ FieldOptions) (err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to remove the last parameter? don't see a reason to keep it

Copy link
Author

Choose a reason for hiding this comment

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

yes it's possible. i didn't remove it because the binary version has it:

go-amino/binary-decode.go

Lines 763 to 764 in 59d50ef

func (cdc *Codec) decodeReflectBinaryStruct(bz []byte, info *TypeInfo, rv reflect.Value,
_ FieldOptions, bare bool) (n int, err error) {

Copy link
Author

Choose a reason for hiding this comment

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

updated 👍

@NicolasMahe NicolasMahe requested a review from melekes February 18, 2020 06:55
@jacquesvcritien
Copy link

Are there plans for this to be merged please?

@melekes
Copy link
Contributor

melekes commented Mar 14, 2023

Are there plans for this to be merged please?

I don't think this repo is being maintained. Better to fork and continue from there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants