papegaaij commented on code in PR #1577:
URL: https://github.com/apache/wicket/pull/1577#discussion_r3961475632
##########
wicket-ioc/src/main/java/org/apache/wicket/proxy/bytebuddy/ByteBuddyProxyFactory.java:
##########
@@ -17,8 +17,10 @@
package org.apache.wicket.proxy.bytebuddy;
import java.io.Serializable;
+import java.lang.invoke.MethodHandles;
import java.lang.reflect.Constructor;
import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Modifier;
Review Comment:
`Modifier` is not needed once the branch is on the package rather than on
the visibility.
```suggestion
```
##########
wicket-ioc/src/main/java/org/apache/wicket/proxy/bytebuddy/ByteBuddyProxyFactory.java:
##########
@@ -116,10 +120,26 @@ public static <T> Class<T> createOrGetProxyClass(Class<T>
type)
.implement(InterceptorMutator.class).intercept(FieldAccessor.ofBeanProperty())
.implement(Serializable.class,
IWriteReplace.class,
ILazyInitProxy.class).intercept(MethodDelegation.toField(INTERCEPTOR_FIELD_NAME))
.make()
- .load(classLoader,
ClassLoadingStrategy.Default.INJECTION.allowExistingTypes())
+ .load(classLoader,
loadingStrategy)
.getLoaded());
Review Comment:
The monitor argument makes class creation for one `(loader, type)` pair
mutually exclusive. Without it, `UsingLookup` has no `allowExistingTypes()`
equivalent and concurrent first use of a package-private type defines the class
twice — WICKET-7005 again.
```suggestion
.load(classLoader,
resolveLoadingStrategy(type))
.getLoaded(),
DYNAMIC_CLASS_CACHE);
```
##########
wicket-ioc/src/main/java/org/apache/wicket/proxy/bytebuddy/ByteBuddyProxyFactory.java:
##########
@@ -100,8 +102,10 @@ public <T> T createProxy(final Class<T> type, final
IProxyTargetLocator locator)
@SuppressWarnings("unchecked")
public static <T> Class<T> createOrGetProxyClass(Class<T> type)
- {
+ {
+ ClassLoadingStrategy<ClassLoader> loadingStrategy =
resolveLoadingStrategy(type);
ClassLoader classLoader = resolveClassLoader();
+
Review Comment:
Resolving the strategy here runs `privateLookupIn` on every `createProxy`
call, not just on a cache miss, and makes it throw even when the proxy class is
already cached. Moving it into the lambda also lets the trailing whitespace go.
```suggestion
{
ClassLoader classLoader = resolveClassLoader();
```
##########
wicket-ioc/src/main/java/org/apache/wicket/proxy/bytebuddy/ByteBuddyProxyFactory.java:
##########
@@ -205,7 +225,9 @@ private static boolean hasNoArgConstructor(Class<?> type)
for (Constructor<?> constructor :
type.getDeclaredConstructors())
{
if (constructor.getParameterTypes().length == 0)
+ {
Review Comment:
Unrelated to the fix — worth dropping to keep the commit to one thing.
##########
wicket-ioc/src/main/java/org/apache/wicket/proxy/bytebuddy/ByteBuddyProxyFactory.java:
##########
@@ -116,10 +120,26 @@ public static <T> Class<T> createOrGetProxyClass(Class<T>
type)
.implement(InterceptorMutator.class).intercept(FieldAccessor.ofBeanProperty())
.implement(Serializable.class,
IWriteReplace.class,
ILazyInitProxy.class).intercept(MethodDelegation.toField(INTERCEPTOR_FIELD_NAME))
.make()
- .load(classLoader,
ClassLoadingStrategy.Default.INJECTION.allowExistingTypes())
+ .load(classLoader,
loadingStrategy)
.getLoaded());
}
+ private static ClassLoadingStrategy<ClassLoader>
resolveLoadingStrategy(Class<?> type)
+ {
+ try
+ {
+ int modifiers = type.getModifiers();
+
+ return Modifier.isPublic(modifiers)
+ ?
ClassLoadingStrategy.Default.WRAPPER.allowExistingTypes()
+ :
ClassLoadingStrategy.UsingLookup.of(MethodHandles.privateLookupIn(type,
MethodHandles.lookup()));
+ }
+ catch (IllegalAccessException e)
+ {
+ throw new WicketRuntimeException(e);
+ }
+ }
Review Comment:
This is the core of it: branch on whether the proxy keeps the type's
package, not on the type's visibility. Only a `java.*` proxy is renamed into
another package by `WicketNamingStrategy`, and that is the only case that needs
a class loader of its own — `privateLookupIn` cannot serve it anyway, since
`java.base` does not open its packages.
The message on the exception matters here: when it fires, it is because a
named module does not `open` its package to wicket-ioc, and a bare
`IllegalAccessException` gives the user nothing to act on.
```suggestion
/**
* The proxy has to be defined in the same runtime package as the type
it proxies, or the
* package private methods it overrides are not overridden at all. Only
a proxy for a
* <em>java.**</em> type is renamed into another package, and needs a
class loader of its own.
*/
private static ClassLoadingStrategy<ClassLoader>
resolveLoadingStrategy(Class<?> type)
{
if (type.getName().startsWith("java."))
{
return
ClassLoadingStrategy.Default.WRAPPER.allowExistingTypes();
}
try
{
return ClassLoadingStrategy.UsingLookup
.of(MethodHandles.privateLookupIn(type,
MethodHandles.lookup()));
}
catch (IllegalAccessException e)
{
throw new WicketRuntimeException("Cannot create a proxy
for " + type.getName()
+ ", because its package is not open to " +
ByteBuddyProxyFactory.class.getModule(), e);
}
}
```
##########
pom.xml:
##########
@@ -159,7 +159,7 @@
<aspectj.version>1.9.25.1</aspectj.version>
<assertj-core.version>3.27.7</assertj-core.version>
<bouncycastle.version>1.85.2</bouncycastle.version>
- <byte-buddy.version>1.18.8</byte-buddy.version>
+ <byte-buddy.version>1.18.13</byte-buddy.version>
Review Comment:
This bump is separable from the fix: 1.18.13 does not disable `INJECTION` (I
checked on JDK 25), so it is not what forces the change. No objection to
bumping, but it reads as part of the fix here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
