M6 Bug Fix generated GSP views: scaffolded count model field type and welcome page static compilation - #16322
Conversation
The scaffolding Service template declares `Long count()` and the
Controller template passes its result as the `${propertyName}Count`
model value, but index.gsp declared the matching typed model field as
`Integer`. Model fields are populated by reflective Field.set, which
performs no numeric conversion, so every scaffolded index view threw
IllegalArgumentException on first render.
Declare the field as Long so it matches what the service returns.
GroovyPage.applyModelFieldsFromBinding only caught IllegalAccessException,
which cannot occur because GroovyPageMetaInfo already makes each model
field accessible. The one failure that can occur, a type mismatch, escaped
uncaught and surfaced as a bare JDK reflection message naming the mangled
generated page class. Catch IllegalArgumentException and report the field,
the declared type, the supplied type and the page instead.
Enabling static GSP compilation in a generated app:
grails {
compileStatic {
all = true
gsp = true
}
}
made compileGroovyPages fail on the welcome page. Four sort closures took
untyped parameters, so static type checking inferred java.lang.Object for
the element and rejected the property and method calls on it:
Cannot find matching method java.lang.Object#toLowerCase()
No such property: name for class: java.lang.Object
Type the closure parameters, and convert the values whose static type is
Object rather than String. The plugin and mime type comparisons need the
concrete element type, so those are cast; grails.plugins.GrailsPlugin is
referenced by its qualified name because GSP already auto-imports the
unrelated grails.plugins.metadata.GrailsPlugin annotation.
The forge resource and the web profile skeleton carry byte-identical
copies of this page, so both are updated.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16322 +/- ##
==================================================
+ Coverage 54.7302% 54.7318% +0.0017%
+ Complexity 20538 20537 -1
==================================================
Files 2104 2104
Lines 101149 101155 +6
Branches 17966 17966
==================================================
+ Hits 55359 55364 +5
+ Misses 37772 37771 -1
- Partials 8018 8020 +2
🚀 New features to boost your workflow:
|
The mime type sort closure spelled out grails.web.mime.MimeType at both parameters, which is long and reads poorly. Nothing in the GSP default imports declares a competing MimeType, so a page import works here and the closure reads as `MimeType a, MimeType b`. The plugin comparison keeps its qualified name: GSP always imports grails.plugins.metadata.GrailsPlugin, so importing grails.plugins.GrailsPlugin fails with "The name GrailsPlugin is already declared".
The previous commit made the page compile statically, but did it by
annotating the comparators rather than by restoring the element types,
which left three closures heavier than the rest of the page. Every other
sort here reads `sort { it.something }`.
Derive the domain grouping key as a String instead of from an untyped
local, and the sort needs no annotation at all. Declare the listener maps
as Map<String, String>, and the toString() calls guarding the comparison
are unnecessary. Sort the plugin rows by a single key so the cast appears
once rather than on both sides.
The plugin comparison still needs its cast and its qualified name: the
rows are heterogeneous maps, so the value is Object, and GSP always
imports grails.plugins.metadata.GrailsPlugin.
Do not name a closure parameter `it`. It compiles ahead of time, but the
page is parsed again at runtime when reloading is on, and that path
rejects it with "The current parameter list already contains a parameter
of the name it".
Changing the field to Long fixed views generated alongside the generated
service, which declares Long count(), but broke dynamic scaffolding. A
controller using `static scaffold = X` is backed by RestfulController,
whose countResources() returns Integer, so those pages then failed the
other way:
Model field 'authorCount' is declared as java.lang.Long
but the model supplied an instance of java.lang.Integer
One template serves both paths, so it cannot name either concrete type.
Number accepts what each supplies, and the pagination comparison still
compiles statically against it.
Covered by a test that renders the field from Integer, Long, Short and
BigInteger, so neither supplier can regress the other again.
✅ All tests passed ✅🏷️ Commit: 8855b88 Learn more about TestLens at testlens.app/docs. |
codeconsole
changed the title
AI Review FindingsHead
The findings below are about the choice the [P2] Assign model fields the way Groovy assigns, instead of declaring that model values are not coercedReferences:
The new message states a rule, "model values are not coerced", that is the opposite of what the rest of the page does. Integer i = 42L // 42, java.lang.Integer
int p = 42L // 42
String s = "${40+2}" // GString assigned to StringA model field declared
try {
field.set(this, DefaultTypeTransformation.castToType(value, field.getType()));
} catch (IllegalArgumentException | GroovyCastException e) {
throw new GroovyPagesException("Model field '" + field.getName() + "' is declared as " +
field.getType().getName() + " but the model supplied an instance of " +
value.getClass().getName() + '.', e, -1, getGroovyPageFileName());
}I ran the PR's spec with that in place plus a data-driven case:
The only existing test that changes is a model value of the wrong type names the field and both types, which would flip from The [P2] Document what a declared model field does with a value of another typeReferences:
The guide says nothing about what happens when the supplied value's type differs from the declaration. The only nearby sentence, at line 208, covers the framework-supplied names and says they fail with a [P2] Neither failure had a test that could catch it, and this PR adds coverage for the engine but not for the two pagesReferences:
CI was green on
Nit: the listener sort hunk is not neededReferences:
Nit: the scaffolding guide describes model names that the templates no longer useReferences:
Pre-existing and out of scope, noting it because it is the doc a reader would go to for the count field: it says the standard views expect Confirmed
Verification
|
3 participants
Description
Two defects that stop a freshly generated app from working. Both are in the generated GSP views, and both surface the moment you use the app the generator produces.
1. Scaffolded
index.gspdeclares the count model field asIntegergrails generate-viewsproduces anindex.gspwhose typed model directive declares:but the
Service.groovytemplate declaresLong count(), andController.groovypasses that value straight through as the${propertyName}Countmodel entry. Model fields are populated by reflectiveField.set, which does no numeric conversion, so every scaffolded index view throws on first render:Declaring the field as
Longmatches what the service returns.While tracking this down, the error itself proved harder to read than it needed to be.
GroovyPage.applyModelFieldsFromBindingcaught onlyIllegalAccessException, which cannot occur —GroovyPageMetaInfo.initializeModelFieldsalready callsReflectionUtils.makeAccessibleon every model field. The one failure that can occur, a type mismatch, escaped uncaught and surfaced as a bare JDK reflection message naming the mangled generated page class. It now reports something actionable:Strict assignment is kept deliberately — coercing would silently narrow
LongtoInteger, and the typed model directive exists to give these pages real types.2. Welcome page does not compile under GSP static compilation
Adding the following to a generated app's
build.gradlefailscompileGroovyPages:grails { compileStatic { all = true gsp = true } }Four
sortclosures in the welcome page take untyped parameters, so static type checking infersObjectfor the element and rejects the calls on it. Each loses the element type for a different reason:Object.collect { p, i -> [plugin: p, order: ...] }— map literal values areObjectdomainsByPlugingroupByclosure returns adeflocal, so the key isObjectappListenersmimeTypesapplicationContext.getBean('mimeTypes')returnsObjectFixed by typing the closure parameters and converting the values whose static type is
Object. The plugin and mime type comparisons need the concrete element type, so those are cast.One wrinkle worth recording:
grails.plugins.GrailsPlugincannot be imported into a GSP, because the compiler already auto-imports the unrelatedgrails.plugins.metadata.GrailsPluginannotation and the collision fails the build withThe name GrailsPlugin is already declared. The cast uses the qualified name instead.grails-forge-core's resource copy and the web profile skeleton copy of this page were byte-identical, so both are updated and remain identical.