jdaugherty commented on code in PR #16296:
URL: https://github.com/apache/grails-core/pull/16296#discussion_r3963406381
##########
grails-doc/src/en/guide/upgrading/upgrading80x.adoc:
##########
@@ -3097,3 +3097,70 @@ grails {
imported whether or not they are present — a star import of an absent package
contributes no classes and is
not an error in Groovy, so the probe changed nothing it could observe. An
application that relied on the
import being *omitted* when the package was absent sees no difference in
compiled output.
+
+==== 54. Non-Public Bean Classes Are Marshalled, and Reported Once
+
+`someObject as JSON` (and `as XML`) previously failed when the object's class
was not `public` — an
Review Comment:
This paragraph reads like it was broken since upgrading to Grails 7 - but
it's only broken as of upgrading to Groovy 5. We should call out the Groovy 5
change that made this error, and explain that Grails adds this workaround (with
a warning to update your code).
##########
grails-common/src/main/groovy/org/apache/grails/common/reflect/ReflectionUtils.java:
##########
@@ -0,0 +1,196 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.grails.common.reflect;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.InaccessibleObjectException;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.jspecify.annotations.Nullable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.springframework.util.ClassUtils;
+
+/**
+ * Reads the properties of a bean whose class is not {@code public}.
+ *
+ * <p>A class that is not public -- anonymous, local, or package-private --
cannot have its read
+ * methods invoked from another package even when the methods themselves are
public, so reflection
+ * over such a bean needs help. Groovy 4 stamped {@code ACC_PUBLIC} on
anonymous inner classes and
+ * Groovy 5 does not, which is why beans of that shape reach the framework at
all.
+ *
+ * <p>This compatibility handling is deliberately visible: {@link
#warnOnNonPublicClass} reports the
+ * class once so an application can be corrected, and so the handling can be
withdrawn if the
+ * compiler stops producing non-public classes for this shape.
+ *
+ * @since 8.0.0
+ */
+public final class ReflectionUtils {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(ReflectionUtils.class);
+
+ /** Best-effort upper bound, so that generated class names cannot grow the
set without limit. */
+ private static final int MAX_WARNED_CLASSES = 1024;
+
+ /**
+ * Keyed by class name rather than by {@link Class}, so that reporting a
class never retains its
+ * class loader. A reload therefore does not report the same class again,
which is intended: a
+ * reload loop would otherwise repeat every warning.
+ */
+ private static final Set<String> WARNED_NON_PUBLIC_CLASSES =
ConcurrentHashMap.newKeySet();
+
+ /**
+ * Packages whose classes an application cannot declare public itself, so
reporting them would
+ * only be noise. {@code java.util.KeyValueHolder}, handed out by {@code
Map.entry}, is the one
+ * that turns up in practice.
+ *
+ * <p>The {@code grails} packages are deliberately absent: a framework
class reaching this code
+ * is worth seeing, and the tests declare their fixtures under {@code
org.grails}.
+ */
+ private static final String[] NON_APPLICATION_PACKAGES = {
+ "java.", "javax.", "jakarta.", "groovy.", "org.apache.groovy.",
"org.codehaus.groovy.",
+ "org.springframework."
+ };
+
+ private ReflectionUtils() {
+ }
+
+ /**
+ * Resolves a read method that can actually be invoked on {@code target}.
+ *
+ * <p>Where the property is declared by an interface, the interface method
is returned: it is
+ * declared by an accessible type, and virtual dispatch still reaches the
implementation. Where
+ * it is not -- a non-public class with no interface declaring the getter
-- a private copy of
+ * the method is widened, so that the accessibility flag cannot leak into
a {@code Method}
+ * instance shared through a descriptor cache.
+ *
+ * @param readMethod the property's read method, typically from a {@code
PropertyDescriptor}
+ * @param targetClass the class being read, used to resolve the interface
method
+ * @param target the instance being read, or {@code null} for a
static read method
+ * @return a method that may be invoked on {@code target}
+ */
+ public static Method resolveInvokableReadMethod(Method readMethod,
Class<?> targetClass, @Nullable Object target)
+ throws NoSuchMethodException {
+ Method invokable = ClassUtils.getInterfaceMethodIfPossible(readMethod,
targetClass);
Review Comment:
`ClassUtils.getPubliclyAccessibleMethodIfPossible(readMethod, targetClass)`
is in the spring-core we pin and does this plus the superclass walk: it returns
the first equivalent method declared on a public type, interface or class. For
`CovariantThing` (package-private, extends the public `PublicCovariantBase`)
the interface-only resolver finds nothing and this method widens a copy; the
publicly-accessible one returns `PublicCovariantBase.getValue`, `canAccess` is
true, and `invoke` still dispatches to the override. I checked this with a
probe against spring-core 7.0.9.
That confines the `setAccessible` path to getters with no public declaring
type anywhere in the hierarchy, which is where the non-standard handling
belongs, and it makes the override-vs-bridge `getDeclaredMethod` below rarer.
##########
grails-common/src/test/groovy/org/apache/grails/common/reflect/ReflectionUtilsSpec.groovy:
##########
@@ -0,0 +1,137 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.grails.common.reflect
+
+import java.beans.Introspector
+import java.beans.PropertyDescriptor
+import java.lang.reflect.Field
+import java.lang.reflect.Method
+import java.lang.reflect.Modifier
+
+import org.apache.grails.common.reflect.beans.PublicFieldBean
+import org.apache.grails.common.reflect.beans.PublicThing
+import org.apache.grails.common.reflect.beans.ThingFactory
+
+import spock.lang.Specification
+
+/**
+ * Covers reading a bean whose class is not public. The fixtures deliberately
live in another
+ * package: in this one the JVM access check passes and nothing would need
widening.
+ */
+class ReflectionUtilsSpec extends Specification {
+
+ void setup() {
+ ReflectionUtils.resetWarnedClasses()
+ }
+
+ void cleanup() {
+ ReflectionUtils.resetWarnedClasses()
+ }
+
+ private static Method readMethodOf(Object bean, String property) {
+ PropertyDescriptor descriptor =
Introspector.getBeanInfo(bean.getClass()).propertyDescriptors
Review Comment:
`java.beans.Introspector` already resolves a read method to its publicly
accessible declaration, so two of these features never reach the branch they
are named for. I compiled the fixtures and compared what each introspector
hands back:
| Fixture | `Introspector` read method | `BeanUtils.getPropertyDescriptors`
read method |
|---|---|---|
| `anonymousThing` | `PublicThing.getName`, accessible | the anonymous
class, not accessible |
| `covariantThing` | `PublicCovariantBase.getValue`, accessible |
`CovariantThing.getValue`, not accessible |
| `standaloneThing` | `StandaloneThing.getName`, not accessible | same |
So in "declared by a public interface needs no widening" and "covariant read
method resolves to the override rather than the bridge",
`resolveInvokableReadMethod` returns its argument unchanged and both would pass
with `return readMethod`. Only the standalone feature exercises widening, and
nothing exercises the `getDeclaredMethod` override-vs-bridge selection at all,
because the covariant shape is not in the converters specs either.
Please take the read methods from `BeanUtils.getPropertyDescriptors` here,
which is what the marshallers use and which returns the declaring-class method
(spring-beans is already on this module's classpath through spring-context),
and add the covariant shape to one of the marshaller specs so the bridge
selection is covered end to end. Codecov's 72% on this class lines up with the
two vacuous features.
##########
grails-common/src/main/groovy/org/apache/grails/common/reflect/ReflectionUtils.java:
##########
@@ -0,0 +1,196 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.grails.common.reflect;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.InaccessibleObjectException;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.jspecify.annotations.Nullable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.springframework.util.ClassUtils;
+
+/**
+ * Reads the properties of a bean whose class is not {@code public}.
+ *
+ * <p>A class that is not public -- anonymous, local, or package-private --
cannot have its read
+ * methods invoked from another package even when the methods themselves are
public, so reflection
+ * over such a bean needs help. Groovy 4 stamped {@code ACC_PUBLIC} on
anonymous inner classes and
+ * Groovy 5 does not, which is why beans of that shape reach the framework at
all.
+ *
+ * <p>This compatibility handling is deliberately visible: {@link
#warnOnNonPublicClass} reports the
+ * class once so an application can be corrected, and so the handling can be
withdrawn if the
+ * compiler stops producing non-public classes for this shape.
+ *
+ * @since 8.0.0
+ */
+public final class ReflectionUtils {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(ReflectionUtils.class);
+
+ /** Best-effort upper bound, so that generated class names cannot grow the
set without limit. */
+ private static final int MAX_WARNED_CLASSES = 1024;
+
+ /**
+ * Keyed by class name rather than by {@link Class}, so that reporting a
class never retains its
+ * class loader. A reload therefore does not report the same class again,
which is intended: a
+ * reload loop would otherwise repeat every warning.
+ */
+ private static final Set<String> WARNED_NON_PUBLIC_CLASSES =
ConcurrentHashMap.newKeySet();
+
+ /**
+ * Packages whose classes an application cannot declare public itself, so
reporting them would
+ * only be noise. {@code java.util.KeyValueHolder}, handed out by {@code
Map.entry}, is the one
+ * that turns up in practice.
+ *
+ * <p>The {@code grails} packages are deliberately absent: a framework
class reaching this code
+ * is worth seeing, and the tests declare their fixtures under {@code
org.grails}.
+ */
+ private static final String[] NON_APPLICATION_PACKAGES = {
+ "java.", "javax.", "jakarta.", "groovy.", "org.apache.groovy.",
"org.codehaus.groovy.",
+ "org.springframework."
+ };
+
+ private ReflectionUtils() {
+ }
+
+ /**
+ * Resolves a read method that can actually be invoked on {@code target}.
+ *
+ * <p>Where the property is declared by an interface, the interface method
is returned: it is
+ * declared by an accessible type, and virtual dispatch still reaches the
implementation. Where
+ * it is not -- a non-public class with no interface declaring the getter
-- a private copy of
+ * the method is widened, so that the accessibility flag cannot leak into
a {@code Method}
+ * instance shared through a descriptor cache.
+ *
+ * @param readMethod the property's read method, typically from a {@code
PropertyDescriptor}
+ * @param targetClass the class being read, used to resolve the interface
method
+ * @param target the instance being read, or {@code null} for a
static read method
+ * @return a method that may be invoked on {@code target}
+ */
+ public static Method resolveInvokableReadMethod(Method readMethod,
Class<?> targetClass, @Nullable Object target)
+ throws NoSuchMethodException {
+ Method invokable = ClassUtils.getInterfaceMethodIfPossible(readMethod,
targetClass);
+ if (canAccess(invokable, target)) {
+ return invokable;
+ }
+ // getDeclaredMethod cannot fail here: invokable was resolved from
this very class's methods.
+ Method widened = invokable.getDeclaringClass()
+ .getDeclaredMethod(invokable.getName(),
invokable.getParameterTypes());
+ widened.setAccessible(true);
Review Comment:
`tryMakeReadable` below catches `InaccessibleObjectException` and skips the
field, but this `setAccessible` is left to throw, so a getter on a non-public
class in a named module that does not open its package still fails the whole
conversion (the marshallers wrap it as `ConverterException`, the same as before
this PR). Section 54.1 of the upgrade guide says fields in that situation are
skipped rather than failing the conversion, which reads as if the conversion
survives; with a getter in the same module it will not. Either skip the
property here the same way, or narrow that sentence in the guide. Low priority
since it is not a regression.
##########
grails-common/src/main/groovy/org/apache/grails/common/reflect/ReflectionUtils.java:
##########
@@ -0,0 +1,196 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.grails.common.reflect;
+
+import java.lang.reflect.Field;
+import java.lang.reflect.InaccessibleObjectException;
+import java.lang.reflect.Method;
+import java.lang.reflect.Modifier;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+
+import org.jspecify.annotations.Nullable;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.springframework.util.ClassUtils;
+
+/**
+ * Reads the properties of a bean whose class is not {@code public}.
+ *
+ * <p>A class that is not public -- anonymous, local, or package-private --
cannot have its read
+ * methods invoked from another package even when the methods themselves are
public, so reflection
+ * over such a bean needs help. Groovy 4 stamped {@code ACC_PUBLIC} on
anonymous inner classes and
+ * Groovy 5 does not, which is why beans of that shape reach the framework at
all.
+ *
+ * <p>This compatibility handling is deliberately visible: {@link
#warnOnNonPublicClass} reports the
+ * class once so an application can be corrected, and so the handling can be
withdrawn if the
+ * compiler stops producing non-public classes for this shape.
+ *
+ * @since 8.0.0
+ */
+public final class ReflectionUtils {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(ReflectionUtils.class);
+
+ /** Best-effort upper bound, so that generated class names cannot grow the
set without limit. */
+ private static final int MAX_WARNED_CLASSES = 1024;
+
+ /**
+ * Keyed by class name rather than by {@link Class}, so that reporting a
class never retains its
+ * class loader. A reload therefore does not report the same class again,
which is intended: a
+ * reload loop would otherwise repeat every warning.
+ */
+ private static final Set<String> WARNED_NON_PUBLIC_CLASSES =
ConcurrentHashMap.newKeySet();
+
+ /**
+ * Packages whose classes an application cannot declare public itself, so
reporting them would
+ * only be noise. {@code java.util.KeyValueHolder}, handed out by {@code
Map.entry}, is the one
+ * that turns up in practice.
+ *
+ * <p>The {@code grails} packages are deliberately absent: a framework
class reaching this code
+ * is worth seeing, and the tests declare their fixtures under {@code
org.grails}.
+ */
+ private static final String[] NON_APPLICATION_PACKAGES = {
+ "java.", "javax.", "jakarta.", "groovy.", "org.apache.groovy.",
"org.codehaus.groovy.",
+ "org.springframework."
+ };
+
+ private ReflectionUtils() {
+ }
+
+ /**
+ * Resolves a read method that can actually be invoked on {@code target}.
+ *
+ * <p>Where the property is declared by an interface, the interface method
is returned: it is
+ * declared by an accessible type, and virtual dispatch still reaches the
implementation. Where
+ * it is not -- a non-public class with no interface declaring the getter
-- a private copy of
+ * the method is widened, so that the accessibility flag cannot leak into
a {@code Method}
+ * instance shared through a descriptor cache.
+ *
+ * @param readMethod the property's read method, typically from a {@code
PropertyDescriptor}
+ * @param targetClass the class being read, used to resolve the interface
method
+ * @param target the instance being read, or {@code null} for a
static read method
+ * @return a method that may be invoked on {@code target}
+ */
+ public static Method resolveInvokableReadMethod(Method readMethod,
Class<?> targetClass, @Nullable Object target)
+ throws NoSuchMethodException {
+ Method invokable = ClassUtils.getInterfaceMethodIfPossible(readMethod,
targetClass);
+ if (canAccess(invokable, target)) {
+ return invokable;
+ }
+ // getDeclaredMethod cannot fail here: invokable was resolved from
this very class's methods.
+ Method widened = invokable.getDeclaringClass()
+ .getDeclaredMethod(invokable.getName(),
invokable.getParameterTypes());
+ widened.setAccessible(true);
+ return widened;
+ }
+
+ /**
+ * Makes an instance field readable, widening it only when it is not
readable already.
+ *
+ * <p>The field is expected to have come from {@link
Class#getDeclaredFields}, which hands out a
+ * fresh copy on every call, so widening it is confined to the caller's
own copy.
+ *
+ * @param field an instance field
+ * @param target the instance being read
+ * @return {@code false} when the field cannot be read, in which case the
caller should skip it
+ * rather than fail the whole read
+ */
+ public static boolean tryMakeReadable(Field field, @Nullable Object
target) {
+ if (canAccess(field, target)) {
+ return true;
+ }
+ try {
+ field.setAccessible(true);
+ return true;
+ }
+ catch (InaccessibleObjectException | SecurityException e) {
+ // The declaring class is in a named module that does not open its
package to us
+ if (LOG.isDebugEnabled()) {
+ LOG.debug("Cannot read field [{}] of [{}]: {}",
+ field.getName(), field.getDeclaringClass().getName(),
e.getMessage());
+ }
+ return false;
+ }
+ }
+
+ /**
+ * Reports {@code clazz} if it is not public, at most once per class.
+ *
+ * <p>Synthetic classes and classes from the JDK, Groovy and Spring are
not reported: they are
+ * not written by the application whose log this is, so nobody reading it
can act on them.
+ *
+ * <p>The bookkeeping deliberately runs before the log level is consulted,
so that "once" does
+ * not depend on how logging happens to be configured.
+ *
+ * @return whether this call was the one that reported the class
+ */
+ public static boolean warnOnNonPublicClass(@Nullable Class<?> clazz) {
+ if (clazz == null || Modifier.isPublic(clazz.getModifiers()) ||
!isApplicationClass(clazz)) {
+ return false;
+ }
+ if (WARNED_NON_PUBLIC_CLASSES.size() >= MAX_WARNED_CLASSES ||
+ !WARNED_NON_PUBLIC_CLASSES.add(clazz.getName())) {
+ return false;
+ }
+ if (LOG.isWarnEnabled()) {
+ LOG.warn("Class [{}] is not public, so its properties can only be
read by widening access reflectively. " +
Review Comment:
The point of this warning is to get people to change their code so the class
is public, so the text has to be accurate about why. As written it says the
properties "can only be read by widening access reflectively", which is not
true for the shape that opened #16294: the anonymous `UserDetails` resolves to
the interface method in `resolveInvokableReadMethod` and is invoked like any
other public method, nothing is widened. Since the trigger is the class shape
rather than the mechanism, describe the shape:
```
Class [{}] is not public. Grails reads its properties through compatibility
handling that may be withdrawn in a future major release. Declare it as a named
public class so that it reads as a standard JavaBean, or register an
ObjectMarshaller for it if the class is not yours. To silence this, set the log
level of [{}] above WARN. (warned once per class)
```
The `ObjectMarshaller` clause matters because `NON_APPLICATION_PACKAGES` is
a short list; a non-public bean from any other library gets reported to someone
who cannot declare it public. The upgrade guide quotes this message verbatim in
54.2, so it needs the same edit.
--
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]
