Repository navigation
8.0 (fix): Skip scaffold pages for plugins that are only on the runtime classpath - #16407
Conversation
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 Report❌ Patch coverage is Additional details and impacted files@@ 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
🚀 New features to boost your workflow:
|
matrei
left a comment
There was a problem hiding this comment.
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
implementationdependency, 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
pageTypesaddsclassesDirstopageClasspath, 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
compilableloader 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'simplementation. The mechanism is the same (onruntimeClasspath, notcompileClasspath), 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
readControllerunchanged, and the unit test assertscom.example.Useris 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.classNamegivesOuter$Inner, and replacing only.gives the right resource path. @CompileClasspathfits 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.
|
Thanks for the careful review, @matrei. Everything below is in 5dd2f29. 1. 2. Platform types. The lookup's parent is now 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 Re-run with |
matrei
left a comment
There was a problem hiding this comment.
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.*andorg.grails.gradle.plugin.views.gsp.*) withcleanTestand--no-build-cache. All 78 pass. - A throwaway TestKit spec, not part of the PR, that used the runtime-only fixture and ran
generateScaffoldedViewswith--configuration-cache. The entry is stored and reused, and on reuse the widget pages are generated and the gadget ones are not. After appendinggadget-plugin.jartocompileGroovyPages.classpath, a new entry is stored, and on reuse the gadget pages are generated. So theproject.files { tasks.named(...).get().classpath }wiring works with the configuration cache. The existing configuration-cache test inTagLibraryIndexWiringFunctionalSpeconly runsclasses, so it doesn't covergenerateScaffoldedViews. generateScaffoldedViews --dry-runshows the graphcompileJava,generateTagLibraryIndex,compileGroovy,processResources,classes,packageTagLibraryIndex,generateScaffoldedViews. There's no cycle, andcompileGroovyPagesis not pulled in. One correction to my first round: thepackageTagLibraryIndexedge isn't new with this wiring.allClasspathalready carried the packaged index, so it was there inf7cad10e05too.
Optional
- If you want the configuration-cache behaviour pinned, a
--configuration-cacherun ofgenerateScaffoldedViewsin 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
pageClasspathnow followscompileGroovyPages.classpath, and the new functional-test step (appending the runtime-only plugin to that classpath) fails with the oldallClasspathwiring. It's a real regression guard.- With
ClassLoader.platformClassLoaderas the parent,java.sql.Timestampresolves. The unit test asserts it next tojava.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
classesDirsjoins 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.
|
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 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 |
sbglasius
left a comment
There was a problem hiding this comment.
Code review (low effort, diff-only pass). 3 findings:
-
Silent page drop when
pageClasspathis unset —GenerateScaffoldedViewsTask.groovyreadPluginController(~line 465): if a build hand-registers this task (or any wiring skips settingpageClasspath),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. -
Stale classpath risk if
pageClasspathis populated eagerly — same file (~line 407): the design assumespageClasspathis wired lazily (asGroovyPagePlugindoes via aproject.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 ifcompileGroovyPages' classpath is mutated afterward. -
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.
|
Thanks @sbglasius.
|
✅ All tests passed ✅🏷️ Commit: cfefe7f Learn more about TestLens at testlens.app/docs. |
Follow-up to #16398.
Problem
generateScaffoldedViewsfinds scaffolded controllers in every plugin on the runtime classpath.compileGroovyPagesthen 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
implementationdependency. 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: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
generateScaffoldedViewsgets a newpageClasspathinput, read fromcompileGroovyPages' own classpath, so anything a build adds there is followed.java.sql.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
implementationand one asruntimeOnly. Pages are generated only for the first.All 78 tests pass. I did not run the full
grails-gradle-pluginssuite oraggregateViolations.