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

protocols/horizon: Add custom UnmarshalJSON to struct with int64 fields. #1915

Merged

Conversation

abuiles
Copy link
Contributor

@abuiles abuiles commented Nov 13, 2019

PR Checklist

PR Structure

  • This PR has reasonably narrow scope (if not, break it down into smaller PRs).
  • This PR avoids mixing refactoring changes with feature changes (split into two PRs
    otherwise).
  • This PR's title starts with name of package that is most changed in the PR, ex.
    services/friendbot, or all or doc if the changes are broad or impact many
    packages.

Thoroughness

  • This PR adds tests for the most critical parts of the new functionality or fixes.
  • I've updated any docs (developer docs, .md
    files, etc... affected by this change). Take a look in the docs folder for a given service,
    like this one.

Release planning

  • I've updated the relevant CHANGELOG (here for Horizon) if
    needed with deprecations, added features, breaking changes, and DB schema changes.
  • I've decided if this PR requires a new major/minor version according to
    semver, or if it's mainly a patch change. The PR is targeted at the next
    release branch if it's not a patch change.

What

Add UnmarshalJSON to Trade, this method supports parsing of int64 fields as as json.Number which might be a string or int64. This will allow us to cut a new release for the horizon client which won't break once we change the data type on the JSON payload.

Why

Using Int64 in JSON payloads produces inconsistent results in JS (see #1363).

We are in the process of updating all the int64 JSON fields to be of type string. The idea with this change is to extend the Unmarshal function to take either int64 or string, that way we can release a version of the horizon client which won't break once the payload changes in the API.

See #1609, #1909 and #1912

Known limitations

[TODO or N/A]

@cla-bot cla-bot bot added the cla: yes label Nov 13, 2019
@abuiles abuiles force-pushed the string-or-number-in-json-field branch from 260cfc5 to 9771107 Compare November 13, 2019 20:31
@abuiles abuiles marked this pull request as ready for review November 13, 2019 20:31
@abuiles abuiles force-pushed the string-or-number-in-json-field branch from 9771107 to 30c7754 Compare November 14, 2019 16:58
Copy link
Contributor

@bartekn bartekn left a comment

Choose a reason for hiding this comment

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

I'm wondering if this PR is only for horizon client release can we:

  1. Recreate it on top of master.
  2. Set the base branch to a release branch of horizon client.
  3. Instead of creating custom UnmarshalJSON, just use json.Number where needed.
  4. After releasing horizon client do not merge it into master (yet).
  5. Merge the release branch when Horizon 0.25.0 is released.

This will make this code much less complicated, however, it will be easy to introduce regression bug (ex. new horizon client release from a branch branched off master without this commit).

protocols/horizon/effects/main.go Outdated Show resolved Hide resolved
@tamirms
Copy link
Contributor

tamirms commented Nov 14, 2019

@abuiles should this PR be merged into the horizonclient 2.0.0 release branch instead of the 0.24.0 horizon release?

@abuiles
Copy link
Contributor Author

abuiles commented Nov 14, 2019

@bartekn

Instead of creating custom UnmarshalJSON, just use json.Number where needed.

If we use json.Number wouldn't that be changing the type to json.Number instead of int64? It would be a breaking change for users, since they'll have to change their code to call .Int64()

@bartekn
Copy link
Contributor

bartekn commented Nov 14, 2019

@abuiles You're obviously right! I though that json.Number is int64 but it's string.

@leighmcculloch
Copy link
Member

We could make our own int64 type that acts like json.Number in regards to strings but it would still be a breaking change because an importer would need to explicitly cast from our type to the int64 type they're already using.

@abuiles abuiles changed the base branch from release-horizon-v0.24.0 to release-horizonclient-v2.0.0 November 14, 2019 19:45
@abuiles abuiles changed the base branch from release-horizonclient-v2.0.0 to release-horizon-v0.24.0 November 14, 2019 19:53
@abuiles
Copy link
Contributor Author

abuiles commented Nov 14, 2019

We could make our own int64 type that acts like json.Number in regards to strings but it would still be a breaking change because an importer would need to explicitly cast from our type to the int64 type they're already using.

Exactly, that's what we are trying to achieve with this. We want to make it transparent to final users that the type changed in the payload, but keep it as an int64 in Go.

@abuiles abuiles force-pushed the string-or-number-in-json-field branch from e5bb717 to f6116b6 Compare November 14, 2019 19:59
@abuiles abuiles changed the base branch from release-horizon-v0.24.0 to release-horizonclient-v2.0.0 November 14, 2019 20:00
@abuiles abuiles force-pushed the string-or-number-in-json-field branch from f6116b6 to 512fb74 Compare November 14, 2019 20:01
@abuiles
Copy link
Contributor Author

abuiles commented Nov 14, 2019

@tamirms I squashed all the related commits and updated the branch, PTAL :)

@abuiles abuiles force-pushed the string-or-number-in-json-field branch 2 times, most recently from 143f5c9 to 632e5e1 Compare November 14, 2019 21:38
@abuiles abuiles requested a review from tamirms November 14, 2019 21:38
@abuiles
Copy link
Contributor Author

abuiles commented Nov 14, 2019

@tamirms updated the trade struct following your recommendation. Thanks!

@abuiles abuiles force-pushed the string-or-number-in-json-field branch from 632e5e1 to f8fb453 Compare November 14, 2019 21:53
@abuiles abuiles merged commit 2be7761 into release-horizonclient-v2.0.0 Nov 15, 2019
@abuiles abuiles deleted the string-or-number-in-json-field branch November 15, 2019 00:21
@abuiles abuiles mentioned this pull request Jan 20, 2020
9 tasks
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.

4 participants