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

IllegalAccessException when auto-mapping Records (JEP-359) #2195

Closed
GeorgeSalu opened this issue Mar 8, 2021 · 13 comments
Closed

IllegalAccessException when auto-mapping Records (JEP-359) #2195

GeorgeSalu opened this issue Mar 8, 2021 · 13 comments
Assignees
Labels
Milestone

Comments

@GeorgeSalu
Copy link

I want MyBatis to Support for JDK 16 record types

@ksmith97
Copy link

ksmith97 commented May 12, 2021

This actually does work if you follow the directions for dealing with immutable objects and supply a constructor binding in a custom result map. Unfortunately, it is a lot more verbose than would be preferred. Maybe the code could check if the result type is a record and because result objects are always consistent in constructors it could automatically bind it together properly. Unfortunately, this probably relies on the isRecord method that does not exist before JDK 16. If the code allows for it maybe an optional adapter could be written that would allow for this support to be added for those of us on newer JDKs. If it does not allow for it currently maybe a new feature request could be submitted to allow for the code to allow something to hook in at the relevant location to supply this functionality?

@kazuki43zoo kazuki43zoo added the enhancement Improve a feature or add a new feature label Jun 4, 2021
@kazuki43zoo
Copy link
Member

kazuki43zoo commented Jun 4, 2021

@harawata

I will try to support the Record type by following commit(not includes test).
The Record type cannot set a value to the final field, then I changed to skip property mapping for prevent mapping error.

kazuki43zoo@f8a5eda

WDYT?

@harawata
Copy link
Member

harawata commented Jun 4, 2021

@kazuki43zoo ,
Is that a workaround for the behavior I mentioned in the p.s. of this comment?
If so, I would like to consider correcting that behavior (#2207 is the same issue if I understand correctly).
If it's not related, please let me know and I'll take another look.
Thanks!

@kazuki43zoo
Copy link
Member

kazuki43zoo commented Jun 5, 2021

@harawata Thanks for your reaction!!
I think that the root cause is the same with #2207 (= MyBatis try to write value to final field). But It is different behavior with standard type and record type on JDK 16.

standard type:

public class MyEntity {
   private final Integer id;
   private final String name;
   public MyEntity(Integer id, String name) {
     this.id = id;
     this.name = name;
   }
   // ...
}

recored type:

public record MyEntity(Integer id, String name) {
}

The standard type work but it write value to final field(Field#trustedFinal=false) at twice by constructor mapping and property(field) auto mapping. The recored type occurred an Exception because can't write a final field(Field#trustedFinal=true) on record type.

I would like to consider correcting that behavior (#2207 is the same issue if I understand correctly).

I agree with this!!

@harawata
Copy link
Member

harawata commented Jun 6, 2021

Thank you, @kazuki43zoo for the information!

I didn't know about the trustedFinal. There is no public method to check if a Field is 'trusted' or not, it seems. 😕

Anyway, my first impression is that this is an issue that should be addressed in org.apache.ibatis.reflection.Reflector (e.g. skip setter detection if it's a Record) [1].
Skipping particular processes may look simpler, but it gets harder to maintain in the long run.
Will there be any difficulty with this approach?

[1] There is another outstanding feature request slightly related to this topic. It might be a good time to add support for the prefix-less getter/setter methods to Reflector. This can be a separate discussion, though.

@cmosher01
Copy link

FYI, I had record types working fine with JDK 14 in preview mode. But when I switched to JDK 16 (non-preview), I started running into the final field update error: java.lang.IllegalAccessException: Can not set final int field.....

@GeorgeSalu
Copy link
Author

a next version with Record support would be great. @kazuki43zoo

@bzhou
Copy link
Contributor

bzhou commented Mar 10, 2022

Pull request #2477 sent

@bzhou
Copy link
Contributor

bzhou commented Mar 11, 2022

Anyway, my first impression is that this is an issue that should be addressed in org.apache.ibatis.reflection.Reflector (e.g. skip setter detection if it's a Record) [1]. Skipping particular processes may look simpler, but it gets harder to maintain in the long run. Will there be any difficulty with this approach?

Agreed about the approach should be addressing Reflector.

The reason it is trying to write to final field, is that it treat java record like any class, so it tries to

  • find all getters, there won't be any normally;
  • find all setters, of course there won't be any because records are immutable;
  • find all "fields", it finds all record components, but does not understand they are read-only.

Dealing with java record like this actually is a problematic bug.

My patch

  • detects a class is a record by checking the name of its super class (so does not require java 16 and backward compatible)
  • in that case simply finds all accessors (methods with no parameter) and add them as getters

That's it. You can see easily that in the case of java record, it might even be slightly faster :-)

@harawata harawata changed the title Support for JDK 16 record types IllegalAccessException when auto-mapping Records (JEP-359) Mar 23, 2022
@harawata
Copy link
Member

Thanks to @bzhou 's contribution, the problem with auto-mapping is fixed in the latest 3.5.10-SNAPSHOT.
Please test it with your solutions and give us feedback if you experience any issue.
Thank you all for comments and upvote reactions!

@harawata harawata added this to the 3.5.10 milestone Mar 26, 2022
@harawata harawata self-assigned this Mar 26, 2022
@GeorgeSalu
Copy link
Author

the xml mapping remains the same using immutable objects before JDK 16 record types ?

@harawata
Copy link
Member

@GeorgeSalu ,

I'm not sure if I understand correctly, but the reported error occurs when you rely on auto-mapping.

As mentioned earlier, if you use properly configured <resultMap />, there should be no problem with mapping JDK 16 record or traditional immutable objects even with version ≤ 3.5.9.
Please see the "constructor" section in the doc for the details.

@GeorgeSalu
Copy link
Author

Any release date forecast?

@harawata harawata added bug and removed enhancement Improve a feature or add a new feature labels May 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

6 participants