From 84f8cf8b5274955904703863fa052d6172de2f20 Mon Sep 17 00:00:00 2001 From: Charles Munger Date: Tue, 25 May 2021 11:42:20 -0700 Subject: [PATCH 1/3] Use the parent classloader if the context classloader is a child of it. This should only affect cases that previously would have thrown an exception. Fixes #2303 --- .../bytebuddy/SubclassBytecodeGenerator.java | 40 +++++++++- ...kagePrivateWithContextClassLoaderTest.java | 74 +++++++++++++++++++ 2 files changed, 111 insertions(+), 3 deletions(-) create mode 100644 src/test/java/org/mockitousage/bugs/creation/PackagePrivateWithContextClassLoaderTest.java diff --git a/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java b/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java index dc5c6e55f9..ac1330b8da 100644 --- a/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java +++ b/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java @@ -66,6 +66,21 @@ protected SubclassBytecodeGenerator(SubclassLoader loader, Implementation readRe random = new Random(); } + private static boolean needsSamePackageClassLoader(MockFeatures features) { + if (!Modifier.isPublic(features.mockedType.getModifiers()) + || !features.mockedType.isInterface()) { + // The mocked type is package private or is not an interface and thus may contain package + // private methods. + return true; + } + for (Class iface : features.interfaces) { + if (!Modifier.isPublic(iface.getModifiers())) { + return true; + } + } + return false; + } + @Override public Class mockClass(MockFeatures features) { DynamicType.Builder builder = @@ -100,14 +115,33 @@ public Class mockClass(MockFeatures features) { .throwing(ClassNotFoundException.class, IOException.class) .intercept(readReplace); } - ClassLoader classLoader = new MultipleParentClassLoader.Builder() + MultipleParentClassLoader.Builder loaderBuilder = new MultipleParentClassLoader.Builder() .append(features.mockedType) .append(features.interfaces) - .append(currentThread().getContextClassLoader()) .append(MockAccess.class, DispatcherDefaultingToRealMethod.class) .append(MockMethodInterceptor.class, MockMethodInterceptor.ForHashCode.class, - MockMethodInterceptor.ForEquals.class).build(MockMethodInterceptor.class.getClassLoader()); + MockMethodInterceptor.ForEquals.class); + ClassLoader contextLoader = currentThread().getContextClassLoader(); + boolean shouldIncludeContextLoader = true; + if (needsSamePackageClassLoader(features)) { + // For the generated class to access package-private methods, it must be defined by the + // same classloader as its type. All the other added classloaders are required to load + // the type; if the context classloader is a child of the mocked type's defining + // classloader, it will break a mock that would have worked. Check if the context class + // loader is a child of the classloader we'd otherwise use, and possibly skip it. + ClassLoader candidateLoader = loaderBuilder.build(); + for (ClassLoader parent = contextLoader; parent != null; parent = parent.getParent()) { + if (parent == candidateLoader) { + shouldIncludeContextLoader = false; + break; + } + } + } + if (shouldIncludeContextLoader) { + loaderBuilder = loaderBuilder.append(contextLoader); + } + ClassLoader classLoader = loaderBuilder.build(MockMethodInterceptor.class.getClassLoader()); if (classLoader != features.mockedType.getClassLoader()) { assertVisibility(features.mockedType); for (Class iFace : features.interfaces) { diff --git a/src/test/java/org/mockitousage/bugs/creation/PackagePrivateWithContextClassLoaderTest.java b/src/test/java/org/mockitousage/bugs/creation/PackagePrivateWithContextClassLoaderTest.java new file mode 100644 index 0000000000..9b44a48077 --- /dev/null +++ b/src/test/java/org/mockitousage/bugs/creation/PackagePrivateWithContextClassLoaderTest.java @@ -0,0 +1,74 @@ +/* + * Copyright (c) 2017 Mockito contributors + * This program is made available under the terms of the MIT License. + */ +package org.mockitousage.bugs.creation; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; +import static org.mockito.Mockito.withSettings; + +import org.junit.After; +import org.junit.Before; +import org.junit.Test; + +/** Regression tests for issue #2303*/ +public class PackagePrivateWithContextClassLoaderTest { + + private ClassLoader oldContextClassloader; + + public static class PublicClass { + + int packagePrivateMethod() { + return 0; + } + } + + public interface PublicInterface { + + } + + interface PackagePrivateInterface { + + } + + static class PackagePrivateClass { + + } + + @Before + public void setUp() { + oldContextClassloader = Thread.currentThread().getContextClassLoader(); + Thread.currentThread().setContextClassLoader(new ClassLoader(oldContextClassloader) { + }); + } + + @After + public void teardown() { + Thread.currentThread().setContextClassLoader(oldContextClassloader); + } + + @Test + public void should_be_able_to_mock_package_private_method() throws Exception { + PublicClass publicClass = mock(PublicClass.class); + when(publicClass.packagePrivateMethod()).thenReturn(3); + assertThat(publicClass.packagePrivateMethod()).isEqualTo(3); + } + + @Test + public void should_be_able_to_mock_package_private_class() throws Exception { + PackagePrivateClass mock = mock(PackagePrivateClass.class); + } + + @Test + public void should_be_able_to_mock_package_private_interface() throws Exception { + PackagePrivateInterface mock = mock(PackagePrivateInterface.class); + } + + @Test + public void should_be_able_to_mock_package_private_extra_interface() throws Exception { + PackagePrivateInterface mock = (PackagePrivateInterface) mock(PublicInterface.class, + withSettings().extraInterfaces(PackagePrivateInterface.class)); + } +} From 91c2ef17f994f9114a401a1f58806284d08ac19f Mon Sep 17 00:00:00 2001 From: Charles Munger Date: Tue, 25 May 2021 12:41:15 -0700 Subject: [PATCH 2/3] Fix formatting --- .../bytebuddy/SubclassBytecodeGenerator.java | 21 ++++++++++-------- ...kagePrivateWithContextClassLoaderTest.java | 22 ++++++++----------- 2 files changed, 21 insertions(+), 22 deletions(-) diff --git a/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java b/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java index 0424912ee3..985823f39a 100644 --- a/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java +++ b/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java @@ -92,8 +92,9 @@ protected SubclassBytecodeGenerator( private static boolean needsSamePackageClassLoader(MockFeatures features) { if (!Modifier.isPublic(features.mockedType.getModifiers()) - || !features.mockedType.isInterface()) { - // The mocked type is package private or is not an interface and thus may contain package + || !features.mockedType.isInterface()) { + // The mocked type is package private or is not an interface and thus may contain + // package // private methods. return true; } @@ -107,13 +108,15 @@ private static boolean needsSamePackageClassLoader(MockFeatures features) { @Override public Class mockClass(MockFeatures features) { - MultipleParentClassLoader.Builder loaderBuilder = new MultipleParentClassLoader.Builder() - .append(features.mockedType) - .append(features.interfaces) - .append(MockAccess.class, DispatcherDefaultingToRealMethod.class) - .append(MockMethodInterceptor.class, - MockMethodInterceptor.ForHashCode.class, - MockMethodInterceptor.ForEquals.class); + MultipleParentClassLoader.Builder loaderBuilder = + new MultipleParentClassLoader.Builder() + .append(features.mockedType) + .append(features.interfaces) + .append(MockAccess.class, DispatcherDefaultingToRealMethod.class) + .append( + MockMethodInterceptor.class, + MockMethodInterceptor.ForHashCode.class, + MockMethodInterceptor.ForEquals.class); ClassLoader contextLoader = currentThread().getContextClassLoader(); boolean shouldIncludeContextLoader = true; if (needsSamePackageClassLoader(features)) { diff --git a/src/test/java/org/mockitousage/bugs/creation/PackagePrivateWithContextClassLoaderTest.java b/src/test/java/org/mockitousage/bugs/creation/PackagePrivateWithContextClassLoaderTest.java index 9b44a48077..e01365ba99 100644 --- a/src/test/java/org/mockitousage/bugs/creation/PackagePrivateWithContextClassLoaderTest.java +++ b/src/test/java/org/mockitousage/bugs/creation/PackagePrivateWithContextClassLoaderTest.java @@ -25,23 +25,16 @@ int packagePrivateMethod() { } } - public interface PublicInterface { + public interface PublicInterface {} - } - - interface PackagePrivateInterface { - - } - - static class PackagePrivateClass { + interface PackagePrivateInterface {} - } + static class PackagePrivateClass {} @Before public void setUp() { oldContextClassloader = Thread.currentThread().getContextClassLoader(); - Thread.currentThread().setContextClassLoader(new ClassLoader(oldContextClassloader) { - }); + Thread.currentThread().setContextClassLoader(new ClassLoader(oldContextClassloader) {}); } @After @@ -68,7 +61,10 @@ public void should_be_able_to_mock_package_private_interface() throws Exception @Test public void should_be_able_to_mock_package_private_extra_interface() throws Exception { - PackagePrivateInterface mock = (PackagePrivateInterface) mock(PublicInterface.class, - withSettings().extraInterfaces(PackagePrivateInterface.class)); + PackagePrivateInterface mock = + (PackagePrivateInterface) + mock( + PublicInterface.class, + withSettings().extraInterfaces(PackagePrivateInterface.class)); } } From 97903b992e664cf46214757d727ba23c5e946131 Mon Sep 17 00:00:00 2001 From: Charles Munger Date: Thu, 27 May 2021 13:30:29 -0700 Subject: [PATCH 3/3] Address review comments --- .../bytebuddy/SubclassBytecodeGenerator.java | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java b/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java index 985823f39a..edb9640afb 100644 --- a/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java +++ b/src/main/java/org/mockito/internal/creation/bytebuddy/SubclassBytecodeGenerator.java @@ -94,8 +94,7 @@ private static boolean needsSamePackageClassLoader(MockFeatures features) { if (!Modifier.isPublic(features.mockedType.getModifiers()) || !features.mockedType.isInterface()) { // The mocked type is package private or is not an interface and thus may contain - // package - // private methods. + // package private methods. return true; } for (Class iface : features.interfaces) { @@ -110,10 +109,11 @@ private static boolean needsSamePackageClassLoader(MockFeatures features) { public Class mockClass(MockFeatures features) { MultipleParentClassLoader.Builder loaderBuilder = new MultipleParentClassLoader.Builder() - .append(features.mockedType) - .append(features.interfaces) - .append(MockAccess.class, DispatcherDefaultingToRealMethod.class) - .append( + .appendMostSpecific(features.mockedType) + .appendMostSpecific(features.interfaces) + .appendMostSpecific( + MockAccess.class, DispatcherDefaultingToRealMethod.class) + .appendMostSpecific( MockMethodInterceptor.class, MockMethodInterceptor.ForHashCode.class, MockMethodInterceptor.ForEquals.class); @@ -134,7 +134,7 @@ public Class mockClass(MockFeatures features) { } } if (shouldIncludeContextLoader) { - loaderBuilder = loaderBuilder.append(contextLoader); + loaderBuilder = loaderBuilder.appendMostSpecific(contextLoader); } ClassLoader classLoader = loaderBuilder.build(MockMethodInterceptor.class.getClassLoader());