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 JsonNumber bson serialization #898

Open
wants to merge 1 commit into
base: 2.11.x
Choose a base branch
from

Conversation

rorueda
Copy link

@rorueda rorueda commented Jul 23, 2024

  • Always use the right number type when serializing, so formats that support the type info can deserialize to the correct type. Currently, if you serialize an int JsonNumber, when deserializing you get a long JsonNumber.
  • Fix fallback deserialization of custom Number implementations. The bson decoder advances when getBigDecimal is called - so, in the second call you either get an exception or one of the fields is swallowed.
  • Fix element index increment when serializing BigInteger. Increment is already done by encodeBigDecimal.

- Always use the right number type when serializing, so formats
that support the type info can deserialize to the correct type
- Fix fallback deserialization of custom Number implementations
- Fix element index increment when serializing BigInteger
@CLAassistant
Copy link

CLAassistant commented Jul 23, 2024

CLA assistant check
All committers have signed the CLA.

@@ -279,6 +279,11 @@ protected Number getBestNumber() {
};
}

@Override
protected BigDecimal getBigDecimalFromNumber(Number number) {
return ((Decimal128) number).bigDecimalValue();
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be implemented using instanceOf or different convertions

* @throws UnsupportedOperationException If custom number types are not expected
*/
protected BigDecimal getBigDecimalFromNumber(Number number) {
throw new UnsupportedOperationException();
Copy link
Member

Choose a reason for hiding this comment

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

Definitely needs a proper default implementation.

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

Successfully merging this pull request may close these issues.

4 participants