sbglasius commented on code in PR #16296:
URL: https://github.com/apache/grails-core/pull/16296#discussion_r3916991229
##########
grails-converters/src/main/groovy/org/grails/web/converters/marshaller/json/GenericJavaBeanMarshaller.java:
##########
@@ -64,14 +66,17 @@ public void marshalObject(Object o, JSON json) throws
ConverterException {
if (Modifier.isStatic(readMethod.getModifiers())) continue;
if (readMethod.getAnnotation(PersistenceMethod.class) !=
null) continue;
if (readMethod.getAnnotation(ControllerMethod.class) !=
null) continue;
- Object value = readMethod.invoke(o, (Object[]) null);
+ Method invokableMethod =
ClassUtils.getInterfaceMethodIfPossible(readMethod, clazz);
Review Comment:
Yes, it does, but only for non-interface classes. The full answer goes like
this:
```
== beans.PkgWithInterface (class public: false)
raw read method canAccess from this package: false
getInterfaceMethodIfPossible -> beans.Public (same object as raw: false)
resolved canAccess BEFORE makeAccessible: true <- already accessible
resolved canAccess AFTER makeAccessible: true
the raw class-declared method still canAccess: false <- untouched
== beans.PkgNoInterface (class public: false)
getInterfaceMethodIfPossible -> beans.PkgNoInterface (same object as raw:
true)
resolved canAccess BEFORE makeAccessible: false
resolved canAccess AFTER makeAccessible: true <- flag flipped
a later BeanUtils+resolve sees it accessible: true (same object: true)
a FRESH getDeclaredMethod copy canAccess: false <- not globally opened
```
* Interface case (the reported bug — anonymous `UserDetails`):
`getInterfaceMethodIfPossible` hands back a different Method, the interface's,
which is public-on-public. `ReflectionUtils.makeAccessible` short-circuits and
*nothing is mutated.* The class's own read method stays inaccessible.
* No-interface case (package-private class, public getter, nothing to
resolve to): yes, `setAccessible(true)` fires and it persists. Two bounds on
how far:
* The flag lives on the Method instance, not on class metadata.
`BeanUtils.getPropertyDescriptors` is backed by the static
`CachedIntrospectionResults` cache and returns the same instance every call
(confirmed above), so anything else in the JVM that asks Spring for that
class's descriptors gets an already-invokable method, for the lifetime of that
cache.
* A fresh `getDeclaredMethod`/`getMethod` copy is still false. The member
is not globally opened, and this grants nothing a caller couldn't get itself —
any classpath code in the unnamed module can `setAccessible` a public method of
a non-public class. It's also the pattern Spring itself uses on cached members
(`AutowiredAnnotationBeanPostProcessor`, `AbstractNestablePropertyAccessor`).
If you'd rather not mutate shared cached state at all, the clean alternative
is to work on our own copy — `clazz.getDeclaredMethod(name)` returns a new
instance per call (also confirmed above), so `setAccessible` on that leaks
nowhere. Cost is one extra reflective lookup per property per marshal unless we
cache it ourselves.
--
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]
