sbglasius commented on code in PR #16296: URL: https://github.com/apache/grails-core/pull/16296#discussion_r3965396987
########## 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: Confirmed — I reproduced your table before changing anything, and it is exactly right. `Introspector` hands back `PublicThing.getName` and `PublicCovariantBase.getValue`, both already accessible, so those two features asserted nothing about the code they were named for. Fixed in `60598510cf`: - `readMethodOf` now uses `BeanUtils.getPropertyDescriptors`, with a comment saying why, so the next person does not helpfully switch it back to `Introspector`. - The covariant feature is re-pointed: with `getPubliclyAccessibleMethodIfPossible` that shape resolves to the public superclass, so it now asserts *that* rather than implying widening. - New fixture `HiddenCovariantThing` — a package-private covariant override of a package-private base, so there is no public type anywhere. That is the only remaining shape that reaches the `getDeclaredMethod` override-versus-bridge choice, and it was uncovered. - Both marshaller specs now marshal that shape end to end; the XML one asserts `xml.count('<tag>') == 1` so a bridge cannot quietly add a second node. The check that these are no longer vacuous: stubbing `resolveInvokableReadMethod` to `return readMethod` now fails **four** features. Before this commit it failed two. ########## 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: Took your second option — narrowed the guide rather than changing the behaviour, in `60598510cf`. 54.1 now reads: > A `public` *field* that cannot be read at all — because its class is in a named module that does not open its package — is skipped, and the rest of the object still converts. A *getter* in that same situation is not skipped: the conversion fails, as it did before Grails 8. The reasoning for keeping them different: a field in that situation was already being skipped by the old `canAccess` gate, so skipping preserves its behaviour, whereas a getter has always failed — and silently dropping a declared property from a response body seems worse than an error, since the client gets a payload missing a field with no signal. Happy to make it fail-soft instead if you'd rather have consistency. -- 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]
