Add method-based TagLib syntax with legacy compatibility, benchmarks, and docs#15465
Add method-based TagLib syntax with legacy compatibility, benchmarks, and docs#15465davydotcom wants to merge 43 commits into
Conversation
…pdate - implement method-defined tag handler support and invocation context - preserve closure-style behavior across property/direct and namespaced paths - convert built-in web/GSP taglibs to method syntax - add compile-time warning for closure-defined tag fields - add coverage and benchmark for method vs closure invocation - update guides and demo taglib samples to method syntax Co-Authored-By: Oz <oz-agent@warp.dev>
Co-Authored-By: Oz <oz-agent@warp.dev>
Co-Authored-By: Oz <oz-agent@warp.dev>
- treat only Map parameter named attrs as full tag attributes map - allow other Map-typed parameters to bind from attribute key by parameter name - add regression tests for map-valued attribute binding and reserved attrs behavior Co-Authored-By: Oz <oz-agent@warp.dev>
- use private implementation helpers to avoid recursive dispatch in typed overloads - keep Map-based handlers for validation-safe fallback behavior - add regression test ensuring private/protected methods are not exposed as tag methods - document overload pattern for typed signatures with existing validation paths Co-Authored-By: Oz <oz-agent@warp.dev>
…gnment - keep typed overloads delegating to private implementation helpers - remove unnecessary attrs.name writes since typed args are sourced from attrs - preserve behavior validated by focused FormTagLib and method-tag test suites Co-Authored-By: Oz <oz-agent@warp.dev>
- add thread-safe ClassValue cache for invokable public tag methods by name - remove per-invocation getMethods scans in hasInvokableTagMethod/invokeTagMethod - optimize TagLibrary.propertyMissing by caching method fallback closures in non-dev mode - use resolved namespace for default-namespace fallback closures Co-Authored-By: Oz <oz-agent@warp.dev>
- restore attrs-reserved binding for paginate - route namespaced method tag calls via tag output capture - add fieldValue(Map) compatibility overload - harden form fields rendering/raw handling with method dispatch Co-Authored-By: Oz <oz-agent@warp.dev>
Doc ExamplesFiles: class SimpleTagLib {
static namespace = "my"
def example() { // ← new method added
def example = { attrs -> // ← old closure NOT removed
//...
}Same issue in
|
…egistered as tag methods Make helper methods private across all affected TagLib files to prevent TagMethodInvoker.isTagMethodCandidate() from matching them as tag methods. Remove convenience overloads (e.g. textField(String,Map)) entirely where Groovy 4's multimethod restriction forbids mixing private/public methods of the same name. Changes: - ApplicationTagLib: make renderResourceLink, doCreateLink private - FormatTagLib: make messageHelper private - UrlMappingTagLib: make appendClass private - ValidationTagLib: remove fieldValue(Map) overload, make formatValue private, remove formatValue from returnObjectForTags - FormTagLib: remove 5 typed convenience overloads, make renderNoSelectionOption private - FormFieldsTagLib: make 9 protected helper methods private - TagMethodInvoker: sort methods by descending param count to prefer (Map,Closure) over (Map) signatures - Checkstyle/CodeNarc fixes: alphabetical imports, blank lines before constructors, single-quoted strings
# Conflicts: # grails-fields/grails-app/taglib/grails/plugin/formfields/FormFieldsTagLib.groovy # grails-gsp/plugin/src/main/groovy/org/grails/plugins/web/taglib/ApplicationTagLib.groovy # grails-gsp/plugin/src/main/groovy/org/grails/plugins/web/taglib/FormTagLib.groovy
|
@davydotcom I made some attempts at fixing the test pollution, but I think there is still more work required here. |
|
From my AI Results: |
|
Concerning the continued failures: Original CI failures (5 tests at maxTestParallel=3, 1 test at maxTestParallel=4) are reduced to occasional flakes on ReverseUrlMappingToDefaultActionTests.testLinkTagRendering. That remaining flake is a |
|
Update on this, from the AI: So the tag library state isn't being cleaned up correctly. |
# Conflicts: # DEVELOPMENT.md # grails-gsp/plugin/src/main/groovy/org/grails/plugins/web/taglib/ApplicationTagLib.groovy # grails-gsp/plugin/src/main/groovy/org/grails/plugins/web/taglib/FormTagLib.groovy
|
I decided to move the taglib cleanup to be by feature instead of by spec. The biggest reason was the flakiness of the grails-gsp test run was really due to partial clean-ups. By cleaning up after each feature, each test is isolated. |
# Conflicts: # build-logic/docs-core/build.gradle # build.gradle # grails-forge/build.gradle # grails-gradle/build.gradle
codeconsole
left a comment
There was a problem hiding this comment.
Nice work!
Considerations?:
PR #15465 Review — Method-based TagLib Handlers
Substantial, well-thought refactor (+1401/-244, ~60 files). The ClassValue cache, push/pop discipline in
GroovyPage.invokeTagLibMethod, and the UrlMappingsUnitTest.mockArtefact fix are good. Highest-impact
issues below.
Blockers
B1 — TagMethodInvoker.invokeTagMethod swallows Error and rewraps it as RuntimeException. The
catch only handles RuntimeException; an Error (StackOverflowError, AssertionError) gets boxed, hiding
the true cause. Add if (target instanceof Error err) throw err; and prefer GrailsTagException
(consistent with the closure path) for non-runtime targets.
B2 — Closure-tag dispatch regresses. TagMethodInvoker.getClosureTagProperty walks the superclass
chain via getDeclaredField on every closure tag invocation, replacing what was previously a single
metaclass getProperty(tagName). Tag-heavy GSPs likely take a hit on the closure path that the benchmark
doesn't measure. Cache resolved fields in a ClassValue<Map<String, Optional<Field>>> mirroring
INVOKABLE_METHODS_BY_NAME.
B3 — TagLibArtefactTypeAstTransformation deprecation warning is unconditional. Third-party plugins
that ship closure-defined tag fields will spam warnings during user compilation, with no opt-out. Verify
whether GrailsASTUtils.warning interacts with Groovy's Werror flag — if so this can break user builds.
Gate behind a system property (grails.taglib.warnDeprecatedClosures) and/or skip when the source is from
a plugin jar, not the user's app.
Major
M1 — ValidationTagLib.formatValue silently dropped from returnObjectForTags and made private.
<g:formatValue> was a public tag; this is a breaking behavior change not called out in
upgrading80x.adoc. Either restore it as a public method tag, or document the removal.
M2 — toMethodArguments returns null (rejecting the overload) when any non-attrs/non-Closure arg
is null. Closures pass null through; typed-parameter tags now silently fall through to another
overload or throw MissingMethodException. Only reject null when the parameter is primitive; allow it
for reference types.
M3 — Method resolution depends on JVM-defined getDeclaredMethods() order for same-arity overloads.
Add a deterministic tiebreaker (e.g., parameter-type names) so behavior is identical on HotSpot vs.
Graal/J9.
M4 — is*() no-arg exclusion is heuristic-fragile. Targeted at JavaBean accessors, but a tag
legitimately written boolean isAvailable(...) works only because it has args. Consider a @Tag
annotation override for explicit opt-in.
Minor / DRY
- N1 —
FormTagLibprivate*Implhelpers (textFieldImpl,passwordFieldImpl,submitButtonImpl,
etc.) are ~30 lines of mechanical boilerplate caused by the Groovy 4 multimethod restriction. A single
private void renderField(Map attrs, String type)setting bothattrs.typeandattrs.tagNamecollapses
them. - N2 —
UrlMappingTagLib.paginate:attrs = (TypeConvertingMap) attrsre-casting the parameter to
itself is non-obvious; introduce a typed local. - N3 —
JavascriptTagLib.javascript/escapeJavascriptnot converted while everything around them is —
introduce a typed local. - N3 —
JavascriptTagLib.javascript/escapeJavascriptnot converted while everything around them is — either convert or
add a one-line comment about why. - N4 — Doc examples in
simpleTags.adocandMethodDefinedTagLibSpecreference
propertyMissing('attrs')/propertyMissing('body'). CLAUDE.md says don't expose internal mechanisms in docs — add a public
currentAttrs()/currentBody()helper on theTagLibrarytrait and document that. - N5 —
TagMethodContext.ThreadLocalis onlyremove()d when the stack empties onpop. If any dispatch path ever skips
pop, a later request on the same container thread sees staleattrs/body. Add a request-lifecycle interceptor that calls
clearAll()defensively. - N6 —
FRAMEWORK_METHOD_NAMESandNON_TAG_METHOD_NAMESoverlap conceptually; consolidate to avoid drift. - N7 — Test deletions (
RestfulReverseUrlRenderingTests-22, etc.) are not coverage losses — they're redundant after
UrlMappingCleanupInterceptorand the newmockArtefactcleanup. Good DRY.
Positives
ClassValueis the right primitive for class-keyed caches (auto-invalidates on unload).try/finallypush/pop inGroovyPage.invokeTagLibMethodis correct.- Benchmark spec is honest — prints, doesn't assertion-gate the percentage.
mockArtefactdestroys the cached singleton before re-registering — fixes a real test-isolation hazard.
Recommendation
Fix B1, B2, B3, M1, M2 before merge; the rest are post-merge improvements. The formatValue removal (M1) is the most likely to
bite real users silently
Resolve blockers and major comments from review of PR #15465: - B1: rethrow Error from TagMethodInvoker.invokeTagMethod instead of wrapping in RuntimeException; matches closure-path semantics so StackOverflowError/AssertionError surface unchanged. - B2: cache resolved Closure tag fields per TagLib class via ClassValue. Eliminates per-invocation getDeclaredField walk and exception-driven control flow on the closure dispatch hot path. - M1: restore ValidationTagLib.formatValue as a public method tag and back into returnObjectForTags. Now safe because overload resolution no longer relies on the magic single-attribute fallback. - M2 + jf#6: replace the magic single-param attribute fallback in toMethodArguments with strict containsKey-based binding. Absent attribute rejects the overload; null is now a legal binding for reference-typed parameters and only rejected for primitives. - M3: deterministic same-arity tiebreaker (signature compareTo) so overload resolution is stable across HotSpot/Graal/J9. - jf#1: fix broken doc examples in namespaces.adoc and tagReturnValue.adoc that mixed method and closure syntax in the same snippet. - jf#2: introduce @grails.gsp.NotATag annotation as an explicit method opt-out, plus signature-based filtering for overrides of Object and GroovyObject methods (toString/hashCode/equals declared on the user class). Matches the controllers-style "public methods are tags; helpers are private" convention without forcing annotations. - B3: gate the closure deprecation AST warning behind the system property grails.taglib.warnDeprecatedClosures (default true). Set false to silence at compile time. - Document method-based TagLib handlers, @NotATag, the suppression property, and the tag method registration behavior change in upgrading70x.adoc (new section 16). New specs: - org.grails.taglib.TagMethodInvokerSpec - covers Error/RuntimeException propagation, Object/GroovyObject override exclusion, Spring lifecycle exclusion, @NotATag, closure-field cache including subclass shadowing, null/missing attribute binding semantics, and same-arity overload ordering. - grails.gsp.taglib.compiler.TagLibArtefactTypeAstTransformationSpec - covers default warning emission and suppression via system property. - ValidationTagLibSpec - new specs for <g:formatValue> tag-syntax and function-syntax invocation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed the review feedback in 7c89b78. Status of every concern raised: Resolved
Documented (not reverted)jf#4 — Verified invalid
Test coverageNew focused specs:
Open follow-ups (post-merge polish)The N1–N7 cosmetic items from @codeconsole's review are intentionally deferred — happy to file follow-up issues if useful. Most impactful would be N4 (replace |
|
Following up on my earlier review with empirical reproducers for the issues that are introduced by this PR (i.e. behaviors that did not exist on A single Spock spec exercises each issue against the public Reproducer: Run with: C1 —
|
| ID | Severity | Reproducer | Status |
|---|---|---|---|
| C1 | Blocking for users | C1: …silently fails when parameter names are not preserved |
Confirmed |
| H1 | High | H1, H1b |
Confirmed |
| H2 | Medium-High | H2: …overrideMethods is now 'true' |
Confirmed via diff + signature assertion |
| M1 | Medium | M1, M1b |
Confirmed |
C1 is the one I'd really like to see addressed before merge — the headline feature silently does nothing in a fresh user app, and the diagnostic gives no clue why. H1 is the design polarity question: opt-out vs opt-in for tag-method discovery.
The reproducer is on a branch on my fork — feel free to pull it down or leave it where it is.
|
I updated the reproducer branch with a stronger H2 runtime check:
|
typed method tag arguments bind reliably in user applications. Narrow method tag discovery to avoid exposing public helper methods as tags by default. Conventional attrs/body signatures remain supported, while zero-arg and typed-parameter method tags require @tag. Preserve @NotATag opt-out behavior and add debug logging when tag dispatchers override existing methods. Expand unit, Gradle plugin, docs, and grails-test-examples coverage for the reviewed reproducer cases.
|
@jamesfredley I think i've addressed all of the issues you raised. |
|
This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments. |
91be40c to
ddeb082
Compare
🚨 TestLens detected 2 failed tests 🚨Here is what you can do:
Test Summary
🏷️ Commit: 11cf62f Test FailuresGroovyChangeLogSpec > outputs a warning message by calling the warn method (:grails-data-hibernate5-dbmigration:test in CI - Groovy Joint Validation Build / build_grails)GroovyChangeLogSpec > updates a database with Groovy Change (:grails-data-hibernate5-dbmigration:test in CI - Groovy Joint Validation Build / build_grails)Muted TestsSelect tests to mute in this pull request:
Reuse successful test results:
Click the checkbox to trigger a rerun:
Learn more about TestLens at testlens.app. |

Summary
This PR introduces method-based TagLib handlers as the recommended syntax, while preserving full backward compatibility with closure-based handlers and legacy invocation paths.
Rebased onto
8.0.xfrom the original PR #15459.What's included
attrsandbody) for method handlersdef greeting(String name)binds fromname="...")Performance
The method-vs-closure benchmark added in this change set shows an approximately 7–10% improvement for method-based invocation in the covered scenarios.
TagLib syntax examples
Recommended (method-based)
Usage:
Legacy-compatible (closure field)
Validation performed
FormTagLib2TestsFormTagLib3TestsSelectTagTestsNamespacedTagLibMethodTestsTagLibMethodMissingSpecMethodDefinedTagLibSpec:grails-gsp:test:grails-test-examples-app1:integrationTest --tests functionaltests.MiscFunctionalSpecCo-Authored-By: Oz oz-agent@warp.dev