"The type is not public and its mock class is loaded by a different class loader" with a context classloader that delegates #2303
Comments
|
Rafael, I took a look at this code and it wasn't completely clear to my why we are failing here. The reason that is probably happening is that Mockito first adds the mocked type ( PackagePrivate before adding the current classloader: So it looks up the classloader for the mocked type, which is the old classloader. Then it looks up the current classloader (which you changed) and then determines that they don't match. Thus This code landed as part of supporting Java 9+ modules: #1582 It wouldn't surprise me if there is an edge case we are missing. Since the new classloader delegates to the former, I would have expected the classes to match and thus pass the checks, but they don't. Presumably because of the classloader equality check. Can we be lenient here and allow for child classloaders to be valid as well? |
|
The reason we fail is that a package-private type is not visible outside of its own class loader. Even if we defined the subclass in the same nominal package, if it was loaded by a different class loader, it could not see its superclass. To avoid this visibility issue, we fail before even attempting to create the mock. We could consider to drop the context class loader if a mocked class is package-private. I am not sure why we need the context class loader to begin with but I assume its related to serialization what tends to be an edge case. |
|
I can think of a few options:
|
|
I'll mess around a bit more and report back if I figure out what the issue is. |
|
OK, I have a the minimal fix (as in, it will only affect cases that would have broken before): If the mocked type is a non-interface or non-public, or any of the extra interfaces are non-public, don't include the context classloader in MultipleParentsClassLoader iff the context classloader is a child of the classloader we'd use if it wasn't included. This fixes my issue, and in general likely fixes cases where a context classloader is set that follows the parent-first delegation pattern. I thought about simply removing the context loader entirely, but test authors who hit this can set and unset it in a try-finally block around their mock calls if necessary, and since there's no comments about why it's included I'd be wary of breaking somebody depending on the existing behavior. I am going to run this fix through google's internal suite of tests to see if it breaks anything. |
I think the right approach is to exclude the context loader for package-private classes by default since it will never work and to fail if serialization is enabled in addition. |
It's not just package-private classes - mocking a public non-final-non-interface class also poses problems, since that will break stubbing/verification of package-private methods (#796). My proposed fix: 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;
}
...
MultipleParentClassLoader.Builder loaderBuilder = new MultipleParentClassLoader.Builder()
.appendMostSpecific(getAllTypes(features.mockedType))
.appendMostSpecific(features.interfaces)
.appendMostSpecific(MockAccess.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.appendMostSpecific(contextLoader);
}
ClassLoader classLoader = loaderBuilder.build();The focus was on only changing behavior for cases that failed before. Since we verify that the classloader we define in is a parent of the context classloader, we shouldn't see any problems with serialization, right? Or at least no new problems, as the existing |
|
@charlesmunger Do you mind opening a PR that includes your proposed fix and adds a regression test for your use case? If all tests pass, I am inclined to merge as-is. If we end up breaking anybody else, we should add regression tests for their use cases and modify the implementation accordingly. |
This should only affect cases that previously would have thrown an exception. Fixes mockito#2303
Repro case:
Error:
If mockito actually tried to use the classloader, I think it would work. Since we don't see an interesting exception deep in the stack, my guess is that mockito is doing some extra validation to avoid generating bytecode it thinks would fail.
The text was updated successfully, but these errors were encountered: