matrei commented on PR #16322:
URL: https://github.com/apache/grails-core/pull/16322#issuecomment-5615272342
## AI Review Findings
Head `8855b88340`, base `8.0.x` at `0980623481`. The head is 103 commits
behind the base but merges clean (`git merge-tree` reports no conflicts). Both
defects in the description are real and both fixes work:
- Compiling the base-branch welcome page statically (copied into
`grails-test-examples/gsp-compile-static`) fails with 4 type-checking errors at
three sites, the plugins sort, the domains-by-plugin sort and the mime-types
sort. The PR's page compiles, and the compiled class extends
`CompileStaticGroovyPage`, so it really was compiled statically rather than
falling back.
- The scaffolded `index.gsp` expanded for a domain class compiles statically
with `Number` and the `${count} > params.int('max')` comparison type-checks.
- `GspCompileStaticSpec`: 30 tests pass. Checkstyle on `grails-gsp-core`
main: 0 violations.
The findings below are about the choice the `GroovyPage` change makes, and
about what would keep either bug from coming back. None of them changes the
welcome page hunks.
### [P2] Assign model fields the way Groovy assigns, instead of declaring
that model values are not coerced
References:
- `grails-gsp/core/src/main/groovy/org/grails/gsp/GroovyPage.java:180-197`
(`applyModelFieldsFromBinding`)
- `grails-scaffolding/src/main/templates/scaffolding/index.gsp:1`
The new message states a rule, "model values are not coerced", that is the
opposite of what the rest of the page does. `<g:set type="int" var="total"
value="${books.size()}"/>` converts (the static compilation guide says so
explicitly), and plain Groovy converts too:
```groovy
Integer i = 42L // 42, java.lang.Integer
int p = 42L // 42
String s = "${40+2}" // GString assigned to String
```
A model field declared `Integer bookCount` is the one place in a page where
the same assignment throws. That declaration is also the natural one to write:
GORM's `count()` returns `Integer` (`GormEntity.groovy:732`), the generated
service returns `Long`, and `RestfulController.countResources()` returns
`Integer`, so any page author who declares the count with the type of whichever
source they looked at has a coin-flip chance of a render failure. `String`
fields fed a GString from a controller fail the same way today.
`DefaultTypeTransformation.castToType` is Groovy's own assignment
conversion, so it gives exactly the semantics a `def`-free Groovy assignment
would:
```java
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:
| declared | supplied | result |
|---|---|---|
| `Integer` | `42L` | renders `42` |
| `int` | `42L` | renders `42` |
| `Long` | `42` | renders `42` |
| `long` | `42 as Short` | renders `42` |
| `String` | `"${40 + 2}"` (GString) | renders `42` |
| `Number` | `42 as BigInteger` | renders `42` |
| `Integer` | `new Date()` | `GroovyPagesException` naming the field, cause
`GroovyCastException` |
The only existing test that changes is *a model value of the wrong type
names the field and both types*, which would flip from `Integer`/`Long` to a
pair that genuinely cannot be converted, `Integer`/`Date` say. Everything else
in the spec passes unchanged. The prototype is not in the working tree.
The `Number` change in the scaffold template is still the honest type for a
value that is `Long` from one controller and `Integer` from the other, so keep
it either way. With coercion it stops being load-bearing, and every
already-generated `Integer` view in existing 8.0.x applications starts working
as well, which the template change alone does not give them.
### [P2] Document what a declared model field does with a value of another
type
References:
- `grails-doc/src/en/guide/theWebLayer/gsp/gspStaticCompilation.adoc:28-56`
(Declaring the Model)
- `grails-doc/src/en/guide/theWebLayer/gsp/gspStaticCompilation.adoc:208`
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 `GroovyCastException`.
Whichever way the finding above is decided, that section needs one sentence:
either "a model value is converted the way a Groovy assignment converts it, so
a `Long` count satisfies an `Integer` field" or "a model value must be
assignable to the declared type; a mismatch fails at render naming the field".
The behaviour is user-facing and is exactly what this PR changes, so this is
the doc coverage the contributing rules ask for.
### [P2] Neither failure had a test that could catch it, and this PR adds
coverage for the engine but not for the two pages
References:
- `grails-test-examples/gsp-compile-static/build.gradle`
-
`grails-test-examples/scaffolding-fields/grails-app/controllers/scaffoldingfields/EmployeeController.groovy:32`
-
`grails-gsp/core/src/test/groovy/org/grails/gsp/GspCompileStaticSpec.groovy:241-274`
CI was green on `8.0.x` with both bugs present, and stays green if either
regresses:
- Nothing compiles the welcome page statically. `gsp-compile-static` has its
own small pages only. A `Copy` step that drops
`grails-profiles/web/skeleton/grails-app/views/index.gsp` into that app's views
ahead of `compileGroovyPages` is the check I ran by hand above, and it fails on
the base branch with the four errors. The forge copy is already pinned to the
profile copy by *test the profile skeleton mirrors the forge welcome templates*
in `GrailsGspSpec`, so one copy is enough.
- Nothing renders a service-backed scaffolded index. `scaffolding-fields`
and `hyphenated` use `static scaffold = Domain`, which goes through
`RestfulServiceController.countResources()` and supplies an `Integer`, so the
generated-service `Long` path is never rendered. The new spec cases prove the
engine accepts a `Long` for a `Number` field; they do not prove the template
declares `Number`. A test that expands `scaffolding/index.gsp` for a domain
class and renders it with `[bookList: [], bookCount: 3L]` would pin the
template itself.
### Nit: the listener sort hunk is not needed
References:
- `grails-profiles/web/skeleton/grails-app/views/index.gsp:570`
- `grails-forge/grails-forge-core/src/main/resources/gsp/index.gsp:570`
`Map<String, String> a, Map<String, String> b` on the listener sort compiles
either way: the `collect` above it builds maps whose values are all `String`,
so `a.name` already infers as `String`. I reverted that one hunk on the PR's
page and it compiled. The description says four closures fail; three do.
Harmless, and fine to keep for symmetry with the other three.
### Nit: the scaffolding guide describes model names that the templates no
longer use
References:
- `grails-doc/src/en/guide/scaffolding.adoc:191`
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
`<propertyName>InstanceList` and `<propertyName>Instance`, while the templates
bind `<propertyName>List` and `<propertyName>Count`.
### Confirmed
- The `grails.plugins.GrailsPlugin` fully-qualified cast is needed:
`GroovyPageParser.DEFAULT_IMPORTS` imports
`grails.plugins.metadata.GrailsPlugin` into every page, so a page import of the
plugin interface would clash.
- The rewritten sorts keep their ordering. The one-argument `sort` on the
plugin rows and on the `domainsByPlugin` entries orders by the same lower-cased
key the comparators used, and `List.sort(Closure)` still sorts in place as
before.
- The `groupBy` closure now returns a typed `String`, which is what lets
`it.key.toLowerCase()` type-check on the entry.
- Reading the model value before the `try` changes nothing: only
`IllegalAccessException` was caught before, so a failing `getProperty`
propagated then and propagates now.
## Verification
- `./gradlew :grails-gsp-core:test --tests
org.grails.gsp.GspCompileStaticSpec`: 30 tests, 0 failures on the PR head.
- `./gradlew :grails-gsp-core:checkstyleMain`: 0 violations.
- `./gradlew :grails-test-examples-gsp-compile-static:compileGroovyPages`
with the PR's welcome page and an expanded scaffold `index.gsp`
(`List<gspstatic.Book> bookList; Number bookCount`) copied into the app:
success, both classes extend `CompileStaticGroovyPage`.
- Same task with the merge-base welcome page: fails, `Cannot find matching
method java.lang.Object#toLowerCase()` at generated lines 42, 413 and 1340 plus
`No such property: name for class: java.lang.Object` at 1340.
- Same task with the PR's page and only the listener sort hunk reverted:
success.
- Coercion prototype in `GroovyPage.java` plus 7 temporary spec cases: 37
tests, the single failure being the PR's `Integer`/`Long` mismatch case, as
expected. Both files restored afterwards; `git status` shows no tracked changes.
- The copied pages were removed from
`grails-test-examples/gsp-compile-static` afterwards.
--
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]
