Fix marshalling of non-public classes and Serializable beans

[sbglasius]

Fixes #16294
Fixes #16295

someObject as JSON (and as XML) failed for two very common shapes of object. Both are fixed here, since fixing the first one only exposes the second.

#16294 — IllegalAccessException on a non-public class

A class that is not public — anonymous, local or package-private — cannot have its read methods invoked reflectively from another package, even though the methods themselves carry the public modifier. All four bean marshallers called readMethod.invoke(...) with no accessibility handling, so handing as JSON an anonymous implementation of a public interface (the usual shape of a Spring Security UserDetails) failed with:

java.lang.IllegalAccessException: class org.grails.web.converters.marshaller.json.GroovyBeanMarshaller
    cannot access a member of class com.example.DemoController$1 with modifiers "public"

The read method is now resolved to the interface method where one exists, and made accessible otherwise:

Method invokableMethod = ClassUtils.getInterfaceMethodIfPossible(readMethod, clazz);
ReflectionUtils.makeAccessible(invokableMethod);
Object value = invokableMethod.invoke(o, (Object[]) null);

Resolving to the interface is not sufficient on its own — a package-private class with a public getter and no interface has nowhere to resolve to — so makeAccessible carries that case.

The public field loops of both GroovyBeanMarshallers failed identically on field.get(o) and are now made accessible too.

#16295 — IllegalArgumentException on any Serializable bean

GenericJavaBeanMarshaller evaluated field.canAccess(o) before the static check, and per its javadoc canAccess throws for a static member when the object is non-null. Any bean declaring private static final long serialVersionUID — nearly every Serializable bean — therefore failed:

java.lang.IllegalArgumentException: non-null object for
    private static final long org.springframework.security.core.authority.SimpleGrantedAuthority.serialVersionUID

The modifier checks now run first so they short-circuit before canAccess. This is a regression in the 8.x line from 9e60b8a4de, which mechanically swapped the non-throwing isAccessible() for canAccess(o).

One change beyond the two issues

Groovy compiles the variables captured by an anonymous class into ACC_PUBLIC | ACC_SYNTHETIC groovy.lang.Reference fields. Once the field loops could actually read them, they were emitted as duplicate keys with empty-object values ({"name":{},"age":{}}) — the new tests caught exactly that. Synthetic fields are compiler artifacts and never part of a bean's state, so they are now skipped in all four marshallers.

Files changed

• grails-converters/.../marshaller/json/GroovyBeanMarshaller.java
• grails-converters/.../marshaller/json/GenericJavaBeanMarshaller.java
• grails-converters/.../marshaller/xml/GroovyBeanMarshaller.java
• grails-converters/.../marshaller/xml/GenericJavaBeanMarshaller.java

Tests

New fixtures under org.grails.web.converters.beans, deliberately in a different package from the marshallers — in the same package the JVM access check passes and neither bug reproduces. They cover a public interface with both abstract and default read methods (mirroring UserDetails), Groovy and Java anonymous / package-private / no-interface implementations, an anonymous class with a declared public field, and a Serializable bean carrying private static final long serialVersionUID alongside a public constant and a public instance field.

Two new specs of 9 features each, driven through the public new JSON(x) / new XML(x) API: json.NonPublicClassMarshallingSpec and xml.NonPublicClassMarshallingSpec. Every non-public case first asserts !Modifier.isPublic(person.getClass().modifiers), so the tests fail loudly rather than silently passing if a future compiler stops producing a non-public class.

With the production change reverted, 18 of 18 fail with the two reported exceptions verbatim; with it, 18/18 pass. Also green locally: full :grails-converters:test, :grails-rest-transforms:test, :grails-test-suite-web:test, :grails-test-suite-uber:test, :grails-web-common:test and :grails-converters:codeStyle.

No documentation change: this restores the documented behaviour of as JSON / as XML and adds no public API.

[codecov]

Codecov Report

❌ Patch coverage is 66.12903% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.8230%. Comparing base (7801ced) to head (e9932f3).
⚠️ Report is 2 commits behind head on 8.0.x.

Files with missing lines	Patch %	Lines
.../apache/grails/common/reflect/ReflectionUtils.java	71.7949%	6 Missing and 5 partials ⚠️
...ers/marshaller/json/GenericJavaBeanMarshaller.java	50.0000%	0 Missing and 3 partials ⚠️
...ters/marshaller/xml/GenericJavaBeanMarshaller.java	57.1429%	0 Missing and 3 partials ⚠️
...nverters/marshaller/json/GroovyBeanMarshaller.java	60.0000%	0 Missing and 2 partials ⚠️
...onverters/marshaller/xml/GroovyBeanMarshaller.java	60.0000%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##                8.0.x     #16296        +/-   ##
==================================================
+ Coverage     54.7302%   54.8230%   +0.0929%     
- Complexity      20538      20592        +54     
==================================================
  Files            2104       2105         +1     
  Lines          101149     101202        +53     
  Branches        17966      17980        +14     
==================================================
+ Hits            55359      55482       +123     
+ Misses          37772      37664       -108     
- Partials         8018       8056        +38     

Files with missing lines	Coverage Δ
...nverters/marshaller/json/GroovyBeanMarshaller.java	70.2703% <60.0000%> (+8.5056%)	⬆️
...onverters/marshaller/xml/GroovyBeanMarshaller.java	61.5385% <60.0000%> (+58.7607%)	⬆️
...ers/marshaller/json/GenericJavaBeanMarshaller.java	63.1579% <50.0000%> (+60.2167%)	⬆️
...ters/marshaller/xml/GenericJavaBeanMarshaller.java	67.8571% <57.1429%> (+63.6905%)	⬆️
.../apache/grails/common/reflect/ReflectionUtils.java	71.7949% <71.7949%> (ø)

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[bkoehm]

Question: Any need to test a Groovy bean that isn't @CompileStatic? Probably not, but I thought I'd ask the question anyway.

[sbglasius]

It doesn't make a difference if it's @CompileStatic or not. Reflection wise the class is very similar looking.

[sbglasius]

A DynamicGroovyPersonFactory was added, just to be on the safe side.

[jdaugherty]

Doesn't this make it permanently accessible and not just for this code?

[sbglasius]

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.

[jdaugherty]

I'm honestly not sure in this case. I'm hoping we can discuss in the weekly - instead of fixing this, why not force people to make those inner classes public?

[sbglasius]

@jdaugherty I don't feel we reached an agreement on the weekly?

And with regards to a case, where access is widened, is that even a problem. Every developer can do so them self, using the exact same techniques.

We could also easily go for the "not mutated" case.

IMO Grails does not have to become the framework where everything has to be according to standards.

[bkoehm]

I don't see much downside to going the "not mutated" case but I acknowledge the concern may be performance, as you put it: "Cost is one extra reflective lookup per property per marshal." I think this is an acceptable cost, but it's easy for me to say that as I don't have visibility into that potential cost for other people. I will accept that low cost. I'm a thumbs up on the "not mutated" case suggestion.

[jdaugherty]

@sbglasius is this a result of Groovy now honoring the modifiers where previously it would treat package private / protected as public?

[sbglasius]

@sbglasius is this a result of Groovy now honoring the modifiers where previously it would treat package private / protected as public?

Yes, it is because Groovy 4 stamped ACC_PUBLIC on anonymous inner classes, Groovy 5 keeps them package-private. It would actually show in Groovy 4, if a class was marked @PackageScope.

[jdaugherty]

I question if we should instead just force people to update their code to make those inner classes public. @sbglasius I think you said the plugin itself didn't do this, can't we fix that instead? Otherwise, we're encouraging bad habbits as newer versions of groovy are released. (I am ultimately ok with this change, but I'm playing devil's advocate here to ensure we're not introducing something that should just be a note in the upgrade guide).

I'm also curious what @matrei thinks on this.

[bkoehm]

I question if we should instead just force people to update their code to make those inner classes public. @sbglasius I think you said the plugin itself didn't do this, can't we fix that instead? Otherwise, we're encouraging bad habbits as newer versions of groovy are released. (I am ultimately ok with this change, but I'm playing devil's advocate here to ensure we're not introducing something that should just be a note in the upgrade guide).

I was under the impression that more than just inner classes are in play, such as anonymous classes?

[sbglasius]

@jdaugherty What @bkoehm is a good point. It's not just non-public classes that's the issue, also anonymous classes. Does that change your mind?

[matrei]

AI Review

Overview

Fixes two production-visible bugs in the four JSON/XML bean marshallers under grails-converters (#16294, #16295). The problem shapes are extremely common — Spring Security–style anonymous UserDetails implementations, any Serializable bean with serialVersionUID — so this is a real quality-of-life fix.

Three concerns are addressed:

1. Non-public read methods — readMethod.invoke(o, …) failed with IllegalAccessException when the declaring class wasn't accessible from another package, even if the method itself was public. Resolved by walking to the interface method via ClassUtils.getInterfaceMethodIfPossible, and falling back to reflectively widening a fresh copy of the method when no interface match exists.
2. Field.canAccess(o) throws on static fields with a non-null obj — the JDK contract explicitly throws IllegalArgumentException in that case, so any bean declaring private static final long serialVersionUID blew up. Fixed by short-circuiting the modifier checks before canAccess.
3. Groovy synthetic capture fields — anonymous inner classes compile enclosing variables into public synthetic Reference fields. Once the field loops can read them, they emitted as duplicate keys with empty-object values. All four marshallers now filter field.isSynthetic().

All four marshallers get symmetric treatment. New specs (NonPublicClassMarshallingSpec for JSON and XML) plus a dynamic-Groovy fixture cover the paths, and each spec has a dedicated assertion that the shared descriptor cache is not mutated.

Correctness

• Interface-method resolution: ClassUtils.getInterfaceMethodIfPossible(readMethod, clazz) never returns null (falls back to the original), and virtual dispatch on invoke(o, …) still calls the overridden concrete method. This is exactly how Spring resolves inaccessible-class calls internally.
• Static-field short-circuit: !Modifier.isStatic(modifiers) is evaluated before field.canAccess(o) in each marshaller, so the canAccess exception on static fields is avoided.
• Synthetic filter: necessary now that field access is widened in the Groovy marshallers, otherwise Groovy's Reference capture fields would leak. The XML spec explicitly asserts xml.count('<name>') == 1, which locks this in.
• Cache non-mutation (the important one for this codebase, given how many callers hit BeanUtils.getPropertyDescriptors):

  Method invokableMethod = ClassUtils.getInterfaceMethodIfPossible(readMethod, clazz);
  if (!invokableMethod.canAccess(o)) {
      // Widen a private copy, so the flag never leaks into the shared descriptor cache
      invokableMethod = invokableMethod.getDeclaringClass().getDeclaredMethod(invokableMethod.getName());
      invokableMethod.setAccessible(true);
  }

  Class.getDeclaredMethod allocates a new Method per call (via Method.copy() in the JDK), so setAccessible(true) on the copy cannot flow back to Spring's CachedIntrospectionResults. The dedicated spec method that reads the cached descriptor back after marshalling and asserts the flag is still down is the right way to lock this in.
• Fields don't need the same treatment: Class.getDeclaredFields() already returns fresh copies each call, so the ReflectionUtils.makeAccessible(field) calls in the two GroovyBeanMarshallers can't leak. Correctly identified and called out in the commit message.
• Existing safeguards preserved: PersistenceMethod / ControllerMethod skips, metaClass / class filtering, and GORM attached/errors filtering are untouched.

Suggestions

1. Java vs Groovy field-access asymmetry — worth explicitly deciding on:

   • GroovyBeanMarshaller (both): drops the field.canAccess(o) guard and unconditionally calls ReflectionUtils.makeAccessible(field) before field.get(o).
   • GenericJavaBeanMarshaller (both): keeps field.canAccess(o) and does not widen.

   Consequence: a Java anonymous class with a public field on a non-public declaring class will have that field silently skipped, while its Groovy equivalent (anonymousPersonWithPublicField) is emitted. If that asymmetry is intentional, fine — but the tests don't cover the Java-side variant, so it's easy to regress. Consider either aligning behavior (widen on both) or adding a test that asserts the current Java behavior on purpose.

2. Duplicated widening block across four files — the same five lines and the same comment appear in all four marshallers. Consider a small helper to keep the intent centralized:

   private static Method resolveInvokable(Method readMethod, Class<?> targetClass, Object target) throws NoSuchMethodException {
       Method invokable = ClassUtils.getInterfaceMethodIfPossible(readMethod, targetClass);
       if (invokable.canAccess(target)) return invokable;
       Method copy = invokable.getDeclaringClass().getDeclaredMethod(invokable.getName());
       copy.setAccessible(true);
       return copy;
   }

   Even placed as a private static in each file, it eliminates four copies of the comment. Optional.

3. Prefer readMethod.getParameterTypes() over the implicit empty array — getDeclaredMethod(invokable.getName()) works today because JavaBean getters are no-arg, but the call is subtly ambiguous to a reader who doesn't know that invariant. getDeclaredMethod(invokable.getName(), invokable.getParameterTypes()) reads as "get the exact same method" and is trivially more defensive.

4. Leak-detection test only covers the standalone-Java path — JavaPersonFactory.standalonePerson('user') is the only shape exercised. The Groovy standalone path (PackagePrivateStandaloneGroovyBean) goes through the same widening but its spec doesn't assert the same invariant. Both marshallers copy the same pattern, so this is defensible, but a matching Groovy-side assertion would prove both marshallers preserve the invariant symmetrically.

5. NoSuchMethodException propagation — the new getDeclaredMethod call adds a checked exception into the try body. Fine because the outer catch (Exception e) wraps it as ConverterException. A one-liner comment noting "cannot fail: invokable came from this class's declared methods" would help future readers who wonder whether this is a real failure surface.

6. Nit — variable name — invokableMethod reads oddly; invokable or just method would be cleaner.

Test coverage

Strong. Coverage matrix:

Shape	Groovy static	Groovy dynamic	Java
Anonymous impl of public interface	✅	✅	✅
Anonymous with public field	✅	—	—
Package-private impl of public interface	✅	—	✅
Package-private standalone (no interface)	✅	—	✅
Serializable + static fields	—	—	✅
Composite map (reproducer)	✅	—	—
Shared descriptor cache not mutated	—	—	✅

Optional additions: a Java anonymous class with a public field (to lock in the intentional asymmetry, if kept), a Groovy-side cache-non-mutation assertion, and a Serializable Groovy bean.

Risk

Low. Changes are localized to four small marshallers. On the happy path (public class, public read method) the only added cost is one ClassUtils.getInterfaceMethodIfPossible cache lookup and one canAccess check — both cheap. The widening branch runs only when access is actually denied, and pays exactly one getDeclaredMethod per property in that case. The synthetic-field filter is defensive and can only remove keys that were themselves produced by the widened field access.

Verdict

LGTM. Well-scoped fix with a thorough commit message and matching test coverage. The Java-vs-Groovy field-access asymmetry (point 1) is the only thing I'd want a decision on before merge — either accept it as intentional and add a Java-side test to lock it in, or align by widening in the Java GenericJavaBeanMarshaller too. The other suggestions are polish.

[sbglasius]

@jdaugherty and @bkoehm do you want to weigh in on @matrei's bullet 1?

[jdaugherty]

@sbglasius My original concern was we should force these to be made public (as in change your code, not the framework) and not do this PR. Isn't that what bullet 1 is effectively identifying - that this is already the behavior of java based code?

[bkoehm]

@jdaugherty and @bkoehm do you want to weigh in on @matrei's bullet 1?

If we're talking about the AI-generated "Interface-method resolution" bullet, I can't make much of it.

But I'll say, I'm nearly ready to approve this, but with a couple points. One, by approving, I'm not looking to circumvent any opinions of @jdaugherty . Two, I would like to know if you (@sbglasius) had any opinion on my response in this comment:
#16296 (comment)
My only outstanding question on this PR is whether the "not mutated" case is the way to go and I was waiting for a further discussion on that before approving.

[sbglasius]

@jdaugherty Pushed e9932f371f with what we agreed on Slack:

• The warning. Every non-public bean class is reported once, through its own logger (org.apache.grails.common.reflect.ReflectionUtils), so one logging line silences it. The bookkeeping runs before the log level is consulted, so "once" doesn't depend on how logging is configured, and it holds class names rather than Class references so no class loader is pinned. JDK/Groovy/Spring classes aren't reported — nobody reading the log can declare those public. warnOnNonPublicClass is the seam to delete if Groovy stops compiling anonymous classes as non-public.
• One shared utility, not a fifth. The widening block was copy-pasted in all four marshallers; it now lives in org.apache.grails.common.reflect.ReflectionUtils in grails-common, which is already on the compile classpath of every module that could need it. I left the GORM one alone — org.grails.datastore.mapping.reflect.ReflectionUtils is published, public since 1.0, with 8 call sites — so the full move you suggested is Consolidate the reflection utility classes into grails-common #16326 rather than growing this PR into a cross-module API change.
• Java and Groovy aligned (Mattias's open point): the Java marshallers gated their field loop on canAccess, so a public field on a non-public class was silently dropped where the Groovy ones emitted it. Both now widen a field copy, and skip rather than fail when a field genuinely can't be widened (a class in a named module that doesn't open its package).
• Upgrade guide §54, since that field change alters response payloads and the warning needs a documented off switch.

On forcing the fix instead of handling it: for the shape in the original report there's no modifier to add — an anonymous class can't be declared public — which is what the warning is for, pointing people at the named-class fix.

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: e9932f3
▶️ Tests: 71070 executed
⚪️ Checks: 89/89 completed

---

Learn more about TestLens at testlens.app/docs.

[jdaugherty]

I like the direction and I think the warning is an acceptable solution. There's some minor improvements then we can merge ...

[jdaugherty]

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).

[jdaugherty]

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.

[jdaugherty]

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.

[jdaugherty]

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.

[jdaugherty]

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.