Skip to content

8.0 (fix): Skip scaffold pages for plugins that are only on the runtime classpath - #16407

Merged
jdaugherty merged 5 commits into
apache:8.0.xfrom
codeconsole:fix/scaffold-runtime-only-plugin-controllers-8.0.x
Sep 28, 2026
Merged

jdaugherty merged 5 commits into
apache:8.0.xfrom
codeconsole:fix/scaffold-runtime-only-plugin-controllers-8.0.x

Conversation

@codeconsole

@codeconsole codeconsole commented Sep 25, 2026 •

Copy link
Copy Markdown
Contributor

Follow-up to #16398.

Problem

generateScaffoldedViews finds scaffolded controllers in every plugin on the runtime classpath. compileGroovyPages then compiles the generated pages against the compile classpath.

Some plugins are only on the runtime classpath, typically because they come in through another project's implementation dependency. The page compiler can't see those plugins' domain classes. So every page generated for them fails to compile and is left out, with a warning and a compiler error like this:

Left out the optional page grails-scaffolded/com.example.Widget/index-78a2d25e9f37d1c1.gsp, which does not compile ...
unable to resolve class com.example.Widget

No page is actually lost, because the plugin's own build compiles those pages. The build output just fills up with these warnings, repeated in every project the plugin reaches.

Fix

  • generateScaffoldedViews gets a new pageClasspath input, read from compileGroovyPages' own classpath, so anything a build adds there is followed.
  • A plugin's controller only gets pages generated if its domain class is on that classpath. Skipped controllers are logged at INFO.
  • The application's own controllers are not affected. JDK domain types still resolve, including those in platform modules such as java.sql.
  • The scaffolding guide is updated.

I considered compiling the pages against the runtime classpath instead. Generated pages share one compilation with the application's hand-written views, though, so that would also change what those views compile against.

Tests

  • A new TestKit test uses one plugin as implementation and one as runtimeOnly. Pages are generated only for the first.
  • New unit tests cover the skip, a JDK domain type, and a plugin controller that scaffolds a class from the project itself.
  • With the fix reverted, the new tests fail.
cd grails-gradle
./gradlew :grails-gradle-plugins:test --tests 'org.grails.gradle.plugin.scaffolding.*' --tests 'org.grails.gradle.plugin.views.gsp.*'
./gradlew :grails-gradle-plugins:codeStyle

All 78 tests pass. I did not run the full grails-gradle-plugins suite or aggregateViolations.

Controllers are read from every plugin on the runtime classpath, but the
pages expanded for them are compiled against compileGroovyPages'
classpath, which starts from the compile classpath. A plugin that reaches
a project only at runtime - through an implementation dependency of a
project it depends on - is on the first and not the second, so every page
expanded for its controllers named a domain class the compiler could not
resolve, and each was left out with a warning and a compiler error. In a
multi-project build that repeats in every project the plugin reaches,
while the plugin's own build, which has its domain classes, expands and
compiles those pages anyway.

generateScaffoldedViews now takes the classpath the pages are compiled
against as pageClasspath, and expands pages for a plugin's controller
only where the domain class it scaffolds is on it, noting a controller it
leaves out at info level. A type the platform provides needs no entry, and
an application's own controllers are not checked, since they were
compiled against their domain classes.
@codecov

codecov Bot commented Sep 26, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 72.41379% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.8603%. Comparing base (334007a) to head (cfefe7f).

Files with missing lines Patch % Lines
...gin/scaffolding/GenerateScaffoldedViewsTask.groovy 74.0741% 0 Missing and 7 partials ⚠️
...ls/gradle/plugin/views/gsp/GroovyPagePlugin.groovy 50.0000% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##                8.0.x     #16407        +/-   ##
==================================================
+ Coverage     57.8440%   57.8603%   +0.0163%     
- Complexity      22858      22870        +12     
==================================================
  Files            2137       2137                
  Lines          104666     104678        +12     
  Branches        18808      18811         +3     
==================================================
+ Hits            60543      60567        +24     
+ Misses          35761      35747        -14     
- Partials         8362       8364         +2     
Files with missing lines Coverage Δ
...ls/gradle/plugin/views/gsp/GroovyPagePlugin.groovy 68.7500% <50.0000%> (-0.2155%) ⬇️
...gin/scaffolding/GenerateScaffoldedViewsTask.groovy 80.6723% <74.0741%> (+0.4091%) ⬆️

... and 3 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.

@codeconsole codeconsole changed the title Leave a plugin the scaffold pages cannot be compiled against to its own build Skip scaffold pages for plugins that are only on the runtime classpath Sep 26, 2026
@codeconsole codeconsole added this to the grails:8.0.0-RC2 milestone Sep 26, 2026
@codeconsole codeconsole changed the title Skip scaffold pages for plugins that are only on the runtime classpath 8.0 (fix): Skip scaffold pages for plugins that are only on the runtime classpath Sep 26, 2026

@matrei matrei left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at head f7cad10e05 against 8.0.x (merge-base 0dab8a7263). 8.0.x has moved on since, but none of the new commits touch scaffolding or GroovyPagePlugin. CI is green.

I ran the PR's targeted tests locally with cleanTest and --no-build-cache (org.grails.gradle.plugin.scaffolding.* and org.grails.gradle.plugin.views.gsp.* in grails-gradle-plugins). All 78 pass.

The fix looks right to me, and I agree with not moving the page compilation to the runtime classpath. I also checked that a runtime-only plugin's own pages can still be found. The plugin's build treats its controllers as its own and compiles their pages into its views.properties, and at runtime DefaultGroovyPageLocator.findPage falls through to findBinaryScriptSource, which searches each binary plugin's precompiled views. So skipping those controllers here doesn't lose anything the plugin didn't already provide. Nothing below blocks the change.

1. pageClasspath is wired from allClasspath, not from compileGroovyPages.classpath

The description says pageClasspath is "wired from the same classpath compileGroovyPages uses", but it is wired from the same variable, allClasspath. If a build adds to compileGroovyPages.classpath (a custom configuration, say), the pages would compile against the extra entries while generateScaffoldedViews still skips plugins whose domain classes are only there. Wiring it lazily from the task itself would keep the two from drifting apart:

it.pageClasspath.from(project.files { tasks.named('compileGroovyPages', GroovyPageForkCompileTask).get().classpath })

This is not a cycle. compileGroovyPages depends on this task's outputs, not on its classpath. As a side effect it also covers the index directories allClasspath carries. Those pull in packageTagLibraryIndex (and through it classes) as a new dependency of generateScaffoldedViews, which the check doesn't need. That's harmless, but it's a new ordering edge. If you'd rather keep allClasspath, a short comment saying the two must stay in sync would do.

2. The domain-class lookup only sees the bootstrap class loader

new URLClassLoader(pageTypes, (ClassLoader) null) delegates to the bootstrap loader, so java.lang.Long resolves but a type in a platform module doesn't. For example, java/sql/Timestamp.class lives in java.sql, which the platform loader defines, so getResource returns null. A plugin controller scaffolding such a type would be skipped silently (at INFO), while its pages would compile fine. It's very unlikely in practice. Using ClassLoader.platformClassLoader as the parent would make the "a type the platform provides needs no entry" claim in the commit message hold as written. A note is fine too.

It's also worth saying somewhere (javadoc, or the INFO message) that this is a necessary check, not a sufficient one. A page can still fail on another type the template mentions, such as an association's type that is itself runtime-only. That case still ends up as an optional page with a warning, which is fine, but "not on the classpath" doesn't cover everything that can fail.

3. Docs

  • scaffolding.adoc:64: with the new sentences in front of it, "For each, it expands every copy of each template..." no longer clearly refers to the controllers. "For each controller it covers, ..." would fix that.
  • "A plugin the project has only at runtime ... is not: a page naming its domain class could not be compiled" is hard to parse, because "is not" refers back to "is on the classpath" two clauses earlier. Something like: "A plugin the project has only at runtime, such as one that reaches it through another project's implementation dependency, is not on that classpath. A page naming its domain class could not be compiled here, so its controllers are left to the plugin's own build." reads more easily.
  • The native-image advice ("declare it as a dependency of the application") is correct. It might be worth saying why the plugin's own pages might not be enough: they only exist for the template copies the plugin's build saw, so an application template override, or a different theme earlier on the application's classpath, gives a page the plugin never compiled. In a native image that now only shows up as the runtime WARN, because the build no longer says anything above INFO for these controllers.

4. Minor

  • pageTypes adds classesDirs to pageClasspath, which already contains them when the plugin wires it. That's needed when the task is used on its own, and the unit test "a plugin controller scaffolding a domain class of the project's own" depends on it. A short comment would stop someone from "cleaning it up" later.
  • The compilable loader is opened even when nothing on the runtime classpath is a plugin. It's cheap, so this is just an observation.
  • The functional test simulates "only at runtime" with runtimeOnly files(...). The case the description motivates is a multi-project build, where the plugin reaches the app through another project's implementation. The mechanism is the same (on runtimeClasspath, not compileClasspath), so I don't think a second TestKit project is needed. I'm only mentioning it in case you already have that setup handy.

Verified as correct

  • Only plugin controllers are filtered. The application's own controllers go through readController unchanged, and the unit test asserts com.example.User is still expanded.
  • Templates are still collected from the full runtime classpath, so a runtime-only plugin's templates are still expanded for the controllers that remain. The functional test's ['gadget show ...', 'widget show ...'] assertion pins this.
  • Nested domain classes resolve: Type.className gives Outer$Inner, and replacing only . gives the right resource path.
  • @CompileClasspath fits an input where only the presence of classes matters, and the resources in the index directories are ignored by its normalization, so they don't invalidate the cache.
  • The two existing plugin-controller unit tests were updated to put the domain class on pageClasspath. Without that change, both would now fail.

… types

Review follow-ups:

- pageClasspath is read from compileGroovyPages' classpath rather than
  from the collection that task is configured with, so a build that adds
  to that classpath is followed. The functional test adds the runtime-only
  plugin to it and checks its pages are then generated.
- The domain-class lookup has the platform class loader as its parent, so
  a type from a platform module such as java.sql resolves as java.lang's
  do. With no parent, java.sql.Timestamp was not found and its controller
  was skipped although its pages would compile.
- The javadoc says the check is necessary, not sufficient, and why the
  project's classes are added to the lookup.
- The scaffolding guide's wording is clearer, and it says why a plugin
  the application has only at runtime should be declared as a dependency
  for a native image.
@codeconsole

Copy link
Copy Markdown
Contributor Author

Thanks for the careful review, @matrei. Everything below is in 5dd2f29.

1. pageClasspath wiring. Done the way you suggested: project.files { tasks.named('compileGroovyPages', GroovyPageForkCompileTask).get().classpath }. The functional test now appends the runtime-only plugin to compileGroovyPages.classpath in the build script and checks its pages are then generated. That assertion fails with the old allClasspath wiring. TagLibraryIndexWiringFunctionalSpec, which includes the no-cycle and configuration-cache tests, still passes.

2. Platform types. The lookup's parent is now ClassLoader.platformClassLoader. The platform-type unit test gained a java.sql.Timestamp controller, which fails with the old null parent. I also added the "necessary, not sufficient" note to readPluginController's javadoc, using an association's type as the example.

3. Docs. Took your wording for line 64, including "For each controller it covers". The native-image advice now says why: the plugin's own build only compiled pages for the template copies it saw, so an application template or a theme earlier on the classpath gives a page it never compiled.

4. Minor. Added a comment on why classesDirs joins the lookup. I left the loader opening as it is. I also left out the multi-project fixture, as you suggested.

Re-run with cleanTest --no-build-cache: the 78 tests in org.grails.gradle.plugin.scaffolding.* and org.grails.gradle.plugin.views.gsp.* pass, and so does codeStyle.

@codeconsole
codeconsole requested a review from matrei September 27, 2026 04:24

@matrei matrei left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at head 5dd2f2979c against 8.0.x (merge-base 0dab8a7263, still merges cleanly, and nothing since touches scaffolding, GroovyPagePlugin or the guide). CI is green.

Thanks, all four points are addressed, and I have nothing blocking. Approving from my side.

What I ran locally:

  • The PR's targeted tests (org.grails.gradle.plugin.scaffolding.* and org.grails.gradle.plugin.views.gsp.*) with cleanTest and --no-build-cache. All 78 pass.
  • A throwaway TestKit spec, not part of the PR, that used the runtime-only fixture and ran generateScaffoldedViews with --configuration-cache. The entry is stored and reused, and on reuse the widget pages are generated and the gadget ones are not. After appending gadget-plugin.jar to compileGroovyPages.classpath, a new entry is stored, and on reuse the gadget pages are generated. So the project.files { tasks.named(...).get().classpath } wiring works with the configuration cache. The existing configuration-cache test in TagLibraryIndexWiringFunctionalSpec only runs classes, so it doesn't cover generateScaffoldedViews.
  • generateScaffoldedViews --dry-run shows the graph compileJava, generateTagLibraryIndex, compileGroovy, processResources, classes, packageTagLibraryIndex, generateScaffoldedViews. There's no cycle, and compileGroovyPages is not pulled in. One correction to my first round: the packageTagLibraryIndex edge isn't new with this wiring. allClasspath already carried the packaged index, so it was there in f7cad10e05 too.

Optional

  • If you want the configuration-cache behaviour pinned, a --configuration-cache run of generateScaffoldedViews in the runtime-only functional test would do it. Right now nothing in the suite would notice if the closure started capturing something that can't be serialized.
  • scaffolding.adoc, native-image paragraph: the new reason makes the plugin item in that ;-separated list quite long, and it holds a second "and" clause. Splitting the reason into its own sentence after the list would read more easily, for example: "... declare it as a dependency of the application. The plugin's own build compiled pages only for the template copies it saw, so an application template, or a theme earlier on the application's classpath, gives a page it never compiled." That's just a suggestion.

Verified as correct

  • pageClasspath now follows compileGroovyPages.classpath, and the new functional-test step (appending the runtime-only plugin to that classpath) fails with the old allClasspath wiring. It's a real regression guard.
  • With ClassLoader.platformClassLoader as the parent, java.sql.Timestamp resolves. The unit test asserts it next to java.lang.Long. Application classes still can't leak in, because the platform loader doesn't see the Gradle or application classpath.
  • The comment on why classesDirs joins the lookup, and the "necessary, not sufficient" javadoc, match the behaviour.
  • The guide's line 64 now reads "For each controller it covers", and the runtime-only sentence is split as suggested.

The functional test now stores and reuses a configuration cache entry
for generateScaffoldedViews, before and after the build adds to
compileGroovyPages' classpath, with the output removed so the reused
entry has to run the task. Nothing else in the suite would notice if
the closure that reads that classpath started capturing something that
cannot be serialized.

The guide's native-image advice keeps the plugin item short; why it
needs declaring is a sentence of its own after the list.
@codeconsole

Copy link
Copy Markdown
Contributor Author

Thanks @matrei, and thanks for running the configuration-cache check. I took both optional points in 894bb72.

Configuration cache. The runtime-only functional test now runs generateScaffoldedViews with --configuration-cache four times. The first stores an entry and the second reuses it. The build then appends gadget-plugin.jar to compileGroovyPages.classpath, the third run stores a new entry, and the fourth reuses that one. Before each reuse the generated output is deleted, so the task actually runs from the reused entry. The first reuse still leaves the gadget pages out, and the second reuse generates them.

Guide. The plugin item in the native-image list is just the instruction now. Why the plugin needs declaring is its own sentence after the list.

The same 78 tests pass with cleanTest --no-build-cache, and so does codeStyle.

@sbglasius sbglasius left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review (low effort, diff-only pass). 3 findings:

  1. Silent page drop when pageClasspath is unset — GenerateScaffoldedViewsTask.groovy readPluginController (~line 465): if a build hand-registers this task (or any wiring skips setting pageClasspath), compilable.getResource(...) fails to resolve every plugin domain class, so all plugin-scaffolded pages silently disappear — only surfaced via an --info-level log line, easy to miss in CI.

  2. Stale classpath risk if pageClasspath is populated eagerly — same file (~line 407): the design assumes pageClasspath is wired lazily (as GroovyPagePlugin does via a project.files { ... } closure). A consumer that wires it eagerly (e.g. .from(someTask.classpath) evaluated at configuration time) would check domain-class resolvability against a stale snapshot if compileGroovyPages' classpath is mutated afterward.

  3. Doc gap for plugins built before this feature — scaffolding.adoc (~line 64): states a runtime-only plugin's pages are "left to the plugin's own build," but that's only true if the plugin itself was built with this new domain-class filtering. A plugin published before this feature existed would have neither the consuming project nor its own (older) build produce the compiled page, silently falling back to first-request expansion — worth calling out explicitly.

Generated via /code-review low.

A task registered by hand, as one written for 8.0.0-RC1 would be, never sets
pageClasspath, so every plugin controller was left out. It now expands them
all, as it did before the check existed.
@codeconsole

Copy link
Copy Markdown
Contributor Author

Thanks @sbglasius.

  1. Valid, in a narrow case. The task has shipped since 8.0.0-M6, so a build that registered it by hand never sets pageClasspath and would lose every plugin page. Fixed in cfefe7f: an empty pageClasspath now checks nothing, so every plugin controller is expanded as before. The two tests that relied on it being empty now set it, so they still exercise the check. A new test covers the unset case, and it fails without the fix.

  2. No change. from(task.classpath) keeps a live reference that is resolved when the task runs, not a snapshot taken at configuration time. It would only miss a build that replaces the property object, which is why the plugin reads it through a closure. The functional test's append-to-compileGroovyPages.classpath step pins that wiring.

  3. No change. The filter applies only to other plugins' controllers in a consuming project. A plugin's own controllers come from its classesDirs unfiltered, so its build compiles their pages whether or not it has this change. For a plugin built before generateScaffoldedViews existed, the guide already says the page is "expanded on its first use" ("a controller of a plugin the application has only at runtime whose pages the plugin's own build did not compile"). For native images it says to declare the plugin as a dependency. Nothing regresses either: before this PR, that page couldn't compile in the consumer anyway.

@testlens-app

testlens-app Bot commented Sep 27, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: cfefe7f
▶️ Tests: 84180 executed
⚪️ Checks: 88/88 completed


Learn more about TestLens at testlens.app/docs.

@jdaugherty
jdaugherty merged commit b607f3d into apache:8.0.x Sep 28, 2026
91 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants