Skip to content

Represent prices as BigDecimal #231

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

Merged
merged 1 commit into from
Apr 7, 2023

Conversation

jon-signal
Copy link
Contributor

The MessageBird API transmits prices as floating point literals rather than arbitrary-precision decimal numbers. This can lead to loss-of-precision issues down the line. For example, consider this (real-world!) API response:

{
  "price": {
    "amount": 0.0018,
    "currency": "USD"
  },
}

If we try to convert the price above into a Joda Money Money instance (again, just as an example), then we run into:

java.lang.ArithmeticException: Scale of amount 0.0017999999690800905 is greater than the scale of the currency USD

While I think it might be wise to transmit the price as a string at the API level, I think there are also things we can do in the client library to help. This pull request, for example, parses the price as a BigDecimal. It preserves the return type of the existing Price#getAmount() method, but introduces a Price#getAmountDecimal() method that returns the BigDecimal representation (ideally without loss of precision).

Although it's beyond the scope of this pull request, if the upstream API response were to change to something like this:

{
  "price": {
    "amount": 0.0018,
    "amountDecimal": "0.0018",
    "currency": "USD"
  },
}

…then this change would be forward-compatible from a Java binary perspective, too.

@denizkilic
Copy link
Contributor

Hi @jon-signal, I agree with using BigDecimal here. The PR looks fine, merging it and the new release will be created.

@denizkilic denizkilic merged commit 2315fb0 into messagebird:master Apr 7, 2023
@jon-signal
Copy link
Contributor Author

Thanks kindly!

@jon-signal jon-signal deleted the decimal_price branch April 7, 2023 14:50
@jon-signal
Copy link
Contributor Author

@denizkilic Apologies—I've just noticed one problem here. We've changed the structure of this class, and so the serialVersionUID needs to change, too!

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