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

Update minimum support Java version from 8 to 11 #2798

Closed
4 of 5 tasks
reta opened this issue Nov 23, 2022 · 4 comments · Fixed by #2804
Closed
4 of 5 tasks

Update minimum support Java version from 8 to 11 #2798

reta opened this issue Nov 23, 2022 · 4 comments · Fixed by #2804

Comments

@reta
Copy link
Contributor

reta commented Nov 23, 2022

Hello!

We [1] have been using Mockito with JDK's SecurityManager (for historical reasons), the things worked fine for us till Mockito 4.8.0 release (and later versions). The culprit is Java9PlusLocationImpl (see please stack trace below):

Caused by: java.security.AccessControlException: access denied ("java.lang.RuntimePermission" "accessDeclaredMembers")
	at java.base/java.security.AccessControlContext.checkPermission(AccessControlContext.java:472)
	at java.base/java.security.AccessController.checkPermission(AccessController.java:897)
	at java.base/java.lang.SecurityManager.checkPermission(SecurityManager.java:322)
	at java.base/java.lang.Class.checkMemberAccess(Class.java:2847)
	at java.base/java.lang.Class.getDeclaredMethod(Class.java:2471)
	at org.mockito.internal.debugging.Java9PlusLocationImpl.stackWalker(Java9PlusLocationImpl.java:191)
	at org.mockito.internal.debugging.Java9PlusLocationImpl.<clinit>(Java9PlusLocationImpl.java:50)
	... 47 more

By all means, Mockito should know nothing about SecurityManager and the way we implemented mocking under SecurityManager is by using Mockito's extensibility model - plugins (primary, MockMaker). Sadly for LocationFactory / Location there is no such plugin available so we are stuck on 4.7.x release line. If such plugin would have been available, we could provide privileged implementation for reflection calls. Alternatively, wrapping up Java 9 implementation into multi-release JAR (and getting rid of reflection), would have helped.

We are happy to submit the pull request with either solution, but it would be helpful to know if there is an interest addressing this issue in general, and preferred approach in particular:

  • plugin for LocationFactory / Location
  • multi-release JAR for Java 9

Thank you.

  • The mockito message in the stacktrace have useful information, but it didn't help
  • The problematic code (if that's possible) is copied here;
    Note that some configuration are impossible to mock via Mockito
  • Provide versions (mockito / jdk / os / any other relevant information)
  • Provide a Short, Self Contained, Correct (Compilable), Example of the issue
    (same as any question on stackoverflow.com)
  • Read the contributing guide

[1] https://github.com/opensearch-project/OpenSearch

@TimvdLippe
Copy link
Contributor

As a third option, we can also drop Java 8, so that we can import the StackWalker directly.

I am not sure what's best. A plugin for the LocationFactory sounds a bit weird, as that is internal to Mockito and how it works. A multi-release jar, I am not sure how well this will integrate with the build systems of our users and I am afraid that we might inadvertently break them.

@mockito/developers We have been hitting several Java 8 issues lately and this is yet another one where we have to workaround Java 8 to support modern JDK versions. WDYT of upgrading to Java 11 as a minimum requirement? I can create a separate issue for that if you want.

@raphw
Copy link
Member

raphw commented Nov 23, 2022

I think it's fair to move on to Java 11. Today Spring 6 arrived which baselines to Java 17, we are not even on the edge of things with this in mind, so thumbs up for bumping to 11.

@reta
Copy link
Contributor Author

reta commented Nov 24, 2022

@raphw that's unexpected win-win outcome :-), thanks @TimvdLippe for this 3rd option, I can take it (under new issue) if no one is working on it at the moment.

@TimvdLippe TimvdLippe mentioned this issue Nov 27, 2022
4 tasks
@TimvdLippe
Copy link
Contributor

TimvdLippe commented Nov 27, 2022

I filed #2802 to consolidate the changes for Mockito 5. For this issue, we will need to:

We can clean up the source code in src/main/java to remove all reflection usages (and thus solve this ticket) in Mockito 5.1.0. I would prefer to make it a clean cut first, so that upgrading is relatively straightforward. Then fixes go in a minor release, so that we can catch regressions separately.

Let me know if you think I missed something. I would welcome some PRs to get these fixed!

@TimvdLippe TimvdLippe changed the title Java9PlusLocationImpl and JDK's SecurityManager: java.security.AccessControlException Update minimum support Java version from 8 to 11 Nov 27, 2022
@TimvdLippe TimvdLippe pinned this issue Nov 27, 2022
reta added a commit to reta/mockito that referenced this issue Nov 28, 2022
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
reta added a commit to reta/mockito that referenced this issue Nov 28, 2022
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
reta added a commit to reta/mockito that referenced this issue Nov 29, 2022
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
reta added a commit to reta/mockito that referenced this issue Nov 30, 2022
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
reta added a commit to reta/mockito that referenced this issue Nov 30, 2022
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
reta added a commit to reta/mockito that referenced this issue Nov 30, 2022
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
reta added a commit to reta/mockito that referenced this issue Nov 30, 2022
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
reta added a commit to reta/mockito that referenced this issue Nov 30, 2022
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
reta added a commit to reta/mockito that referenced this issue Nov 30, 2022
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
reta added a commit to reta/mockito that referenced this issue Nov 30, 2022
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
reta added a commit to reta/mockito that referenced this issue Nov 30, 2022
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
reta added a commit to reta/mockito that referenced this issue Nov 30, 2022
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
reta added a commit to reta/mockito that referenced this issue Nov 30, 2022
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
reta added a commit to reta/mockito that referenced this issue Dec 14, 2022
Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
TimvdLippe pushed a commit that referenced this issue Dec 18, 2022
Fixes #2798

Signed-off-by: Andriy Redko <andriy.redko@aiven.io>
@TimvdLippe TimvdLippe unpinned this issue Dec 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants