jdaugherty commented on code in PR #16398:
URL: https://github.com/apache/grails-core/pull/16398#discussion_r4101152732
##########
grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/views/gsp/GroovyPagePlugin.groovy:
##########
@@ -397,9 +409,17 @@ class GroovyPagePlugin implements Plugin<Project> {
it.compileStaticStrict.set(false)
if (scaffolds) {
it.dependsOn(tasks.named('stageGroovyPages'))
+ // a scaffolded page that does not compile is left to be
produced when it is rendered,
+ // as it was before any was compiled, rather than failing the
build
+
it.generatedDirectories.add(GenerateScaffoldedViewsTask.PAGES_DIRECTORY)
Review Comment:
About the leftover classes listed as a known issue in the description: I
think this is worth handling in this PR. The digest naming makes it happen on
every template edit, and the leftovers get packaged.
After editing `summary.gsp` in the example and running `bootJar`, the jar
still contains the class for the old digest:
```
gsp_grails_test_examples_scaffolding_grails_scaffolded_com_example_Usersummary_f7575c8a00e64f4d4417154fc038879b_gsp.class
```
`gsp/views.properties` has no entry for it. It's never served, but every
artifact built from a workspace that hasn't been cleaned keeps collecting these
classes and their `.data` files until `clean`.
Every scaffolded page's class name shares the `grails_scaffolded` segment.
So after compiling, `compileGroovyPages` could remove scaffolded page classes
and data files that the new registry doesn't reference. Alternatively, the
scaffolded pages could compile into an output directory of their own that is
cleared on each run.
##########
grails-scaffolding/src/main/groovy/org/apache/grails/scaffolding/ScaffoldedPages.java:
##########
@@ -0,0 +1,124 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.grails.scaffolding;
+
+import java.io.IOException;
+import java.io.StringWriter;
+import java.nio.charset.StandardCharsets;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
+import java.util.HexFormat;
+import java.util.Map;
+import java.util.TreeSet;
+
+import groovy.text.GStringTemplateEngine;
+
+/**
+ * Expands a scaffolding template into a page, and names the page the build
compiles from it so
+ * that the runtime resolver can find it.
+ *
+ * <p>The build and the resolver both come here, so a page is expanded and
named by the same code
+ * whichever of them does it. A page is named for the template's path, for
readability, and for a
+ * digest of the template and of the value of each name of the model the
template mentions. A
+ * template the build did not see, or saw expanded with a different model, so
names a page that
+ * does not exist rather than a different one. A name the template never
mentions is left out, so
+ * that a value that differs between the machine that built the application
and the one running it
+ * - {@code packagePath} follows the file separator - does not cost the page;
a template that reads
+ * the model without naming what it reads, through {@code binding} for
instance, is named as though
+ * it did not read it.</p>
+ *
+ * @since 8.0
+ */
+public final class ScaffoldedPages {
+
+ /**
+ * The directory under the views root that holds the compiled pages. A
controller's views are
+ * resolved from a directory named after a Java identifier, which cannot
contain a hyphen, so no
+ * request for a controller's view reaches a page here.
+ */
+ public static final String DIRECTORY = "grails-scaffolded";
+
+ /** Bytes of the digest kept in a page's name. */
+ private static final int KEY_BYTES = 16;
Review Comment:
Nit: the digest and the fully qualified domain class name both go into every
compiled class name, and in the example those names reach 165 characters:
```
gsp_grails_test_examples_scaffolding_grails_scaffolded_com_example_community_Usercreate_b6f0ef269f3e30f60f28e24033275fd3_gsp$_run_closure2$_closure6$_closure8.class
```
With a deeper package and a longer entity name, a project under a typical
Windows user directory gets close to the 260-character `MAX_PATH` inside
`build/gsp-classes/main`. The JVM copes, but other tools that touch build
output (archivers, antivirus, some IDE indexers) may not.
The digest only has to tell apart copies of the same template for the same
domain class, because the domain class and the template path are already in the
URI. 8 bytes (16 hex characters) would leave plenty of margin and make every
name 16 characters shorter.
##########
grails-scaffolding/src/main/groovy/org/apache/grails/scaffolding/ScaffoldedPagesGenerator.groovy:
##########
@@ -0,0 +1,139 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.grails.scaffolding
+
+import groovy.transform.CompileStatic
+
+import grails.codegen.model.ModelBuilder
+
+/**
+ * Expands scaffolding templates into the pages the build compiles, run by the
Gradle plugin's
+ * {@code generateScaffoldedViews} task in a JVM on the application's own
classpath.
+ *
+ * <p>Running here rather than in the build means a page is modelled by the
application's
+ * {@link ModelBuilder}, expanded by the application's Groovy and named by
{@link ScaffoldedPages},
+ * exactly as the resolver models, expands and names it when the view is asked
for.</p>
+ *
+ * <pre>
+ * ScaffoldedPagesGenerator <plan> <origins> <output
directory> <page encoding>
+ * </pre>
+ *
+ * <p>Each line of the plan names a domain class and, tab separated, the
templates directories to
+ * expand for it. A templates directory holds a file per template path, such
as {@code show.gsp} or
+ * {@code admin/show.gsp}. Each line of the origins names a templates
directory and, after a tab,
+ * where its template came from. The pages are written in the encoding they
will be compiled
+ * with.</p>
+ *
+ * @since 8.0
+ */
+@CompileStatic
+class ScaffoldedPagesGenerator implements ModelBuilder {
+
+ static void main(String[] args) {
+ if (args.length != 4) {
+ System.err.println('Usage: ScaffoldedPagesGenerator <plan>
<origins> <output directory> <page encoding>')
+ System.exit(2)
+ }
+ Map<String, List<File>> plan = [:]
+ new File(args[0]).readLines('UTF-8').each { String line ->
+ List<String> fields = line.split('\t').toList()*.trim().findAll {
String field -> field }
+ if (fields) {
+ plan.put(fields.head(), fields.tail().collect { String dir ->
new File(dir) })
+ }
+ }
+ Map<File, String> origins = [:]
+ new File(args[1]).readLines('UTF-8').each { String line ->
+ int tab = line.indexOf('\t')
+ if (tab > 0) {
+ origins.put(new File(line.substring(0, tab)),
line.substring(tab + 1))
+ }
+ }
+ new ScaffoldedPagesGenerator().generate(plan, new File(args[2]),
args[3], origins)
+ }
+
+ /**
+ * Writes, under {@code outputDir} where the resolver looks for it, the
page for each domain
+ * class and each template in the directories planned for it. A template
that cannot be
+ * expanded for a domain class is reported and left out.
+ *
+ * <p>A page ends with a comment naming the template it was expanded from,
which renders as
+ * nothing, so that a page the build reports can be traced to the template
to fix.</p>
+ *
+ * @param plan each domain class, with the templates directories to expand
for it
+ * @param origins where the template in each templates directory came
from; the template's own
+ * file for a directory not named
+ * @return how many pages were written
+ */
+ int generate(Map<String, List<File>> plan, File outputDir, String encoding
= 'UTF-8', Map<File, String> origins = [:]) {
+ int written = 0
+ plan.each { String domain, List<File> templateDirs ->
+ Map<String, Object> model = model(domain).asMap()
+ for (Template template : read(templateDirs, origins)) {
+ String page
+ try {
+ page = ScaffoldedPages.expand(template.content, model)
+ }
+ catch (Exception e) {
Review Comment:
Being lenient makes sense for templates that come from dependencies,
including a stock copy the application has replaced, but I don't think it
should cover the application's own templates.
I appended an invalid `<%-- probe --%>` to
`grails-test-examples/scaffolding/src/main/templates/scaffolding/summary.gsp`
and `:grails-test-examples-scaffolding:bootJar` still succeeded. The only sign
was one stderr line per domain class:
```
Could not expand the scaffolding template summary for com.example.Book, so
no page is compiled for it; if it is rendered it fails the same way:
groovy.lang.MissingPropertyException: No such property: probe for class:
groovy.lang.Binding
```
On the JVM that turns into an error the first time the view is rendered, and
in a native image the failure is certain. Templates in
`src/main/templates/scaffolding`, or packaged by the app under
`META-INF/templates/scaffolding`, are the user's own code, just like a
hand-written GSP, and a hand-written GSP that doesn't compile still fails the
build under this PR.
The task already knows which copies belong to the application
(`templateOverrides` / `packagedTemplates`). Could an expansion failure for one
of those fail the task, while classpath copies keep the lenient path? The same
goes for a page expanded from an application template that then fails in
`compileGroovyPages`: `generatedDirectories` covers all of `grails-scaffolded`,
so that is also only a warning. If failing outright is too strict, an opt-in
strict flag would at least give native-image builds a way to catch this before
deploying.
--
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]
