Skip to content

Preserve view precedence when precompiling scaffolds - #16385

Merged
jdaugherty merged 8 commits into
apache:8.0.xfrom
codeconsole:fix/scaffold-precompiled-view-collisions
Sep 24, 2026
Merged

jdaugherty merged 8 commits into
apache:8.0.xfrom
codeconsole:fix/scaffold-precompiled-view-collisions

Conversation

@codeconsole

@codeconsole codeconsole commented Sep 23, 2026 •

Copy link
Copy Markdown
Contributor

An application with a namespaced @Scaffold controller can render a different page after packaging than it does under bootRun. For example, an admin EventController generates an application-level /event/show.gsp that shadows a calendar plugin's custom event view. Development skips the application's precompiled view map, hiding the collision.

This change makes scaffold precompilation preserve runtime view precedence:

  • Preserve handwritten application views and views registered by dependency plugins, including runtimeOnly plugins. Read both supported plugin index locations in runtime order, and require a plugin descriptor for JARs.
  • Leave namespaced controllers and their shared view directories to runtime resolution. Detect static fields and getters, inherited declarations, and trait-supplied namespaces without loading application classes.
  • Use Grails' controller and domain naming rules, including acronym-prefixed names. Keep precompiling unaffected views and remove stale generated pages when a plugin starts providing a view.

Namespaced controllers and controllers whose view directories collide while scaffolding different domains use runtime view generation. Native-image applications need concrete GSP views for those controllers. The guide explains this behavior and the generation, staging, and compilation tasks; build warnings identify skipped scaffold directories. Precompiling namespace-specific views is a separate enhancement.

Validation:

  • All executed GitHub Actions checks on the updated branch (9276e6bd34) passed, including the Gradle-plugin builds, end-to-end build, framework and datastore suites, style analysis, and coverage checks. Conditional jobs reported as skipped did not run.
  • Local checks on the final review-fix commit (94fc557516) passed all 46 tests in GenerateScaffoldedViewsTaskSpec and GroovyPagePluginFunctionalSpec, plus module codeStyle and validateDependencyVersions. Coverage includes inherited and trait namespaces, acronym naming, plugin-index precedence, descriptorless JARs, unreadable ancestor classes, stale output removal, and incremental staging.
  • An l3me build using the initial pinned fix completed GSP compilation, with the conflicting application event-view entry absent and the calendar plugin's custom view preserved.

@codecov

codecov Bot commented Sep 23, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 76.34409% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.4491%. Comparing base (7ea4cb9) to head (9276e6b).

Files with missing lines Patch % Lines
...gin/scaffolding/GenerateScaffoldedViewsTask.groovy 77.7778% 5 Missing and 15 partials ⚠️
...ls/gradle/plugin/views/gsp/GroovyPagePlugin.groovy 33.3333% 2 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##                8.0.x     #16385        +/-   ##
==================================================
+ Coverage     57.4441%   57.4491%   +0.0049%     
- Complexity      22558      22567         +9     
==================================================
  Files            2129       2129                
  Lines          103840     103899        +59     
  Branches        18646      18662        +16     
==================================================
+ Hits            59650      59689        +39     
- Misses          35929      35942        +13     
- Partials         8261       8268         +7     
Files with missing lines Coverage Δ
...ls/gradle/plugin/views/gsp/GroovyPagePlugin.groovy 57.2917% <33.3333%> (-0.6031%) ⬇️
...gin/scaffolding/GenerateScaffoldedViewsTask.groovy 72.8477% <77.7778%> (+6.8902%) ⬆️

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

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

Review Findings

Head a820aab753, base 8.0.x at 96e0e72f68 (the current tip), one commit, merges clean. CI on the head is fully green (all checks passed, including both Gradle-plugin builds and the end-to-end build).

The diagnosis in the description holds against the runtime code. DefaultGroovyPageLocator.findPageInBinding consults the application's precompiledGspMap (findResourceScriptSource) before it asks the binary plugins (findBinaryScriptSource), and isPrecompiledAvailable() returns false in development mode, so a scaffold page written into the application's registry shadows a plugin page in a packaged application only. ScaffoldingViewResolver.loadView only expands a template when super.loadView found nothing, so a plugin page wins over scaffolding at runtime, and for a namespaced controller it looks for <namespace>/<view>.gsp under the templates first. Both of the task's new exclusions therefore make build-time output agree with what the resolvers do. The registry keys match too: compileGroovyPages runs with serverpath /WEB-INF/grails-app/views/, which is the prefix findPluginViews compares against, and it is the same path resolveViewInBinaryPlugin builds for the lookup.

Approving. Three suggestions, none blocking, then the nits.

Suggestions

  • grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/scaffolding/GenerateScaffoldedViewsTask.groovy:268-271: the "Not precompiling" line is logged for every controller with a static namespace, scaffolded or not, because namespaced is collected before readScaffoldDomain filters. In a typical application most namespaced controllers are hand-written, so this reports something that was never going to happen. Guard it on the removal: if (found.remove(controllerName) != null) { logger.info(...) }.
  • Same place: for a controller that is scaffolded, this is a silent loss for a native image. Before this change its views were precompiled (into the wrong directory, granted); after it, nothing is, and the first request in an image fails because the resolver cannot define a class. The guide paragraph says so, but a build that knows it has just left a scaffolded controller without views could say so at warn, the way the mixed-domain case at line 276 already does. Something like "Not precompiling the views of event: admin.EventController declares a namespace, so they are expanded at runtime; a native image needs concrete views for it".
  • Follow-up rather than for this PR: GrailsConventionGroovyPageLocator.findView (lines 146-152) tries /<namespace>/<controller>/<view> before the unqualified path, and ScaffoldingViewResolver treats a view whose URL contains /<namespace>/ as namespace-specific. So a namespaced controller's pages could be precompiled into admin/event/*.gsp (from admin/<view>.gsp under the templates when present, else the default) without touching the shared directory, which would restore native-image support for them. It needs the namespace value at build time. A static final String namespace = 'admin' carries a ConstantValue attribute; the usual static namespace = 'admin' is assigned in <clinit>, which is an LDC/PUTSTATIC pair ASM can read without loading the class. Worth an issue if there is appetite; the PR's test explicitly asserts admin/event is not produced, so that decision would want revisiting then.

Nits

  • GenerateScaffoldedViewsTask.groovy:91-93 (and the pre-existing templateClasspath at 86-88): a resolved configuration as a task input is what @Classpath is for. It normalises jar entries the way the compile tasks do, so a manifest-timestamp-only rebuild of a dependency does not invalidate this cacheable task. @InputFiles @PathSensitive(RELATIVE) is not wrong, only more sensitive than it needs to be.
  • GenerateScaffoldedViewsTask.groovy:143,270: both new log lines build a GString before logger.info has checked the level, and the first sits inside the per-view loop. Gradle's logger is SLF4J, so logger.info('Skipping {}/{}.gsp, a plugin declares it', controller.key, viewName) and logger.info('Not precompiling {}: its namespace is resolved at runtime', controllerName) defer the formatting to when info is on. That is also the majority style in the module (19 calls with placeholders against 9 with GStrings), and this file holds five of the nine, so the three pre-existing lines at 119, 139 and 152 could follow while it is open.
  • GenerateScaffoldedViewsTask.groovy:256,259: each controller's bytes are read twice, once for hasNamespace and once inside readScaffoldDomain. One ClassReader could serve both visitors. Cheap either way.
  • GenerateScaffoldedViewsTaskSpec.groovy:335 and GroovyPagePluginFunctionalSpec.groovy:131: no blank line between the previous feature method's closing brace and the new one. Every other method in both specs is separated by one.
  • scaffolding.adoc:62: "Scaffold views may be precompiled when building a deployment archive" is the first the guide says of build-time scaffold precompilation at all. A reader who has not seen the 8.0 release notes will not know what "precompiled" refers to here. One sentence saying the Gradle plugin expands the templates during compileGroovyPages so packaged applications and native images do not generate them on first request would anchor the two paragraphs that follow.

Confirmed

  • hasNamespace matches what DefaultGrailsControllerClass reads: getStaticPropertyValue("namespace"), which a Groovy static namespace = 'admin' satisfies through both the private static field and the generated static getter, and a Java public static String namespace through the field. The instance-property case is correctly left alone (the resolver would not see a namespace either). Inheritance walks superName through a parent-less URLClassLoader over classesDirs plus the compile classpath, which is the only place a superclass can be, and stops at java/lang/Object or an unresolvable parent.
  • Leaving out the whole shared directory when an unqualified scaffolded controller shares a name with a namespaced one, scaffolded or not, is the consistent choice. In development, a hand-written admin.EventController with no show view falls through to /event/show, where nothing exists because scaffolding generates per requesting controller; a precompiled event/show.gsp would have changed that in the archive. The guide sentence covers it.
  • viewClasspath is configurations.runtimeClasspath, not the source set's runtime classpath, so the project's own compiled pages are not on it and runtimeOnly plugins are. The directory branch covers a project dependency resolved to classes, and the jar branch the published case; the TestKit test exercises the directory form through runtimeOnly files('calendar-plugin') and checks that emptying the index invalidates the task.
  • Stale output: generate() still deletes the output directory first, so a page a plugin starts providing disappears from the staged set, and stageGroovyPages is a Sync, so it disappears from the compilation input too. The unit test covers both directions.
  • templateClasspath and viewClasspath are read at execution time from ConfigurableFileCollections fed by configuration providers, which is configuration-cache safe.

Verification

  • cd grails-gradle && ./gradlew --no-build-cache --continue :grails-gradle-plugins:cleanTest :grails-gradle-plugins:test --tests '*GenerateScaffoldedViewsTaskSpec' --tests '*GroovyPagePluginFunctionalSpec' :grails-gradle-plugins:codeStyle on a820aab753 in a scratch worktree: BUILD SUCCESSFUL in 1m 4s. GenerateScaffoldedViewsTaskSpec 22 tests, GroovyPagePluginFunctionalSpec 7 tests, 0 failures, 0 errors; result XML timestamps 18:38 today. codenarcMain ran clean; checkstyleMain has no sources in this module and the test-source style tasks are skipped by the build as configured.
  • gh pr checks 16385: every check on the head passed.
  • git merge-tree --write-tree origin/8.0.x pr-16385: no conflicts against 96e0e72f68.
  • Runtime claims checked by reading, not running: DefaultGroovyPageLocator lookup order and isPrecompiledAvailable(), ScaffoldingViewResolver.loadView, GrailsConventionGroovyPageLocator.findView, DefaultGrailsControllerClass namespace reading, and the serverpath the plugin passes to compileGroovyPages.
  • The scratch worktree is removed; the main checkout was not touched.

@jdaugherty jdaugherty 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 b743404, and re-ran :grails-gradle-plugins:test (both specs, 22 cases) plus :grails-gradle-plugins:codeStyle in a worktree — all green.

The plugin-index half of the fix looks sound and complete. The key shape matches the serverpath GroovyPagePlugin sets on compileGroovyPages and the lookup BinaryGrailsPlugin.resolveView performs, and precompiledViewMap is the only way a binary plugin exposes views, so reading views.properties is the whole picture rather than a heuristic. Static-only namespace detection matches DefaultGrailsControllerClass, which reads the property with getStaticPropertyValue. Adding runtimeClasspath as a task input does not close a task-graph cycle — this generator sits downstream of compileGroovy and feeds only stageGroovyPages, unlike the compiler-config generator whose cycle needed it upstream. The second commit's @classpath inputs, shared ClassReader, guarded found.remove, and warn-level native-image message all read as improvements.

One confirmed bug below on how the view directory name is derived — it predates this PR but the new plugin check inherits the key — plus a question on how broad the namespace rule needs to be, and some robustness notes.

A controller's views live under GrailsNameUtils' logical property name, which keeps a
name beginning with two capitals unchanged, so APIController resolves API/show.gsp.
Decapitalizing wrote aPI/ instead - precompiled, never rendered - and pointed both the
application-view and plugin-index checks at the wrong key. The domain's property name
is bound through GrailsNameUtils too, as the runtime model builder binds it.
BinaryGrailsPlugin takes views.properties beside the plugin descriptor first and falls
back to gsp/views.properties, reading one per plugin; probe the same two in the same
order. The index key prefix now comes from GroovyPagePlugin.VIEWS_SERVER_PATH, the value
compileGroovyPages is given, so the two cannot drift apart.
Inherited namespaces are looked up on controllerClasspath rather than templateClasspath,
so scoping where templates are read from cannot silently stop them being found. A
superclass the bundled ASM cannot read is treated as declaring no namespace instead of
failing the build, and each superclass is read once however many controllers share it.
Document why the namespace rule stays broad and why an empty namespace still counts.

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

Second pass. The eight points from the first round are all addressed, and I re-derived the load-bearing claims rather than taking them on trust.

viewDirectory now matches AbstractGrailsClass exactly, including the blank-logical-name fallback (logicalName ?: controllerClassName against StringUtils.hasText(name)), and propertyName matches ModelImpl — that one passes the qualified name, but getPropertyNameRepresentation strips the package, so simple and qualified give the same answer. The keys GroovyPageCompiler writes are viewPrefix + relPath, so they really are /WEB-INF/grails-app/views/<dir>/<view>.gsp and the shared VIEWS_SERVER_PATH is now the only place that prefix is spelled. The descriptor-relative index matches what initializeViewMap resolves through createRelative. The superclass memo is consulted before the read, so a shared base is parsed once.

On the blanket namespace rule: ScaffoldingViewResolver.loadView does consult namespace-specific templates on the no-view path even with enableNamespaceViewDefaults false, so the trade-off the javadoc now describes is the right one.

I also checked the premises around the change. findPage tries the application's precompiled map before findBinaryScriptSource, so an application-level page does shadow a plugin's. A plugin's GSPs are only ever shipped precompiled — processResources excludes **/*.gsp — so reading the index is enough to see everything a plugin serves. And compiling each namespace declaration form with Groovy 5.1.2 (plain, typed, static final, inherited, trait-supplied), every one emits either a static field or a static getNamespace(), so the detection has no gap. :grails-gradle-plugins:test and codeStyle pass here.

Four small things left, none of them blocking: an incomplete catch, an asymmetry with which artifacts the runtime reads an index from, an undocumented reason the getter branch matters, and two doc points.

Comment thread grails-doc/src/en/guide/scaffolding.adoc Outdated
@testlens-app

testlens-app Bot commented Sep 24, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: 9276e6b
▶️ Tests: 82246 executed
⚪️ Checks: 88/88 completed


Learn more about TestLens at testlens.app/docs.

@jdaugherty jdaugherty added this to the grails:8.0.0-RC2 milestone Sep 24, 2026
@jdaugherty
jdaugherty merged commit 2b6f364 into apache:8.0.x Sep 24, 2026
91 checks passed
@codeconsole

Copy link
Copy Markdown
Contributor Author

@jdaugherty Thanks for the review and the merge. Your question about how broad the namespace rule needed to be led to a follow-up, #16398, which replaces this approach instead of hardening it further.

Rather than putting generated pages in the controllers' view directories and predicting the resolver's precedence at build time, the build compiles one page per template and domain class under grails-scaffolded/. The page's name covers the template and model it was expanded from. The resolver still decides which template a request uses, exactly as at runtime, and only swaps the expansion for the compiled page when one matches. So nothing can shadow a declared view. Namespaced controllers, same-named controllers scaffolding different domains, and namespace-specific templates are now precompiled too. The plugin-index, namespace-detection and collision handling from this PR go away.

The pages are expanded by the application's own scaffolding library and Groovy, in a JVM on its runtime classpath, so the build and the resolver share one implementation. The description covers native images, costs, and what was and wasn't run.

jdaugherty pushed a commit that referenced this pull request Sep 25, 2026
8.0.x now carries #16385, which kept the path-based approach and hardened it. This branch replaces
that approach, so in the five files only #16385 touched - the generation task, the GSP plugin
wiring, their specs and the scaffolding guide - this branch's version is kept, apart from
GroovyPagePlugin.VIEWS_SERVER_PATH, which still names the views root the pages are compiled under.
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.

3 participants