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

Fixing #1163 : Inconsistent Behaviour in http server : Big Integers are getting converted to float before storing thus losing their precision #1227

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

Conversation

deydebaditya
Copy link

Change description

When working with map[string]interface{} in Go, json.Marshal will automatically infer the data type for each element based on the interface{} type that we use.
However, in our case, json.Marshal is casting values incorrectly (e.g., casting integer values to float64), it may be due to Go’s internal handling of untyped constants and generic types within interface{}

To fix this, I've added an explicit check during unmarshalling of the JSON body in the request payload for int64 types.
I've also added a recursive unmarshalling to check for any such big integer values in nested JSON fields as well.

To ensure that json.Marshal preserves the original data types, I've enforced specific types in our map by using json.Number for numeric data types before marshaling again.

Lastly, instead of using a vanilla json.Unmarshal() at the end of the request map creation, the JSON decoder needs to be aware that it explicitly needs to use numbers, instead of using Go's default handling of the interface{} type.

Working

Request

image

Data set in DICE DB

image

Response from HTTP server

image

@deydebaditya deydebaditya changed the title Fixing Inconsistent Behaviour in http server : Big Integers are getting converted to float before storing thus losing their precision Fixing #1163 : Inconsistent Behaviour in http server : Big Integers are getting converted to float before storing thus losing their precision Nov 1, 2024
@AshwinKul28 AshwinKul28 self-requested a review November 1, 2024 06:58
Copy link
Contributor

@AshwinKul28 AshwinKul28 left a comment

Choose a reason for hiding this comment

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

Hello @deydebaditya thanks a lot for the changes, look great. I have left few comments please have a look on them.

Also, @lucifercr07 I remember you were doing some efforts regarding the same issue, can you please have a look at it?

internal/server/utils/redisCmdAdapter.go Outdated Show resolved Hide resolved
@@ -252,3 +254,58 @@ func isBase64Encoded(s string) bool {
}
return false
}

func unmarshalRequestBody(data []byte, v *map[string]interface{}) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since maps are already reference types, no need to send them as pointers.

Copy link
Author

Choose a reason for hiding this comment

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

@AshwinKul28 When you pass a map to a function, Go passes a reference to the map, meaning any modifications to the contents of the map itself (adding, removing, or updating keys and values) are indeed reflected outside the function.
But, in this case, I'm trying to overwrite the entire reference itself of v in the last line of the method using deoder.Decode(&v), basically trying to store the decoded map reference in the input map, so that the caller can directly access the map without a need for reassignment. So, passing by reference makes sense here IMO.

Copy link
Author

Choose a reason for hiding this comment

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

@AshwinKul28 If there are no further comments, let's merge this PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @deydebaditya sorry got occupied with other stuff. I see one lint error which is what I was talking about earlier in this thread. Since map is a ref type no need to pass it as pointer again.

Once you make that change hope linter checks will be successful

@AshwinKul28
Copy link
Contributor

Hey @deydebaditya can we work onto closing this PR? Just a one linter change is required as discussed above and we are good to go! Thanks

@deydebaditya
Copy link
Author

deydebaditya commented Nov 12, 2024 via email

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.

2 participants