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: Serialise nested protobuf structs correctly #674

Merged
merged 1 commit into from
May 5, 2022
Merged

fix: Serialise nested protobuf structs correctly #674

merged 1 commit into from
May 5, 2022

Conversation

jvallesm
Copy link
Contributor

@jvallesm jvallesm commented May 5, 2022

This PR addresses #671

When a notification request is received via the gRPC server, the Data field in the request is parsed from a *structpb.Struct into a map[string]interface{}. This map will eventually be serialised with json.Marshal (here or here).

Using fmt.Sprintf("%v", v) to parse each value in the struct caused nested structs to be serialised as strings, e.g.:

{"popup":"map[body:how are you? title:hey]","source":"poc.pushnotif"}

Using structpb.AsMap directly yields the correct serialisation:

{"popup":{"body":"how are you?","title":"hey"},"source":"poc.pushnotif"}

You can see an example in this code snippet.

Using `fmt.Sprintf("%v", v)` to parse each value in the struct caused
nested structs to be serialised as strings, e.g.:

  {"popup":"map[body:how are you? title:hey]","source":"poc.pushnotif"}

Using `structpb.AsMap` directly yields the correct serialisation:

  {"popup":{"body":"how are you?","title":"hey"},"source":"poc.pushnotif"}
@jvallesm jvallesm marked this pull request as ready for review May 5, 2022 07:58
@appleboy appleboy added the bug label May 5, 2022
@appleboy
Copy link
Owner

appleboy commented May 5, 2022

LGTM, Thanks

@appleboy appleboy merged commit 3a593aa into appleboy:master May 5, 2022
@jvallesm
Copy link
Contributor Author

Hey @appleboy 👋 Again, thanks for considering the fix and merging it.

Do you know when the next release will be? We found this issue when bumping our gorush version to 1.15 and found the mobile didn't interpret the payload correctly. With a release (even a minor version update) that includes this change we can keep up with the latest fixes in the repo without breaking our backend-mobile contracts 👩‍🌾

@appleboy
Copy link
Owner

appleboy commented Jul 2, 2022

@jvallesm I will take a look.

@appleboy appleboy added this to the v1.16.0 milestone Dec 18, 2022
@appleboy
Copy link
Owner

https://github.com/appleboy/gorush/releases/tag/v1.16.0

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

Successfully merging this pull request may close these issues.

None yet

2 participants