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 for JDK 16 record type #2477

Merged
merged 10 commits into from
Mar 23, 2022
Merged

support for JDK 16 record type #2477

merged 10 commits into from
Mar 23, 2022

Conversation

bzhou
Copy link
Contributor

@bzhou bzhou commented Mar 10, 2022

This is to address #2195 by handling record type in Reflector.java.

The Reflector change does not require newer version of java.

The corresponding test change does require JDK 16 or later.

@coveralls
Copy link

coveralls commented Mar 21, 2022

Coverage Status

Coverage decreased (-0.06%) to 87.224% when pulling 136595c on bzhou:master into 0ccf9ba on mybatis:master.

@harawata
Copy link
Member

Thank you, @bzhou !

I made some changes.
Please check my commits and let me know if I missed something.

@bzhou
Copy link
Contributor Author

bzhou commented Mar 21, 2022

Looks good, thank you @harawata.

In terms of testing, in my test code, there was one case where I used
@Arg(column="property_id", javaType=PropId.class, typeHandler=PropIdTypeHandler.class),
The PropId is also a record class with just one Integer component. This is a case where the old code gets very confused and error out.

I tried run your latest simplified testing against the old Reflector code, turns out

  • testSelectRecord passes
  • testInsertRecord passes
  • testSelectRecordAutomapping caught IllegalAccessException: Can not set final int field org.apache.ibatis.submitted.record_type.Property.id to java.lang.Integer

So I guess that's also good.

I'm good with your changes, please merge into mybatis:mybatis-3 when convenient. Thanks!

@harawata
Copy link
Member

Thank you, @bzhou !

Based on your feedback, I have added a few more commits.

  • Modified the test a little bit and now the testInsertRecord will fail with the old implementation. The difference is that the old implementation reads the field value directly while the new implementation invokes the accessor method.
  • Added a new test that involving nested records. It uses nested result map which may be more flexible compared to type handler. This mapping should work with the old implementation as well.

@bzhou
Copy link
Contributor Author

bzhou commented Mar 23, 2022

Really cool, thanks @harawata!

@harawata harawata merged commit 9ee6941 into mybatis:master Mar 23, 2022
@harawata
Copy link
Member

The change is merged and is in the latest 3.5.10-SNAPSHOT.
I'll comment on #2195 later.
Thanks again, @bzhou !

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.

3 participants