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 serialization of Long inside of Request.data #2051

Merged
merged 3 commits into from May 18, 2022

Conversation

adinauer
Copy link
Member

@adinauer adinauer commented May 16, 2022

📜 Description

Handle Number inside JsonReflectionObjectSerializer which includes Long.

💡 Motivation and Context

Fix #2044

💚 How did you test it?

Unit Test

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the docs if needed
  • No breaking changes

🔮 Next steps

} else if (object instanceof Float) {
return object;
} else if (object instanceof Double) {
} else if (object instanceof Number) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit confused why do we have special handling for byte and short and don't for others?

Copy link
Member Author

Choose a reason for hiding this comment

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

Also not sure, why they are converted to Integer. I'll try to find out.

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a test that fails if the special handling is removed. I've added another test that serializes then deserializes the output again. It seems to work without the special handling. But I'm not sure if there's a reason it's there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please check with @denrase

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess i had the wrong expectation in the test, therefore the cast to int. So this part should be correct now.

The intent here was to return the string, characters cast to string and all Java primitives (of which i forgot Long apparently 🤦), as those are pretty much the leaves of the tree we are trying to traverse.

Number is a superclass to more than those primitives, but i'm not sure what the impact would be if we for example encounter something like DoubleAccumulator ?

https://docs.oracle.com/javase/8/docs/api/java/lang/Number.html

Maybe we should introduce a method Bool isPrimitive(Object object) (and also check for Long) to keep the original intent and not have to think about those other Number subclasses.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just tested DoubleAccumulator, BigInteger and BigDecimal locally. All of them worked. Didn't test all Number subtypes but I'd say risk is pretty low here.

@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (6.x.x@48c6733). Click here to learn what that means.
The diff coverage is n/a.

@@           Coverage Diff            @@
##             6.x.x    #2051   +/-   ##
========================================
  Coverage         ?   80.88%           
  Complexity       ?     3170           
========================================
  Files            ?      228           
  Lines            ?    11716           
  Branches         ?     1574           
========================================
  Hits             ?     9476           
  Misses           ?     1649           
  Partials         ?      591           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48c6733...9f44ec1. Read the comment docs.

@adinauer adinauer merged commit cfdd10b into 6.x.x May 18, 2022
@adinauer adinauer deleted the fix/long-serialization-in-request-data branch May 18, 2022 06:16
@github-actions
Copy link
Contributor

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Fix serialization of Long inside of Request.data ([#2051](https://github.com/getsentry/sentry-java/pull/2051))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against 9f44ec1

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.

None yet

5 participants