codeconsole commented on code in PR #16385:
URL: https://github.com/apache/grails-core/pull/16385#discussion_r4089255687
##########
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:
Fixed in 94fc557516. The ancestor-read path now catches
IllegalArgumentException, IOException, and IndexOutOfBoundsException. The try
block covers both constructing ClassReader and visiting the ancestor, so
malformed bytecode encountered during either step follows the documented
fallback. Regression tests cover an unsupported version, truncated and empty
class files, and a stream that throws IOException.
##########
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:
Fixed in 94fc557516. JAR indexes now contribute views only when the JAR also
contains META-INF/grails-plugin.xml. Directory outputs remain ungated because a
project dependency can put its descriptor and compiled index in separate output
directories; the method documentation explains this distinction. Tests cover
descriptorless JARs and directories for both index locations, and the existing
plugin-JAR fixtures now include descriptors.
##########
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:
Addressed in 94fc557516. The hasNamespace javadoc now explains the renamed
trait field, the static getNamespace() accessor generated on the implementing
class, and why no interface walk is needed. The functional spec also compiles a
real Groovy trait with a static namespace and verifies that its scaffolded
implementing controller is excluded from shared view generation and produces
the native-image warning.
##########
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:
Addressed both points in 94fc557516. The guide now names
generateScaffoldedViews, stageGroovyPages, and compileGroovyPages separately,
gives the generated and staged output directories, and explains that
compilation runs its prerequisite tasks automatically. It also documents
controllers whose view directories collide while scaffolding different domains
and their need for concrete views in native images. The collision warning now
mentions that native-image requirement too. The two affected specs passed all
46 tests; module codeStyle and validateDependencyVersions passed as well.
--
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]
