sbglasius commented on code in PR #16296: URL: https://github.com/apache/grails-core/pull/16296#discussion_r3965393393
########## 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: You're right that the old text was inaccurate, and for exactly the reason you give — the anonymous `UserDetails` resolves to the interface method and nothing is widened. I've taken your wording as-is, and `54.2` now quotes it verbatim. Since the guide and the code can drift apart silently, I also added a check to the verification step that extracts the `LOG.warn` string from the class, normalises the two `{}` placeholders, and diffs it against the quoted block in the guide. It matches. ########## 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: Rewritten in `60598510cf`. The section now opens on the Groovy codegen change — Groovy 4 stamped `ACC_PUBLIC` on anonymous inner classes, Groovy 5 does not — says explicitly that nothing in the application changed, only the modifier the compiler emits, and then says Grails adds compatibility handling and reports each class so the code can be corrected. I also added a NOTE that ordinary Groovy code is unaffected: property access and method calls through the metaclass still work on such an object from another package, as does the JDK `Introspector`; only raw reflection against the method as the non-public class declares it fails, which is what bean marshalling was doing. Verified both, see my top-level comment. The `serialVersionUID` fix (#16295) kept its own paragraph, since that one is unrelated to Groovy 5 and hits practically every `Serializable` bean. -- 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]
