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 empty memo field in JSON when memo_type is text #635

Merged
merged 6 commits into from
Sep 7, 2018
Merged

Fix empty memo field in JSON when memo_type is text #635

merged 6 commits into from
Sep 7, 2018

Conversation

lechengfan
Copy link
Contributor

@lechengfan lechengfan commented Sep 5, 2018

  • resolves JSON: Memo: An empty "memo" is not being generated for a transaction even if "memo_type" is "text" #149, where the "memo" field does not exist in the JSON response for a transaction whose memo_type is text and the memo happens to be an empty string.
  • Fix is to define a custom JSON marshaler that converts the Memo field to be a string pointer. That way if the memo_type is none, it can be set to nil and the resulting JSON response doesn't have a memo field. In all other cases, it points to a string (possibly the empty string), which would cause the JSON response to include "memo" as a field.
  • Add some test cases to ensure this behavior, and also make sure that the default marshalling and unmarshalling of Transactions still work.

@kr
Copy link
Contributor

kr commented Sep 5, 2018

points to an empty string which would cause the JSON response to have "memo": "" as a field

For the sake of precision, maybe we can change the description to say 'points to a string (possibly the empty string), which would cause the JSON response to have "memo" as a field'.

@bartekn bartekn added this to the Horizon v0.15.0 milestone Sep 6, 2018
@bartekn bartekn added the horizon label Sep 6, 2018
@@ -372,6 +373,20 @@ type Transaction struct {
ValidBefore string `json:"valid_before,omitempty"`
}

func (t Transaction) MarshalJSON() ([]byte, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a short comment explaining why we need this method.

I'm also wondering if we should change Memo field tag on Transaction to json:"-" to avoid confusion.

@@ -49,3 +50,50 @@ func TestAccount(t *testing.T) {
})
})
}

func TestTransactionJSONMarshal(t *testing.T) {
Convey("After marshalling and unmarshalling, the resulting struct should be the exact same as the original", t, func() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not written down anywhere (👎 for us) but we want to remove github.com/smartystreets/goconvey dependency so it's better to write all new tests using Test... functions and assert, mock, etc. packages from github.com/stretchr/testify.

@bartekn
Copy link
Contributor

bartekn commented Sep 6, 2018

PS: please use GH closing tags to automatically close issues. This should be used in PR description or one of the commits.

t.Errorf("MemoType is none, but memo is not nil")
}
})
// If a transaction's memo type is None, then memo field should be nil in JSON
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: s/should be nil in/should be omitted from/

@kr kr mentioned this pull request Sep 6, 2018
@bartekn bartekn merged commit b20aa66 into stellar:master Sep 7, 2018
@bartekn bartekn modified the milestones: Horizon v0.15.0, Horizon v0.14.x - patch releases Sep 10, 2018
@lechengfan lechengfan deleted the memo-json-marshall branch September 10, 2018 19:09
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.

JSON: Memo: An empty "memo" is not being generated for a transaction even if "memo_type" is "text"
3 participants