jdaugherty commented on code in PR #16385:
URL: https://github.com/apache/grails-core/pull/16385#discussion_r4086402159
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/scaffolding/GenerateScaffoldedViewsTask.groovy:
##########
@@ -128,7 +135,11 @@ abstract class GenerateScaffoldedViewsTask extends
DefaultTask {
// a view the application wrote itself already wins at
runtime, so leaving it out
// keeps build-time and runtime resolution agreeing
if (declared.any {
it.path.endsWith("views/${controller.key}/${viewName}.gsp".toString()) }) {
- logger.info("Skipping ${controller.key}/${viewName}.gsp,
the application declares it")
+ logger.info('Skipping {}/{}.gsp, the application declares
it', controller.key, viewName)
+ continue
+ }
+ if
(pluginViews.contains("/WEB-INF/grails-app/views/${controller.key}/${viewName}.gsp".toString()))
{
Review Comment:
`/WEB-INF/grails-app/views/` is a bare literal here, duplicating the
`serverpath` set on `compileGroovyPages` in `GroovyPagePlugin` and the
runtime's `DefaultGroovyPageLocator.PATH_TO_WEB_INF_VIEWS`. If `serverpath`
ever changes, the compiler writes index keys under the new prefix while this
still tests the old one: `contains` returns false for everything, the
suppression becomes a silent no-op, and no test fails. A `private static final
String` at minimum, ideally derived from the same value `GroovyPagePlugin` sets.
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/scaffolding/GenerateScaffoldedViewsTask.groovy:
##########
@@ -189,39 +200,75 @@ abstract class GenerateScaffoldedViewsTask extends
DefaultTask {
templates
}
+ /** Compiled plugin pages win over runtime scaffolding, so they must also
win at build time. */
+ private Set<String> findPluginViews() {
+ Set<String> views = []
+ for (File entry : viewClasspath.files) {
+ Properties index = new Properties()
+ if (entry.isDirectory()) {
+ File resource = new File(entry, 'gsp/views.properties')
Review Comment:
`BinaryGrailsPlugin.initializeViewMap` resolves `views.properties` relative
to the plugin descriptor first — i.e. `META-INF/views.properties` — and only
falls back to `gsp/views.properties`. Nothing in this repository writes the
first location, so this is a legacy packaging shape, but if a plugin carrying
it is on the classpath the runtime serves its pages while this check
contributes nothing and the shadowing returns for that plugin. Either probe
both locations, or note here that the legacy location is deliberately out of
scope.
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/scaffolding/GenerateScaffoldedViewsTask.groovy:
##########
@@ -189,39 +200,75 @@ abstract class GenerateScaffoldedViewsTask extends
DefaultTask {
templates
}
+ /** Compiled plugin pages win over runtime scaffolding, so they must also
win at build time. */
+ private Set<String> findPluginViews() {
+ Set<String> views = []
+ for (File entry : viewClasspath.files) {
+ Properties index = new Properties()
+ if (entry.isDirectory()) {
+ File resource = new File(entry, 'gsp/views.properties')
+ if (resource.isFile()) {
+ resource.withInputStream { InputStream input ->
index.load(input) }
+ }
+ }
+ else if (entry.name.endsWith('.jar') && entry.isFile()) {
+ new JarFile(entry).withCloseable { JarFile jar ->
+ JarEntry resource = jar.getJarEntry('gsp/views.properties')
+ if (resource != null) {
+ jar.getInputStream(resource).withCloseable {
InputStream input -> index.load(input) }
+ }
+ }
+ }
+ views.addAll(index.stringPropertyNames())
+ }
+ views
+ }
+
/**
* Maps view directory name to the fully qualified domain class, for every
{@code @Scaffold}
* controller. Qualified rather than simple because a view declaring the
type of its model has to
* name a type that resolves.
*
- * <p>A view directory is named for the controller alone - {@code
getDeployedViewURI} builds
- * {@code /WEB-INF/grails-app/views/<controller>/<view>.gsp} and never
consults the namespace -
- * so two controllers of the same simple name in different packages share
one directory whatever
- * their namespaces are. Where they scaffold different domains, no single
page can serve both:
- * whichever was written would declare one domain as its model and be
rendered by the controller
- * of the other. Both are left out rather than one of them guessed at, and
the resolver goes on
- * expanding a template per request for them, which is what it did before
any of this and is the
- * one thing that gets each controller its own domain. Everything else in
the project is still
- * precompiled.</p>
+ * <p>Namespaced controllers are left to the runtime resolver, which can
evaluate the namespace
+ * and select namespace-specific templates. Emitting their pages into a
shared, unqualified
+ * directory would make them visible to unrelated controllers. The entire
shared directory is
+ * left out, including when an unqualified controller also claims it.</p>
Review Comment:
Worth pinning down how much this rule needs to cover, since it is the part
of the change that removes capability rather than adding it.
With `enableNamespaceViewDefaults` false — the default —
`ScaffoldingViewResolver.loadView` prefers a namespace-specific template only
when `<namespace>/<view>.gsp` actually resolves; otherwise it expands the same
plain template this task would have precompiled. So for a namespaced controller
with no namespace-specific template, dropping the whole directory costs
precompilation, and in a native image the page entirely, for output that would
have been byte-identical.
Narrowing by reading the namespace *value* isn't available here, so that
isn't the answer: for `static namespace = 'admin'` and `static String namespace
= 'admin'` Groovy emits a private static field assigned in `<clinit>` with no
`ConstantValue` attribute, so the `value` argument in `visitField` is null —
only the `static final String` form carries one. The narrowing that is
implementable is to suppress only when some `<ns>/<view>.gsp` exists in the
template set `loadTemplates()` already reads.
If the blanket rule is the deliberate trade-off, a sentence here on why
would save the next reader the same analysis.
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/scaffolding/GenerateScaffoldedViewsTask.groovy:
##########
@@ -189,39 +200,75 @@ abstract class GenerateScaffoldedViewsTask extends
DefaultTask {
templates
}
+ /** Compiled plugin pages win over runtime scaffolding, so they must also
win at build time. */
+ private Set<String> findPluginViews() {
+ Set<String> views = []
+ for (File entry : viewClasspath.files) {
+ Properties index = new Properties()
+ if (entry.isDirectory()) {
+ File resource = new File(entry, 'gsp/views.properties')
+ if (resource.isFile()) {
+ resource.withInputStream { InputStream input ->
index.load(input) }
+ }
+ }
+ else if (entry.name.endsWith('.jar') && entry.isFile()) {
+ new JarFile(entry).withCloseable { JarFile jar ->
+ JarEntry resource = jar.getJarEntry('gsp/views.properties')
+ if (resource != null) {
+ jar.getInputStream(resource).withCloseable {
InputStream input -> index.load(input) }
+ }
+ }
+ }
+ views.addAll(index.stringPropertyNames())
+ }
+ views
+ }
+
/**
* Maps view directory name to the fully qualified domain class, for every
{@code @Scaffold}
* controller. Qualified rather than simple because a view declaring the
type of its model has to
* name a type that resolves.
*
- * <p>A view directory is named for the controller alone - {@code
getDeployedViewURI} builds
- * {@code /WEB-INF/grails-app/views/<controller>/<view>.gsp} and never
consults the namespace -
- * so two controllers of the same simple name in different packages share
one directory whatever
- * their namespaces are. Where they scaffold different domains, no single
page can serve both:
- * whichever was written would declare one domain as its model and be
rendered by the controller
- * of the other. Both are left out rather than one of them guessed at, and
the resolver goes on
- * expanding a template per request for them, which is what it did before
any of this and is the
- * one thing that gets each controller its own domain. Everything else in
the project is still
- * precompiled.</p>
+ * <p>Namespaced controllers are left to the runtime resolver, which can
evaluate the namespace
+ * and select namespace-specific templates. Emitting their pages into a
shared, unqualified
+ * directory would make them visible to unrelated controllers. The entire
shared directory is
+ * left out, including when an unqualified controller also claims it.</p>
+ *
+ * <p>Likewise, controllers sharing a name but scaffolding different
domains cannot share a
+ * precompiled page. The runtime resolver expands a template for the
appropriate domain.</p>
*/
private Map<String, String> findScaffoldedControllers() {
Map<String, String> found = [:]
Map<String, List<String>> claimants = [:]
- for (File dir : classesDirs.files) {
- if (!dir.isDirectory()) {
- continue
- }
- dir.eachFileRecurse { File f ->
- if (!f.name.endsWith('Controller.class')) {
- return
+ Set<String> namespaced = []
+ URL[] classpath = (classesDirs.files +
templateClasspath.files).collect { it.toURI().toURL() } as URL[]
+ new URLClassLoader(classpath, (ClassLoader) null).withCloseable {
URLClassLoader resources ->
+ for (File dir : classesDirs.files) {
+ if (!dir.isDirectory()) {
+ continue
}
- String domain = readScaffoldDomain(f)
- if (domain == null) {
- return
+ dir.eachFileRecurse { File f ->
+ if (!f.name.endsWith('Controller.class')) {
+ return
+ }
+ String controllerName = decapitalize(f.name -
'Controller.class')
Review Comment:
`decapitalize` isn't how Grails derives a controller's view directory.
`AbstractGrailsClass` builds it as
`getPropertyNameRepresentation(getLogicalName(clazz, 'Controller'))`, and
`GrailsNameUtils.getPropertyNameRepresentation` returns the name **unchanged**
when the first two characters are uppercase. Running both formulas against the
real `GrailsNameUtils`:
```
APIController runtime=API task=aPI MISMATCH
RSSFeedController runtime=RSSFeed task=rSSFeed MISMATCH
XMLExportController runtime=XMLExport task=xMLExport MISMATCH
URLMappingController runtime=URLMapping task=uRLMapping MISMATCH
EventController runtime=event task=event ok
AController runtime=a task=a ok
```
For an acronym-prefixed controller this writes `aPI/show.gsp`, a path the
resolver never asks for — precompiled and then never rendered. It also puts
both precedence checks on the wrong key: the handwritten-override test on line
137 and the new plugin-index test on line 141 both miss, so a plugin's
`API/show.gsp` is still shadowed, which is the case this PR is closing.
`grails.util.GrailsNameUtils` is already a dependency of this module and
already used in it — `GrailsCliGradlePlugin` calls
`getLogicalPropertyName(name, 'Command')`, the same shape needed here. The
generation half predates this PR, but since the new check inherits the key it
seems worth fixing together.
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/scaffolding/GenerateScaffoldedViewsTask.groovy:
##########
@@ -189,39 +200,75 @@ abstract class GenerateScaffoldedViewsTask extends
DefaultTask {
templates
}
+ /** Compiled plugin pages win over runtime scaffolding, so they must also
win at build time. */
+ private Set<String> findPluginViews() {
+ Set<String> views = []
+ for (File entry : viewClasspath.files) {
+ Properties index = new Properties()
+ if (entry.isDirectory()) {
+ File resource = new File(entry, 'gsp/views.properties')
+ if (resource.isFile()) {
+ resource.withInputStream { InputStream input ->
index.load(input) }
+ }
+ }
+ else if (entry.name.endsWith('.jar') && entry.isFile()) {
+ new JarFile(entry).withCloseable { JarFile jar ->
+ JarEntry resource = jar.getJarEntry('gsp/views.properties')
+ if (resource != null) {
+ jar.getInputStream(resource).withCloseable {
InputStream input -> index.load(input) }
+ }
+ }
+ }
+ views.addAll(index.stringPropertyNames())
+ }
+ views
+ }
+
/**
* Maps view directory name to the fully qualified domain class, for every
{@code @Scaffold}
* controller. Qualified rather than simple because a view declaring the
type of its model has to
* name a type that resolves.
*
- * <p>A view directory is named for the controller alone - {@code
getDeployedViewURI} builds
- * {@code /WEB-INF/grails-app/views/<controller>/<view>.gsp} and never
consults the namespace -
- * so two controllers of the same simple name in different packages share
one directory whatever
- * their namespaces are. Where they scaffold different domains, no single
page can serve both:
- * whichever was written would declare one domain as its model and be
rendered by the controller
- * of the other. Both are left out rather than one of them guessed at, and
the resolver goes on
- * expanding a template per request for them, which is what it did before
any of this and is the
- * one thing that gets each controller its own domain. Everything else in
the project is still
- * precompiled.</p>
+ * <p>Namespaced controllers are left to the runtime resolver, which can
evaluate the namespace
+ * and select namespace-specific templates. Emitting their pages into a
shared, unqualified
+ * directory would make them visible to unrelated controllers. The entire
shared directory is
+ * left out, including when an unqualified controller also claims it.</p>
+ *
+ * <p>Likewise, controllers sharing a name but scaffolding different
domains cannot share a
+ * precompiled page. The runtime resolver expands a template for the
appropriate domain.</p>
*/
private Map<String, String> findScaffoldedControllers() {
Map<String, String> found = [:]
Map<String, List<String>> claimants = [:]
- for (File dir : classesDirs.files) {
- if (!dir.isDirectory()) {
- continue
- }
- dir.eachFileRecurse { File f ->
- if (!f.name.endsWith('Controller.class')) {
- return
+ Set<String> namespaced = []
+ URL[] classpath = (classesDirs.files +
templateClasspath.files).collect { it.toURI().toURL() } as URL[]
Review Comment:
This reuses `templateClasspath` as the class-resolution classpath, against
the contract its own javadoc states just above ("The classpath the scaffolding
templates are read from"). Scoping that input to artifacts that actually carry
`META-INF/templates/scaffolding/` is an obvious later optimisation, since
`loadTemplates()` enumerates every entry of every jar on it — and it would
break inherited-namespace detection silently rather than loudly:
`getResourceAsStream` returns null, `hasNamespace` returns false, and
namespaced controllers quietly start being precompiled again. Only the one spec
that stuffs a base-class jar onto `templateClasspath` would notice.
A separate `@Classpath` property wired from `compileClasspath` keeps the two
independent.
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/scaffolding/GenerateScaffoldedViewsTask.groovy:
##########
@@ -237,6 +284,33 @@ abstract class GenerateScaffoldedViewsTask extends
DefaultTask {
found
}
+ /** Read declarations, including inherited ones, without evaluating
application code. */
+ private boolean hasNamespace(ClassReader reader, ClassLoader resources) {
+ boolean declared = false
+ reader.accept(new ClassVisitor(Opcodes.ASM9) {
+ @Override
+ FieldVisitor visitField(int access, String name, String
descriptor, String signature, Object value) {
+ if (name == 'namespace' && (access & Opcodes.ACC_STATIC) != 0)
{
Review Comment:
Worth a comment that this deliberately cannot distinguish a
declared-but-empty namespace. The runtime reads the property with
`getStaticPropertyValue(NAMESPACE_PROPERTY, String)` and
`ScaffoldingViewResolver` gates on Groovy truth, so `static namespace = null`
has no namespace at runtime but still drops the whole view directory here.
It is a limitation rather than something to fix — with `SKIP_CODE`, and with
the value living in `<clinit>` for the usual Groovy declaration forms, it isn't
recoverable — but saying so would stop the next reader trying.
##########
grails-gradle/plugins/src/test/groovy/org/grails/gradle/plugin/views/gsp/GroovyPagePluginFunctionalSpec.groovy:
##########
@@ -128,4 +128,83 @@ class GroovyPagePluginFunctionalSpec extends
GradleSpecification {
and: 'so its test task is not put behind compiling pages it does not
read'
result.output.contains('TEST_WAITS_FOR_PAGE_COMPILATION=false')
}
+
+ def "staged scaffold views preserve namespaces and runtime plugin pages"()
{
+ given:
+ def runner = setupTestResourceProject('gsp-compile-classpath')
+ File projectDir = runner.projectDir
+ new File(projectDir, 'build.gradle').append("""
+ dependencies {
+ implementation localGroovy()
+ runtimeOnly files('calendar-plugin')
+ }
+ sourceSets.main.groovy.srcDir('grails-app/controllers')
+ """)
+ // Only the annotation's bytecode is consumed by the task; no
application is started.
+ Map<String, String> sources = [
+
'src/main/groovy/grails/plugin/scaffolding/annotation/Scaffold.groovy': '''
+ package grails.plugin.scaffolding.annotation
+ import java.lang.annotation.Retention
+ import java.lang.annotation.RetentionPolicy
+ @Retention(RetentionPolicy.RUNTIME)
+ @interface Scaffold { Class value() }
+ ''',
+ 'grails-app/controllers/admin/EventController.groovy': '''
+ package admin
+ import grails.plugin.scaffolding.annotation.Scaffold
+ @Scaffold(String)
+ class EventController { static namespace = 'admin' }
+ ''',
+ 'grails-app/controllers/admin/DashboardController.groovy': '''
+ package admin
+ class DashboardController { static namespace = 'admin' }
+ ''',
+ 'grails-app/controllers/PersonController.groovy': '''
+ import grails.plugin.scaffolding.annotation.Scaffold
+ @Scaffold(String)
+ class PersonController { }
+ ''',
+ 'grails-app/controllers/BookController.groovy': '''
+ import grails.plugin.scaffolding.annotation.Scaffold
+ @Scaffold(String)
+ class BookController { }
+ ''',
+ 'src/main/templates/scaffolding/show.gsp': 'scaffold ${className}',
+ 'grails-app/views/person/index.gsp': 'handwritten index',
+ 'calendar-plugin/gsp/views.properties': '''
+ /WEB-INF/grails-app/views/event/show.gsp=calendar_event_show
+ /WEB-INF/grails-app/views/person/show.gsp=calendar_person_show
+ '''
+ ]
+ sources.each { String path, String content ->
+ File file = new File(projectDir, path)
+ file.parentFile.mkdirs()
+ file.text = content.stripIndent()
+ }
+
+ when:
+ def result = executeTask('stageGroovyPages')
+ File staged = new File(projectDir, 'build/generated/views')
+
+ then: 'the GSP compiler never receives an application page that would
shadow a plugin'
+ assertTaskSuccess('stageGroovyPages', result)
+ !new File(staged, 'event/show.gsp').exists()
+ !new File(staged, 'person/show.gsp').exists()
+ new File(staged, 'book/show.gsp').text == 'scaffold String'
+ new File(staged, 'person/index.gsp').text == 'handwritten index'
+
+ and: 'only skipped scaffold views warn about the native-image
requirement'
+ result.output.contains('Not precompiling the views of event:')
+ result.output.contains('native images require concrete GSP views')
+ !result.output.contains('Not precompiling the views of dashboard:')
+
+ when: 'a runtime dependency no longer provides a page, invalidating
the generation task'
+ new File(projectDir, 'calendar-plugin/gsp/views.properties').text = ''
+ executeTask('stageGroovyPages')
Review Comment:
The result isn't asserted here, so a failure or an unexpected `UP-TO-DATE`
on the incremental rebuild surfaces as a `FileNotFoundException` on line 206
rather than as the Gradle failure that caused it.
`assertTaskSuccess('stageGroovyPages', result)` as in the first phase.
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/scaffolding/GenerateScaffoldedViewsTask.groovy:
##########
@@ -237,6 +284,33 @@ abstract class GenerateScaffoldedViewsTask extends
DefaultTask {
found
}
+ /** Read declarations, including inherited ones, without evaluating
application code. */
+ private boolean hasNamespace(ClassReader reader, ClassLoader resources) {
+ boolean declared = false
+ reader.accept(new ClassVisitor(Opcodes.ASM9) {
+ @Override
+ FieldVisitor visitField(int access, String name, String
descriptor, String signature, Object value) {
+ if (name == 'namespace' && (access & Opcodes.ACC_STATIC) != 0)
{
+ declared = true
+ }
+ null
+ }
+
+ @Override
+ MethodVisitor visitMethod(int access, String name, String
descriptor, String signature, String[] exceptions) {
+ if (name == 'getNamespace' && descriptor.startsWith('()') &&
(access & Opcodes.ACC_STATIC) != 0) {
+ declared = true
+ }
+ null
+ }
+ }, ClassReader.SKIP_CODE | ClassReader.SKIP_DEBUG |
ClassReader.SKIP_FRAMES)
+ if (declared || reader.superName == null || reader.superName ==
'java/lang/Object') {
+ return declared
+ }
+ InputStream parent =
resources.getResourceAsStream("${reader.superName}.class")
+ parent == null ? false : parent.withCloseable { InputStream input ->
hasNamespace(new ClassReader(input), resources) }
Review Comment:
Two things about the ancestor walk.
It now feeds *dependency* bytecode to `ClassReader`; before this change only
freshly compiled project classes were parsed. A class-file major version newer
than the bundled `groovyjarjarasm` throws `IllegalArgumentException` and fails
the build with a message naming neither the controller nor the jar. Treating an
unreadable ancestor as declaring no namespace would degrade more gracefully.
And the walk has no memoization, so a base class shared by N controllers is
re-read and re-parsed N times — the per-class double read is gone now that
`reader` is shared, but this one remains. A `Map<String, Boolean>` keyed on the
internal name would cover it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
