matrei commented on PR #16322:
URL: https://github.com/apache/grails-core/pull/16322#issuecomment-5677678800
## Review Findings, round 2
Head `22163f7f11`, base `8.0.x` at `3067d0a855`. The branch now contains the
base head, so the merge-base is the base itself and there is nothing to merge.
All three P2s from the first round are addressed: model values are converted
with `DefaultTypeTransformation.castToType`, the static compilation guide says
what a mismatched value does, the welcome page is compiled statically by
`gsp-compile-static`, and the scaffolded `index.gsp` count declaration has a
spec. The scaffolding guide nit is fixed too. The listener sort hunk is
unchanged, which is fine.
What I ran on the head, all green:
- `:grails-gsp-core:test --tests GspCompileStaticSpec`: 40 tests, 0 failures.
- `:grails-scaffolding:test --tests ScaffoldedIndexViewModelSpec`: 3 tests,
0 failures.
- `:grails-test-examples-gsp-compile-static:integrationTest`: 7 tests, 0
failures. The staged directory holds the app's five pages plus the welcome
page, `views.properties` lists `/WEB-INF/grails-app/views/index.gsp`, and the
compiled class extends `CompileStaticGroovyPage`.
- `:grails-gsp-core:codeStyle` and `:grails-scaffolding:codeStyle`: no
violations. The modules have no PMD or SpotBugs tasks.
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 `Float` widened to a `Double` field
References:
- `grails-gsp/core/src/main/groovy/org/grails/gsp/GroovyPage.java:198` (the
guard)
- `grails-gsp/core/src/main/groovy/org/grails/gsp/GroovyPage.java:211-219`
(`sameNumericValue`)
- `grails-doc/src/en/guide/theWebLayer/gsp/gspStaticCompilation.adoc:56`
`sameNumericValue` compares the two numbers through `new
BigDecimal(n.toString())`. `Float.toString` prints the shortest decimal that
rounds back to the float, `Double.toString` prints the shortest decimal that
rounds back to the double, and those differ for the same binary value: `0.1f`
prints `0.1`, and the same value as a `double` prints `0.10000000149011612`.
The widening itself is exact, every `float` is representable as a `double`, but
the decimal forms are not equal, so the guard throws.
I rendered `@{ model="Double v"}${v}` with `[v: 0.1f]` through
`GroovyPagesTemplateEngine` on the head:
```
GroovyPagesException: Model field 'v' is declared as java.lang.Double, which
cannot hold the java.lang.Float 0.1 the model supplied without changing it.
```
Same for `double v` with `1.1f`. Plain Groovy assigns `double d = 0.1f`
without complaint, so the guide's sentence at line 56, "converted to the
declared type the way a Groovy assignment converts it", is not true for the one
conversion that loses nothing. The same run showed the reverse direction,
`Float v` with `0.1d`, rendering `0.1`: the narrowing that does drop bits
passes because both sides print `0.1`. The guard is backwards for the float
family.
A `Double` model field fed a `Float` is not exotic: a domain property
declared `Float` rendered through a page whose author wrote `Double`, or
`BigDecimal` for that matter, which works, so the `Double` failure will read as
arbitrary.
The rule the doc states, "a conversion that would change the value fails",
holds up if the comparison is done at the precision of the value that came in
rather than through decimal strings. One shape that gives that: convert
`converted` back to the original's class and compare it to the original, using
`compareTo` when the original is a `BigDecimal` so that scale does not matter.
```java
Number back = (Number) DefaultTypeTransformation.castToType(converted,
original.getClass());
return original instanceof BigDecimal
? ((BigDecimal) original).compareTo((BigDecimal) back) == 0
: original.equals(back);
```
Against the cases in the spec plus the ones above: `0.1f` to `Double` comes
back as `0.1f` and passes; `0.1d` to `Float` comes back as
`0.10000000149011612d` and fails, which is the lossy one; `3_000_000_000L` to
`Integer` comes back as `-1294967296L` and fails; `42.0G` to `Integer` comes
back as `42` and compares equal; `42.9G` fails; `NaN` to `Integer` comes back
as `0.0` and fails; `Long.MAX_VALUE` to `Double` comes back as `Long.MAX_VALUE`
because `Double.longValue` saturates, so it passes the way Java's own `long` to
`double` widening does. The one thing to guard is a `Number` subclass Groovy
cannot cast back to, `AtomicLong` say, where `castToType` throws; falling back
to the current comparison for that is enough. Whichever rule is chosen, the
spec's "converted the way a Groovy assignment converts it" table at
`GspCompileStaticSpec.groovy:264` should carry a `'Double' | 0.1f` row, since
that is the row that fails today.
For the record, two other rejections from the same probe are consistent with
the doc sentence and I do not count them as defects: `Long.MAX_VALUE` into a
`Double` field and `16_777_217` into a `Float` field both throw, and both
really do lose precision.
### [P3] `NaN` into a `BigDecimal` or `BigInteger` field escapes as a raw
`NumberFormatException`
References:
- `grails-gsp/core/src/main/groovy/org/grails/gsp/GroovyPage.java:192-196`
`castToType(Double.NaN, BigDecimal.class)` throws `NumberFormatException`,
not `GroovyCastException`, so it passes the `catch` at line 193 and surfaces
without the field name:
```
NumberFormatException: Character N is neither a decimal digit number,
decimal point, nor "e" notation exponential mark.
```
Every other failure in this method is a `GroovyPagesException` naming the
field. Catching `GroovyCastException | NumberFormatException` (or
`RuntimeException`, since `castToType` has no checked failure mode) at line 193
keeps that promise for the infinities and `NaN` too. The `Double.compare`
fallback at line 217 already anticipates these values for the guard; this is
the same case one step earlier.
### Nit: mirror the plugin's duplicate handling in the staging task
References:
- `grails-test-examples/gsp-compile-static/build.gradle:56-60`
The Gradle plugin's own view staging (`GroovyPagePlugin.stageGroovyPages`)
sets `duplicatesStrategy = DuplicatesStrategy.EXCLUDE` with the application's
views listed first, so the app's page wins. The PR's `Sync` lists the app's
views first too but leaves the strategy at the default, which in Gradle 9 fails
the task on a duplicate path. Nothing collides today, because the app has no
root `index.gsp`; the day someone adds one, the message will be about `Sync`
rather than about the page. One line keeps it consistent with the plugin, and
the comment above the task already explains why the welcome page is there.
### Confirmed
- Setting `source` on `compileGroovyPages` to a `Directory` also sets
`srcDir` (`GroovyPageForkCompileTask.setSource`), so the staged pages keep
their relative names: the welcome page is registered as
`/WEB-INF/grails-app/views/index.gsp`, not under `build/generated`. The plugin
sets `source` inside the task's registration action, so the build script's
`tasks.named` block runs after it and is not overwritten.
- The example app maps `/` to `demo/index`, so the precompiled welcome page
sits in `views.properties` unused at runtime. Compiling it is the whole point,
and it changes nothing the integration tests request.
- `ScaffoldedIndexViewModelSpec` expands the template with
`GStringTemplateEngine` the way `ScaffoldingViewResolver` does, and a model
directive sets `compileStaticModeSetting` in `GroovyPageParser`, so the
declaration really becomes a field. The `3_000_000_000L` row is the one that
would fail if the template went back to `Integer`; the `3` and `3L` rows pass
under either declaration now that values are coerced, which the `where:` label
says.
- `GroovyPagesTemplateEngine.createTemplate(String, String)` caches by name,
and the spec names the `3` and `3L` iterations both `scaffoldedIndex3`, but
each iteration builds a fresh engine and the source is identical, so nothing is
reused across rows.
- `Boolean showAll` fed the `String` `"false"` now renders `true`. That is
Groovy truth, `Boolean b = "false"` gives the same, and it matches the doc
sentence. Before the PR it threw an `IllegalArgumentException` from
`Field.set`. Noting it so nobody reads it as a regression later.
- The `42.0G` to `Integer` row passes through the guard because
`sameNumericValue` compares `BigDecimal` values with `compareTo`, so scale is
ignored. Good.
- `List` declared, `Set` supplied throws a `GroovyCastException`, same as a
plain Groovy assignment, and is reported through the new message with both
types.
- Import order in the spec (`org.codehaus.groovy` between
`org.grails.core.gsp` and `org.grails.taglib`) passes CodeNarc.
- The stale `book/index.gsp` classes from my round-one hand test were still
in the example app's `build/gsp-classes` and were listed in the regenerated
`views.properties`; I removed them. The compile task does not clear stale
output, but that is pre-existing and not touched here.
## Verification
- `./gradlew :grails-gsp-core:test --tests
org.grails.gsp.GspCompileStaticSpec :grails-scaffolding:test --tests
grails.plugin.scaffolding.ScaffoldedIndexViewModelSpec
:grails-test-examples-gsp-compile-static:integrationTest
:grails-gsp-core:checkstyleMain --continue`: BUILD SUCCESSFUL, 40 + 3 + 7
tests, 0 failures.
- `./gradlew :grails-gsp-core:codeStyle :grails-scaffolding:codeStyle`:
BUILD SUCCESSFUL.
- `DefaultTypeTransformation.castToType` probed directly with Groovy 5.1.2
for 23 declared/supplied pairs, and 10 of them rendered through
`GroovyPagesTemplateEngine` on the head via a throwaway spec (deleted
afterwards). The `Double`/`Float`, `NaN`/`BigDecimal` and `Boolean`/`"false"`
results above come from that run.
- `git status` shows no tracked changes.
--
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]
