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

Support java records as measurement classes #983

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

eranl
Copy link
Contributor

@eranl eranl commented Dec 16, 2023

Saving using a record is supported by existing code. This change adds support for loading into a record, using its canonical constructor. Both standard (JDK14+) and Android desugared (SDK34+) records are supported.

Access to record metadata is done through reflection, so this code compiles on JDK8. In order to support Android, and in order to keep the reflection logic simple, no record component reflection classes and methods are used. The price is a minor limitation: records can't have a custom constructor that has the same set of parameter names and types as the canonical one.

In order to add tests with records, I added a test-jdk17 directory and a java17 maven profile. I hope I did that correctly. Is there any other required setup for this?

The two new record test classes are identical except that one uses real records (and some new java features), and the other simulates Android desugared ones. They are both based on InfluxDBResultMapperTest, with the addition of a few record-specific tests.

There is some inconsistency between saving and loading that I am conflicted about:

  • On save, existing logic, which lets you choose which fields to include, applies
  • On load, all fields are always loaded into, since the canonical constructor is used

On the one hand, it would be nice to always save all record fields, for consistency. On the other hand, there is value in leaving the flexibility on save, for use cases that don't care about this consistency, e.g. write-only ones. Always saving all record fields would also be a breaking change for any (unlikely) existing code using records for saving.

And this is on top of an existing inconsistency between saving and loading, which applies to both classes and records:

  • On save, all included fields are always saved
  • On load, only fields that are contained in the query result are actually loaded. All other class fields are silently ignored, while other record fields are initialized to null, which results in an exception for primitive fields.

For this one, I could

  1. prevent the exceptions by initializing such primitive fields to their zero value, or
  2. silently ignore the exceptions, or
  3. catch and give a more informative exception, or
  4. leave as is

Fixes #964

Both standard and Android desugared records are supported
@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2023

Codecov Report

Attention: 10 lines in your changes are missing coverage. Please review.

Comparison is base (9209837) 61.05% compared to head (b6979ee) 61.75%.

❗ Current head b6979ee differs from pull request most recent head d11ed0b. Consider uploading reports for the commit d11ed0b to get more accurate results

Files Patch % Lines
...n/java/org/influxdb/impl/InfluxDBResultMapper.java 88.23% 7 Missing and 3 partials ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #983      +/-   ##
============================================
+ Coverage     61.05%   61.75%   +0.69%     
- Complexity      437      453      +16     
============================================
  Files            71       71              
  Lines          2583     2638      +55     
  Branches        278      288      +10     
============================================
+ Hits           1577     1629      +52     
- Misses          933      936       +3     
  Partials         73       73              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@majst01
Copy link
Collaborator

majst01 commented Dec 21, 2023

Hi @eranl

This is a nice improvement, but honestly i am not able to review this in the required depth because i am since long time no full time java developer anymore.

I am searching for a additional maintainer, so if you are interested let me know

@eranl
Copy link
Contributor Author

eranl commented Dec 21, 2023

Hi @eranl

This is a nice improvement, but honestly i am not able to review this in the required depth because i am since long time no full time java developer anymore.

I am searching for a additional maintainer, so if you are interested let me know

Hi @majst01,

Sure, happy to help. I would need some guidance though, since I've never done it.

@eranl
Copy link
Contributor Author

eranl commented Feb 16, 2024

Hi @majst01,

Any update?

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.

Add support for java records
3 participants