Repository navigation
Preserve view precedence when precompiling scaffolds - #16385
jdaugherty merged 8 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ 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
🚀 New features to boost your workflow:
|
matrei
left a comment
There was a problem hiding this comment.
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 staticnamespace, scaffolded or not, becausenamespacedis collected beforereadScaffoldDomainfilters. 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 ofevent:admin.EventControllerdeclares 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, andScaffoldingViewResolvertreats a view whose URL contains/<namespace>/as namespace-specific. So a namespaced controller's pages could be precompiled intoadmin/event/*.gsp(fromadmin/<view>.gspunder 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. Astatic final String namespace = 'admin'carries aConstantValueattribute; the usualstatic namespace = 'admin'is assigned in<clinit>, which is anLDC/PUTSTATICpair ASM can read without loading the class. Worth an issue if there is appetite; the PR's test explicitly assertsadmin/eventis not produced, so that decision would want revisiting then.
Nits
GenerateScaffoldedViewsTask.groovy:91-93(and the pre-existingtemplateClasspathat 86-88): a resolved configuration as a task input is what@Classpathis 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 beforelogger.infohas checked the level, and the first sits inside the per-view loop. Gradle's logger is SLF4J, sologger.info('Skipping {}/{}.gsp, a plugin declares it', controller.key, viewName)andlogger.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 forhasNamespaceand once insidereadScaffoldDomain. OneClassReadercould serve both visitors. Cheap either way.GenerateScaffoldedViewsTaskSpec.groovy:335andGroovyPagePluginFunctionalSpec.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 duringcompileGroovyPagesso packaged applications and native images do not generate them on first request would anchor the two paragraphs that follow.
Confirmed
hasNamespacematches whatDefaultGrailsControllerClassreads:getStaticPropertyValue("namespace"), which a Groovystatic namespace = 'admin'satisfies through both the private static field and the generated static getter, and a Javapublic static String namespacethrough the field. The instance-property case is correctly left alone (the resolver would not see a namespace either). Inheritance walkssuperNamethrough a parent-lessURLClassLoaderoverclassesDirsplus the compile classpath, which is the only place a superclass can be, and stops atjava/lang/Objector 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.EventControllerwith noshowview falls through to/event/show, where nothing exists because scaffolding generates per requesting controller; a precompiledevent/show.gspwould have changed that in the archive. The guide sentence covers it. viewClasspathisconfigurations.runtimeClasspath, not the source set's runtime classpath, so the project's own compiled pages are not on it andruntimeOnlyplugins 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 throughruntimeOnly 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, andstageGroovyPagesis aSync, so it disappears from the compilation input too. The unit test covers both directions. templateClasspathandviewClasspathare read at execution time fromConfigurableFileCollections 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:codeStyleona820aab753in a scratch worktree: BUILD SUCCESSFUL in 1m 4s.GenerateScaffoldedViewsTaskSpec22 tests,GroovyPagePluginFunctionalSpec7 tests, 0 failures, 0 errors; result XML timestamps 18:38 today.codenarcMainran clean;checkstyleMainhas 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 against96e0e72f68.- Runtime claims checked by reading, not running:
DefaultGroovyPageLocatorlookup order andisPrecompiledAvailable(),ScaffoldingViewResolver.loadView,GrailsConventionGroovyPageLocator.findView,DefaultGrailsControllerClassnamespace reading, and theserverpaththe plugin passes tocompileGroovyPages. - The scratch worktree is removed; the main checkout was not touched.
jdaugherty
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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.
✅ All tests passed ✅🏷️ Commit: 9276e6b Learn more about TestLens at testlens.app/docs. |
|
@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 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. |
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.
An application with a namespaced
@Scaffoldcontroller can render a different page after packaging than it does underbootRun. For example, an adminEventControllergenerates an application-level/event/show.gspthat 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:
runtimeOnlyplugins. Read both supported plugin index locations in runtime order, and require a plugin descriptor for JARs.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:
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.94fc557516) passed all 46 tests inGenerateScaffoldedViewsTaskSpecandGroovyPagePluginFunctionalSpec, plus modulecodeStyleandvalidateDependencyVersions. Coverage includes inherited and trait namespaces, acronym naming, plugin-index precedence, descriptorless JARs, unreadable ancestor classes, stale output removal, and incremental staging.