Fix marshalling of non-public classes and Serializable beans by sbglasius · Pull Request #16296 · apache/grails-core

sbglasius · 2026-09-02T15:05:41Z

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 · 2026-09-02T15:59:06Z

Codecov Report

❌ Patch coverage is 71.42857% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 54.9291%. Comparing base (5670a65) to head (cc6f5c8).

Files with missing lines	Patch %	Lines
...ers/marshaller/json/GenericJavaBeanMarshaller.java	60.0000%	0 Missing and 2 partials ⚠️
...ters/marshaller/xml/GenericJavaBeanMarshaller.java	66.6667%	0 Missing and 2 partials ⚠️
...nverters/marshaller/json/GroovyBeanMarshaller.java	80.0000%	0 Missing and 1 partial ⚠️
...onverters/marshaller/xml/GroovyBeanMarshaller.java	80.0000%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##                8.0.x     #16296        +/-   ##
==================================================
+ Coverage     54.8238%   54.9291%   +0.1053%     
- Complexity      20521      20567        +46     
==================================================
  Files            2104       2104                
  Lines          101102     101114        +12     
  Branches        17932      17934         +2     
==================================================
+ Hits            55428      55541       +113     
+ Misses          37787      37656       -131     
- Partials         7887       7917        +30

Files with missing lines	Coverage Δ
...nverters/marshaller/json/GroovyBeanMarshaller.java	`72.9730% <80.0000%> (+11.2083%)`	⬆️
...onverters/marshaller/xml/GroovyBeanMarshaller.java	`61.5385% <80.0000%> (+58.7607%)`	⬆️
...ers/marshaller/json/GenericJavaBeanMarshaller.java	`64.8649% <60.0000%> (+61.9237%)`	⬆️
...ters/marshaller/xml/GenericJavaBeanMarshaller.java	`70.3704% <66.6667%> (+66.2037%)`	⬆️

... and 10 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 · 2026-09-02T17:10:58Z

+ * Hands out instances of Groovy classes that are not public, but whose read methods are. This is the
+ * shape produced by an anonymous implementation of a public interface inside a Grails controller.
+ */
+@CompileStatic


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

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

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

jdaugherty · 2026-09-02T17:13:42Z

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


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

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.

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?

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

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 · 2026-09-02T17:14:57Z

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

sbglasius · 2026-09-02T17:34:19Z

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

The existing fixtures were all `@CompileStatic`. Dynamic and static Groovy compile an anonymous class to the same shape — a package-private class with public synthetic `Reference` fields for the captured variables — so this covers the same reflection path rather than a new one, and guards against the two compilation modes diverging. Raised in review of #16296.

jdaugherty · 2026-09-04T07:16:43Z

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.

Marshalling `someObject as JSON` (or `as XML`) failed for two common shapes of object. 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 are public. All four bean marshallers called `readMethod.invoke(...)` bare, so handing `as JSON` an anonymous implementation of a public interface (the usual shape of a Spring Security `UserDetails`) blew up with an `IllegalAccessException`. The read method is now resolved to the interface method where one exists and made accessible otherwise. The public field loops of both `GroovyBeanMarshaller`s failed the same way and are now made accessible too. `GenericJavaBeanMarshaller` evaluated `field.canAccess(o)` before the static check, and `canAccess` throws `IllegalArgumentException` for a static member when the object is non-null. Any bean declaring `private static final long serialVersionUID` — nearly every Serializable bean — therefore failed. The modifier checks now run first so they short-circuit. Groovy compiles the variables captured by an anonymous class into public synthetic `Reference` fields. Now that the field loops can read them, they were emitted as duplicate keys with empty-object values, so synthetic fields are skipped in all four marshallers. Fixes #16294 Fixes #16295

The existing fixtures were all `@CompileStatic`. Dynamic and static Groovy compile an anonymous class to the same shape — a package-private class with public synthetic `Reference` fields for the captured variables — so this covers the same reflection path rather than a new one, and guards against the two compilation modes diverging. Raised in review of #16296.

testlens-app · 2026-09-04T10:31:31Z

✅ All tests passed ✅

🏷️ Commit: cc6f5c8
▶️ Tests: 69552 executed
⚪️ Checks: 89/89 completed

Learn more about TestLens at testlens.app/docs.

bkoehm · 2026-09-04T13:49:26Z

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 · 2026-09-04T13:54:01Z

@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?

github-project-automation Bot added this to Apache Grails Sep 2, 2026

sbglasius requested review from bkoehm, dauer, jdaugherty and matrei September 2, 2026 15:09

bkoehm reviewed Sep 2, 2026

View reviewed changes

jdaugherty reviewed Sep 2, 2026

View reviewed changes

sbglasius self-assigned this Sep 2, 2026

sbglasius added this to the grails:8.0.0-RC1 milestone Sep 2, 2026

sbglasius moved this to In Progress in Apache Grails Sep 2, 2026

sbglasius force-pushed the fix/16294-16295-non-public-bean-marshalling branch from c3446ef to 98fb828 Compare September 3, 2026 08:42

sbglasius added 2 commits September 4, 2026 10:28

sbglasius force-pushed the fix/16294-16295-non-public-bean-marshalling branch from 98fb828 to cc6f5c8 Compare September 4, 2026 08:28

Uh oh!

Uh oh!

Conversation

sbglasius commented Sep 2, 2026

#16294 — IllegalAccessException on a non-public class

#16295 — IllegalArgumentException on any Serializable bean

One change beyond the two issues

Files changed

Tests

Uh oh!

codecov Bot commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jdaugherty commented Sep 2, 2026

Uh oh!

sbglasius commented Sep 2, 2026

Uh oh!

jdaugherty commented Sep 4, 2026

Uh oh!

testlens-app Bot commented Sep 4, 2026

✅ All tests passed ✅

Uh oh!

bkoehm commented Sep 4, 2026

Uh oh!

sbglasius commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

#16294 — `IllegalAccessException` on a non-public class

#16295 — `IllegalArgumentException` on any `Serializable` bean

codecov Bot commented Sep 2, 2026 •

edited

Loading