codeconsole commented on PR #16142:
URL: https://github.com/apache/grails-core/pull/16142#issuecomment-5286259682
Thanks — the `-1` catch in particular was a real bug, not a nit.
**Retargeted to `7.1.x`** rather than argue the patch-branch point. You're
right that it adds public API, and that rules out a patch release regardless of
whether anything actually breaks. Rebased onto `7.1.x`; @maczikasz's commit is
still at the base.
That move turned out to pay for itself twice: `7.1.x` has an upgrade guide
covering 7.0 → 7.1, so the `getLastModified()` change now has somewhere to be
announced. `7.0.x` had no within-7.0 guide, which is why I'd fallen back to
"release note" earlier. Your cross-version note is recorded in the same section.
Happy to move it back to `7.0.x` if you and @matrei would rather have it
there — the unreproducible jars are shipping from `7.0.x` today, and
`verify-reproducible.sh` has no GSP exclusion, so release verification
currently diffs on every artifact carrying precompiled pages. Your call, not
mine.
### Fixed
- **`-1` sentinel** (`45e2452cb9`) — confirmed the path:
`establishLastModified` returns `-1` for a null resource,
`FileNotFoundException` and `IOException`, and `applyLastModifiedFromResource`
stores it verbatim. Guard is now `lastModified != 0`, so `0` keeps its "nothing
recorded" meaning and `-1` keeps its self-healing one. Added a feature for it
and verified it's a real guard — reverting to `> 0` fails that one alone and
leaves the rest passing.
- **`checksumOf(byte[])`** (`e4055c2cb7`) — you were right that the stream
signature bought nothing. Drops the read loop, the `IOException`, and the
compiler's checksum-specific wrapper. `establishChecksum` uses
`getContentAsByteArray()`. One wrapper remains only because
`GroovyPageParser`'s constructor takes an `InputStream`; keeping it avoids
reading the file twice. The raw-bytes invariant is now in the javadoc.
- **`LAST_MODIFIED` as permanent ABI** — comment added at the unguarded read
explaining why it must keep being emitted even though the value is always `0`.
- **`getLastModified()` javadoc** — agreed this covers external callers
better than a release note, since it's what they'd actually be reading.
- **Doc version bound**, **both `setLastModified()` assert sites**, **the
`URLClassLoader` leak**, and **all three asserts in the reload spec** — applied
as suggested. The vacuous-pass one was the worst of them: the spec would have
gone green while the regression it exists to catch shipped.
### Open
- **The `(mtime, length)` pre-check.** Agreed on the cost, and the
deployed-reload scenario is exactly what this targets. Two notes if you want it
here rather than as a follow-up: the memo has to stay coherent with the
`CacheEntry` that already gates checks at `grails.gsp.reload.interval`, and it
reintroduces mtime as a fast-path skip rather than as the source of truth —
worth a comment so nobody later mistakes it for the old mechanism. Say the word
and I'll add it with a spec for the "both unchanged" skip.
- **Checksum on the runtime-compile path.** Agreed, and for the reason you
give — dev-mode edits inside the granularity window are exactly what it's good
at, and it's the precondition for retiring the timestamp branch. Filing as a
follow-up unless you'd rather have it here.
One narrowing on the cross-version case, for the record: for GSPs inside
binary plugin jars `resolveViewInBinaryPlugin` nulls the resource callable, so
those never reach the comparison. The reachable case is an application with no
`gsp/views.properties` of its own, where
`getResource('classpath:gsp/views.properties')` picks up a plugin's and those
pages get a live callable.
--
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]
