M6 Bug Fix generated GSP views: scaffolded count model field type and welcome page static compilation - #16322
codeconsole wants to merge 11 commits into
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16322 +/- ##
==================================================
+ Coverage 55.2582% 55.5923% +0.3342%
- Complexity 20976 21127 +151
==================================================
Files 2113 2113
Lines 101632 101800 +168
Branches 18045 18099 +54
==================================================
+ Hits 56160 56593 +433
+ Misses 37422 37099 -323
- Partials 8050 8108 +58
🚀 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.
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
|
A page could convert everywhere except its declared model: <g:set type="int"> converts, and so does a plain Groovy assignment, but a model field declared Integer failed outright when a controller supplied a Long. That declaration is the natural one to write, since count() and the two scaffolding controllers do not agree on a type, and it is what views already generated for 8.0 contain. Convert with DefaultTypeTransformation.castToType, which is Groovy's own assignment conversion. On its own it would wrap 3_000_000_000L into -1294967296 and turn 42.9 into 42 without a word, so a conversion that changes a numeric value is refused, as is a value that cannot be converted at all. Both name the field and the two types. The GSP static compilation guide now says what a declared model does with a value of another type.
Both defects this branch fixes shipped with CI green, because nothing compiled the welcome page statically and nothing rendered the scaffolded index view's model declaration. gsp-compile-static now stages the web profile's welcome page beside its own pages and compiles them together. The forge copy is already pinned to the profile copy by GrailsGspSpec. ScaffoldedIndexViewModelSpec expands scaffolding/index.gsp the way dynamic scaffolding does and renders its model declaration with an Integer, a Long and a count past Integer.MAX_VALUE, which the Integer declaration it used to carry cannot hold.
The guide said the scaffold views expect <propertyName>InstanceList and <propertyName>Instance. The templates bind <propertyName>List and <propertyName>Count, and <propertyName> for a single instance.
|
Thanks. All addressed in Model binding: converts now, but refuses a conversion that changes the value. I took the coercion, with one guard. Docs: one paragraph under Declaring the Model in Tests:
Nits: the description now says three closures fail, with the listener sort kept for consistency. The Verified: |
Review Findings, round 2Head What I ran on the head, all green:
One new finding, in the value-change guard the PR adds on top of the coercion, and one small one next to it. [P2] The value-change guard rejects a
|
Compare floating-point values at their exact binary precision and decimal/integer values without rounded decimal strings. Keep lossy narrowing and large integer precision loss rejected. Wrap conversion failures with the model field diagnostic, including non-finite decimal conversions. Add regression cases, clarify the guide, and mirror first-input-wins duplicate handling when staging test views.
|
Addressed in fc1941a.
Added regression cases and clarified the guide. All 227 GSP core tests and the module's style checks passed; a staging check with a duplicate |
🚨 All tests passed but jobs failed 🚨Failed Jobs without Test Failures❌ CI / Build Grails Forge (Java 21, indy=false) 🏷️ Commit: fc1941a Learn more about TestLens at testlens.app/docs. |
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:The template now declares the count as
Number. The generated service supplies aLong, butstatic scaffold = Xgoes throughRestfulController.countResources(), which supplies anInteger, so no single concrete type fits both.While tracking this down, the binding itself turned out to be stricter than the rest of the page.
<g:set type="int" .../>converts, and so does a plain Groovy assignment, but a model field declaredIntegerfailed outright when given aLong, with a bare JDK reflection message naming the generated class. Model values are now converted to the declared type the way a Groovy assignment converts them, so existing views that declareInteger bookCountkeep working.DefaultTypeTransformation.castToTypeon its own wraps3_000_000_000Linto-1294967296and turns42.9into42, silently. So a conversion that would change a numeric value is refused, as is a value that cannot be converted at all, and both name the field and the two 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 } }Three
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 isObjectmimeTypesapplicationContext.getBean('mimeTypes')returnsObjectFixed by typing the closure parameters and converting the values whose static type is
Object. TheappListenerssort is typed the same way for consistency; its map values are allString, so it compiled without that change. 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.Keeping both from coming back
CI was green on
8.0.xwith both defects present.gsp-compile-staticnow compiles the web profile's welcome page statically alongside its own pages (the forge copy is pinned to the profile copy byGrailsGspSpec), andScaffoldedIndexViewModelSpecexpands the scaffoldedindex.gspand renders its model declaration with anInteger, aLong, and a count pastInteger.MAX_VALUE.