jdaugherty commented on code in PR #16385:
URL: https://github.com/apache/grails-core/pull/16385#discussion_r4088090751
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/scaffolding/GenerateScaffoldedViewsTask.groovy:
##########
@@ -189,54 +217,163 @@ abstract class GenerateScaffoldedViewsTask extends
DefaultTask {
templates
}
+ /**
+ * Compiled plugin pages win over runtime scaffolding, so they must also
win at build time. Each
+ * artifact contributes the first index it carries, as the runtime reads
only one per plugin.
+ */
+ private Set<String> findPluginViews() {
Review Comment:
This reads an index out of every artifact on the runtime classpath, where
the runtime reads one only out of a *binary plugin*: `BinaryGrailsPlugin` calls
`initializeViewMap` under `if (descriptor != null)`, and that descriptor is
`META-INF/grails-plugin.xml`. An artifact carrying a view index without one
contributes nothing at runtime but suppresses generation here, and the page is
then neither precompiled nor served by a plugin — on the JVM it falls back to
runtime expansion, in a native image it is gone.
What produces such an artifact is narrow: a Grails application jar taken as
a dependency, or a plugin repackaged without its descriptor. So leaving it
alone is a defensible call — a sentence saying so would close it the way the
`gsp/views.properties` fallback is closed just above. If it is worth matching,
gating a **jar** entry on `META-INF/grails-plugin.xml` is exact. The directory
branch cannot be gated the same way and would have to stay as it is: a plugin
consumed as a project dependency has its descriptor and its index in different
output directories, and the spec's `plugin-directory` fixture carries no
descriptor at all.
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/scaffolding/GenerateScaffoldedViewsTask.groovy:
##########
@@ -189,54 +217,163 @@ abstract class GenerateScaffoldedViewsTask extends
DefaultTask {
templates
}
+ /**
+ * Compiled plugin pages win over runtime scaffolding, so they must also
win at build time. Each
+ * artifact contributes the first index it carries, as the runtime reads
only one per plugin.
+ */
+ private Set<String> findPluginViews() {
+ Set<String> views = []
+ for (File entry : viewClasspath.files) {
+ Properties index = new Properties()
+ if (entry.isDirectory()) {
+ File resource = VIEW_INDEXES.collect { new File(entry, it)
}.find { it.isFile() }
+ resource?.withInputStream { InputStream input ->
index.load(input) }
+ }
+ else if (entry.name.endsWith('.jar') && entry.isFile()) {
+ new JarFile(entry).withCloseable { JarFile jar ->
+ JarEntry resource = VIEW_INDEXES.collect {
jar.getJarEntry(it) }.find { it != null }
+ 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>This is deliberately broader than it needs to be for a namespaced
controller that has no
+ * namespace-specific template, whose page would come out identical to the
plain one. Narrowing
+ * it needs to know whether {@code <namespace>/<view>.gsp} exists, and
neither half is available
+ * here: the namespace value is assigned in {@code <clinit>} for the usual
Groovy declarations,
+ * so the bytecode carries no constant for it, and the runtime also finds
namespace templates in
+ * places this task does not read - the application's own resources beside
the controller class,
+ * {@code src/main/templates/scaffolding} in development, and a
template-override plugin.
+ * Guessing wrong would precompile a plain page over a namespace-specific
one, silently.</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 = []
+ Map<String, Boolean> ancestors = [:]
+ URL[] classpath = (classesDirs.files +
controllerClasspath.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 = viewDirectory(f.name - '.class')
+ ClassReader reader = new ClassReader(f.bytes)
+ if (hasNamespace(reader, resources, ancestors)) {
+ namespaced.add(controllerName)
+ }
+ String domain = readScaffoldDomain(reader)
+ if (domain == null) {
+ return
+ }
+ claimants.computeIfAbsent(controllerName) { []
}.add(domain)
+ found.put(controllerName, domain)
}
- String controllerName = decapitalize(f.name -
'Controller.class')
- claimants.computeIfAbsent(controllerName) { [] }.add(domain)
- found.put(controllerName, domain)
+ }
+ }
+ namespaced.each { String controllerName ->
+ if (found.remove(controllerName) != null) {
+ logger.warn('Not precompiling the views of {}: a controller
with this name declares or inherits a namespace. ' +
+ 'These scaffold views are expanded at runtime; native
images require concrete GSP views.', controllerName)
}
}
claimants.each { String controllerName, List<String> domains ->
List<String> distinct = domains.unique(false)
if (distinct.size() > 1) {
found.remove(controllerName)
logger.warn("Not precompiling the views of ${controllerName}:
" +
- "${distinct.size()} controllers named
${capitalize(controllerName)}Controller " +
+ "${distinct.size()} controllers with the view
directory ${controllerName} " +
"scaffold different domains (${distinct.join(', ')})
and share the one view " +
'directory. They are expanded per request instead, as
they were before.')
}
}
found
}
+ /**
+ * Read declarations, including inherited ones, without evaluating
application code.
+ *
+ * <p>A declaration is all this can see, not its value, so {@code static
namespace = null}
+ * still counts even though the runtime, which tests the value, gives that
controller no
+ * namespace. The value lives in {@code <clinit>} for the usual Groovy
forms and code is not
+ * read here, so the difference cannot be recovered; the controller is
only expanded at runtime
+ * rather than precompiled.</p>
+ */
+ private boolean hasNamespace(ClassReader reader, ClassLoader resources,
Map<String, Boolean> ancestors) {
+ 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) {
Review Comment:
This branch carries more weight than it looks, and nothing records why. A
namespace supplied by a trait is the case that needs it. Compiled with Groovy
5.1.2:
```groovy
trait NsTrait { static String namespace = 'admin' }
class TraitController implements NsTrait { }
```
```
public class TraitController implements NsTrait, NsTrait$Trait$FieldHelper,
GroovyObject {
private static java.lang.String NsTrait__namespace;
public static java.lang.String getNamespace();
public static void setNamespace(java.lang.String);
}
```
The field is renamed `NsTrait__namespace`, so `visitField` never sees it;
only this check does. The same fact is what makes walking `superName` alone
correct — Groovy puts the static accessor on the implementing class, so there
is nothing further to find by following interfaces. As it stands, someone who
notices the walk ignores interfaces has no way to tell that from an oversight,
and "fixing" it would be wasted work.
The spec's `getter` case is hand-written bytecode that happens to have this
shape; it does not say where the shape comes from. A sentence in the
`hasNamespace` javadoc, or a trait case beside the field/getter pair, would pin
both halves.
##########
grails-doc/src/en/guide/scaffolding.adoc:
##########
@@ -59,6 +59,10 @@ With this configured, when you start your application the
actions and views will
A CRUD interface will also be generated. To access this open
`http://localhost:8080/book` in a browser.
+During `compileGroovyPages`, the Gradle plugin expands scaffold templates and
compiles the resulting GSP views so packaged applications and native images do
not need to generate them on the first request. Handwritten views in the
application or supplied by a plugin still take precedence over generated
scaffold views.
Review Comment:
Two things about this pair of paragraphs.
`compileGroovyPages` compiles the views, but it is `generateScaffoldedViews`
that expands the templates and `stageGroovyPages` that puts the result beside
`grails-app/views` for compilation. As written, a reader looking for where the
expanded pages land, or wanting to run just that step, is pointed at the wrong
task.
And the namespace rule is documented while its sibling is not. Two
controllers whose view directories collide and scaffold different domains also
stay runtime-generated, and for the same reason a native image needs concrete
views for them — the task warns about that case too, though only the namespace
warning mentions native images. A reader who hits the collision warning has
nothing in the guide to look it up in.
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/scaffolding/GenerateScaffoldedViewsTask.groovy:
##########
@@ -189,54 +217,163 @@ abstract class GenerateScaffoldedViewsTask extends
DefaultTask {
templates
}
+ /**
+ * Compiled plugin pages win over runtime scaffolding, so they must also
win at build time. Each
+ * artifact contributes the first index it carries, as the runtime reads
only one per plugin.
+ */
+ private Set<String> findPluginViews() {
+ Set<String> views = []
+ for (File entry : viewClasspath.files) {
+ Properties index = new Properties()
+ if (entry.isDirectory()) {
+ File resource = VIEW_INDEXES.collect { new File(entry, it)
}.find { it.isFile() }
+ resource?.withInputStream { InputStream input ->
index.load(input) }
+ }
+ else if (entry.name.endsWith('.jar') && entry.isFile()) {
+ new JarFile(entry).withCloseable { JarFile jar ->
+ JarEntry resource = VIEW_INDEXES.collect {
jar.getJarEntry(it) }.find { it != null }
+ 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>This is deliberately broader than it needs to be for a namespaced
controller that has no
+ * namespace-specific template, whose page would come out identical to the
plain one. Narrowing
+ * it needs to know whether {@code <namespace>/<view>.gsp} exists, and
neither half is available
+ * here: the namespace value is assigned in {@code <clinit>} for the usual
Groovy declarations,
+ * so the bytecode carries no constant for it, and the runtime also finds
namespace templates in
+ * places this task does not read - the application's own resources beside
the controller class,
+ * {@code src/main/templates/scaffolding} in development, and a
template-override plugin.
+ * Guessing wrong would precompile a plain page over a namespace-specific
one, silently.</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 = []
+ Map<String, Boolean> ancestors = [:]
+ URL[] classpath = (classesDirs.files +
controllerClasspath.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 = viewDirectory(f.name - '.class')
+ ClassReader reader = new ClassReader(f.bytes)
+ if (hasNamespace(reader, resources, ancestors)) {
+ namespaced.add(controllerName)
+ }
+ String domain = readScaffoldDomain(reader)
+ if (domain == null) {
+ return
+ }
+ claimants.computeIfAbsent(controllerName) { []
}.add(domain)
+ found.put(controllerName, domain)
}
- String controllerName = decapitalize(f.name -
'Controller.class')
- claimants.computeIfAbsent(controllerName) { [] }.add(domain)
- found.put(controllerName, domain)
+ }
+ }
+ namespaced.each { String controllerName ->
+ if (found.remove(controllerName) != null) {
+ logger.warn('Not precompiling the views of {}: a controller
with this name declares or inherits a namespace. ' +
+ 'These scaffold views are expanded at runtime; native
images require concrete GSP views.', controllerName)
}
}
claimants.each { String controllerName, List<String> domains ->
List<String> distinct = domains.unique(false)
if (distinct.size() > 1) {
found.remove(controllerName)
logger.warn("Not precompiling the views of ${controllerName}:
" +
- "${distinct.size()} controllers named
${capitalize(controllerName)}Controller " +
+ "${distinct.size()} controllers with the view
directory ${controllerName} " +
"scaffold different domains (${distinct.join(', ')})
and share the one view " +
'directory. They are expanded per request instead, as
they were before.')
}
}
found
}
+ /**
+ * Read declarations, including inherited ones, without evaluating
application code.
+ *
+ * <p>A declaration is all this can see, not its value, so {@code static
namespace = null}
+ * still counts even though the runtime, which tests the value, gives that
controller no
+ * namespace. The value lives in {@code <clinit>} for the usual Groovy
forms and code is not
+ * read here, so the difference cannot be recovered; the controller is
only expanded at runtime
+ * rather than precompiled.</p>
+ */
+ private boolean hasNamespace(ClassReader reader, ClassLoader resources,
Map<String, Boolean> ancestors) {
+ 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
+ }
+ String superName = reader.superName
+ Boolean known = ancestors.get(superName)
+ if (known != null) {
+ return known
+ }
+ boolean inherited = ancestorHasNamespace(superName, resources,
ancestors)
+ ancestors.put(superName, inherited)
+ inherited
+ }
+
+ /**
+ * Superclasses can come from dependencies, whose class files may be newer
than the bundled ASM
+ * reads. One that cannot be read is taken to declare no namespace rather
than failing the build.
+ */
+ private boolean ancestorHasNamespace(String internalName, ClassLoader
resources, Map<String, Boolean> ancestors) {
+ InputStream parent =
resources.getResourceAsStream("${internalName}.class")
+ if (parent == null) {
+ return false
+ }
+ ClassReader reader
+ try {
+ reader = parent.withCloseable { InputStream input -> new
ClassReader(input) }
+ }
+ catch (IllegalArgumentException e) {
Review Comment:
The catch is narrower than the failure it is here to absorb. Running the
bundled `groovyjarjarasm` reader over a few shapes of input:
```
intact read ok
future major version IllegalArgumentException: Unsupported class file major
version 200
not a class file IllegalArgumentException: Unsupported class file major
version 30575
truncated ArrayIndexOutOfBoundsException: Index 774 out of
bounds for length 758
empty ArrayIndexOutOfBoundsException: Index 6 out of bounds
for length 0
```
Only the version check raises `IllegalArgumentException`. The reader is
already into the constant pool before it can tell a truncated class file from a
whole one, so that case arrives as an index error instead. `new
ClassReader(InputStream)` also declares `IOException`, which a jar with a
damaged entry raises right here.
Both land where the javadoc above says the build carries on, and both fail
it with a stack trace naming neither the controller nor the jar — the outcome
the catch exists to prevent. Catching `IllegalArgumentException | IOException |
IndexOutOfBoundsException`, or just `Exception`, would make the behaviour match
what is written above 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]
