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

Modify DefaultMethodHandler to work with Android #1436

Closed
wants to merge 45 commits into from

Conversation

xrpdevs
Copy link

@xrpdevs xrpdevs commented Jun 12, 2021

I was getting errors regarding illegal reflective access here, and also another one saying IMPL_LOOKUP was not defined, depending on the Android version I was running on.

These changes should work across the board, Android or actual JRE's.

A very frustrating way for someone inexperienced with reflection to spend a day :/

HTH someone!

jp80 added 4 commits June 12, 2021 18:50
… 28 (Android 8) however, Android 9+ enforces more strict checking with reflection.

Solution is to use double reflection, as below.

Jamie (xrpdevs-at-xrpdevs-co-uk)

TODO: Integrate back into version forked from, keep original legacyReadLookup and safeReadLookup and add conditional branch if running on Dalvik (android java runtime)
Previous commit did not deal with Android 9+ reflection security.

Solution in Android 9 is to use double reflection.

Android's JVM is detected at runtime and MethodHandle is obtained in an Android compatible way, otherwise, we run readLookup() instead.
Copy link
Member

@velo velo left a comment

Choose a reason for hiding this comment

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

Build is not passing, so, I can't review it in details unless this is building successfully

core/src/main/java/feign/DefaultMethodHandler.java Outdated Show resolved Hide resolved
core/src/main/java/feign/DefaultMethodHandler.java Outdated Show resolved Hide resolved
@xrpdevs xrpdevs requested a review from velo June 14, 2021 13:49
if (System.getProperty("java.vm.name").equalsIgnoreCase("Dalvik")) {
return androidLookup(declaringClass);
} else {
return safeReadLookup(declaringClass);
Copy link
Author

Choose a reason for hiding this comment

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

I had "readLookup" instead of "safeReadLookup" here. Building now...

Copy link
Author

@xrpdevs xrpdevs left a comment

Choose a reason for hiding this comment

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

This is building OK now?

@velo
Copy link
Member

velo commented Jun 16, 2021

For some unknow reason, this is not building at all:
Screen Shot 2021-06-16 at 15 22 56

@kdavisk6 any clue?

@xrpdevs
Copy link
Author

xrpdevs commented Jun 16, 2021 via email

@xrpdevs xrpdevs mentioned this pull request Jun 16, 2021
jp80 and others added 21 commits June 16, 2021 13:53
velo added a commit that referenced this pull request Jun 16, 2021
* Modify DefaultMethodHandler to work with Android.

* Modified to work with Android. Previous commit worked up to API level 28 (Android 8) however, Android 9+ enforces more strict checking with reflection.
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

3 participants