codeconsole commented on code in PR #16385:
URL: https://github.com/apache/grails-core/pull/16385#discussion_r4086804784
##########
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:
Fixed in 9f945f3ab0. The view directory is now derived as the runtime
derives it, `getPropertyNameRepresentation(getLogicalName(name,
'Controller'))`, so `APIController` writes `API/` and both precedence checks
key on `API/<view>.gsp`. The domain's `propertyName` binding had the same bug
(`URLMapping` came out as `uRLMapping`); it now goes through
`GrailsNameUtils.getPropertyName`, matching `ModelBuilder`. New specs cover the
directory, the binding, and both the application-view and plugin-index checks
for an acronym controller. They assert on the listed directory name, because a
case-insensitive file system resolves `aPI` to `API` by lookup.
##########
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:
Fixed in a4e129569c. `GroovyPagePlugin.VIEWS_SERVER_PATH` is now the value
`compileGroovyPages` is given for `serverpath` and the prefix the task matches
index keys against, so the two can't drift apart.
##########
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:
Fixed in a4e129569c. The task now probes `META-INF/views.properties` first
and falls back to `gsp/views.properties`, taking one index per artifact as
`initializeViewMap` does. Specs cover the descriptor-relative index in a
directory and in a jar, plus a plugin carrying both, where only the first is
honoured.
##########
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:
I kept the blanket rule and documented it in 68af07731d. The narrowing would
need to know whether `<ns>/<view>.gsp` exists, and the build can't answer that.
Besides the value being in `<clinit>`, the runtime also finds namespace
templates in places this task doesn't read: the application's own
`META-INF/templates/scaffolding/<ns>/` resources beside the controller class,
`src/main/templates/scaffolding/<ns>/` in development, and a template-override
plugin. Getting it wrong would silently precompile a plain page over a
namespace-specific one, so the javadoc now explains the trade-off.
##########
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:
Fixed in 68af07731d. There's a separate `@Classpath controllerClasspath`,
wired from `compileClasspath`, used only to resolve superclasses. The
inherited-from-jar spec now uses it, and a new spec checks that a base class
reachable only on `templateClasspath` is *not* searched, which pins the
independence.
##########
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:
Added to the `hasNamespace` javadoc in 68af07731d: only the declaration is
visible, not its value, so `static namespace = null` still counts. The cost is
runtime expansion rather than precompilation.
--
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]
