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

Amount is sent as number, consider string instead #43

Closed
Psynbiotik opened this issue May 3, 2017 · 6 comments
Closed

Amount is sent as number, consider string instead #43

Psynbiotik opened this issue May 3, 2017 · 6 comments
Assignees

Comments

@Psynbiotik
Copy link

I wanted to find another page which outlines in more depth the issues, but I could only find the ones below.

A quick summary is some JSON parsers will deserialize numbers as floats which obviously is problematic for Money. Other issues with eventually being deserialized as a float are limitations on size and problems with adding/subtracting. If you send it as a String it will be deserialized as a String.

Various browsers use IEEE-754 to represent their numbers by default which would cause issues.

http://stackoverflow.com/questions/30249406/what-is-the-standard-for-formatting-currency-values-in-json

nlohmann/json#98

shopspring/decimal#21

@Psynbiotik
Copy link
Author

@AlexanderYastrebov
Copy link
Member

Related discussion #42

@whiskeysierra
Copy link
Collaborator

Related section in our guidelines: http://zalando.github.io/restful-api-guidelines/common-data-objects/CommonDataObjects.html#should-use-a-common-money-object

Make sure that you don’t convert the “amount” field to float / double types when implementing this interface in a specific language or when doing calculations. Otherwise, you might lose precision. Instead, use exact formats like Java’s BigDecimal. See Stack Overflow for more info.

Some JSON parsers (NodeJS’s, for example) convert numbers to floats by default. After discussing the pros and cons, we’ve decided on "decimal" as our amount format. It is not a standard OpenAPI format, but should help us to avoid parsing numbers as float / doubles.

I hope that spreadsheet is public, otherwise I'll copy the gist here.

@AlexanderYastrebov
Copy link
Member

It is true that the problem with serialization type for monetary amount is purely a client side problem.
OTOH I can imagine (and I would do this myself) API designer who would like his clients to think twice about what they are doing when they are working with monetary and a good way to do this is to use string serialization for amount.
Our guidelines are, well, ours but I think it is good for the library to gain wider adoption.

@Psynbiotik
Copy link
Author

@whiskeysierra Looks like the spreadsheet is not public, copying the gist here would be helpful.

@AlexanderYastrebov I agree, I'd like to see this use string serialization by default to force clients to think about it and prevent some default client side behavior pitfalls or at least have an easy way to enable the option to do string serialization while supporting either/or for deserialization.

@whiskeysierra
Copy link
Collaborator

@Psynbiotik I opened a PR with the discussion rewritten into a somewhat comprehensive document, see #49

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

No branches or pull requests

3 participants