Skip to content

Add method-based TagLib syntax with legacy compatibility, benchmarks, and docs#15465

Merged
jamesfredley merged 49 commits into
8.0.xfrom
feature/taglib-method-actions
May 30, 2026
Merged

Add method-based TagLib syntax with legacy compatibility, benchmarks, and docs#15465
jamesfredley merged 49 commits into
8.0.xfrom
feature/taglib-method-actions

Conversation

@davydotcom
Copy link
Copy Markdown
Contributor

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.x from the original PR #15459.

What's included

  • Add method-defined TagLib support in core dispatch/invocation paths
  • Preserve implicit method context (attrs and body) for method handlers
  • Support binding named attributes directly to method signature arguments (for example def greeting(String name) binds from name="...")
  • Add compatibility shims so legacy property/direct invocations continue to work for method-defined tags
  • Preserve namespaced dispatch behavior and collision handling with method-backed tags
  • Add compile-time warning for closure-defined tag fields in user TagLibs
  • Convert shipped Grails web/GSP taglibs to method-based handlers
  • Add coverage for method-defined tags and legacy compatibility behavior
  • Add benchmark spec for method vs closure invocation
  • Update guides/docs to present method syntax first, with closure syntax as legacy-compatible

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)

class DemoTagLib {
    static namespace = 'demo'

    def greet() {
        out << "Hello, ${attrs.name}!"
    }

    def greeting(String name) {
        out << "Hello, ${name}!"
    }

    def repeat() {
        attrs.times?.toInteger()?.times { n ->
            out << body(n)
        }
    }
}

Usage:

<demo:greet name="Ada" />
<demo:greeting name="Ada" />

Legacy-compatible (closure field)

class DemoTagLib {
    static namespace = 'demo'

    def greet = { attrs, body ->
        out << "Hello, ${attrs.name}!"
    }
}

Validation performed

  • Focused regressions:
    • FormTagLib2Tests
    • FormTagLib3Tests
    • SelectTagTests
    • NamespacedTagLibMethodTests
    • TagLibMethodMissingSpec
    • MethodDefinedTagLibSpec
  • Full suite:
    • :grails-gsp:test
  • Functional validation:
    • :grails-test-examples-app1:integrationTest --tests functionaltests.MiscFunctionalSpec

Co-Authored-By: Oz oz-agent@warp.dev

davydotcom and others added 8 commits February 26, 2026 07:55
…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>
- 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>
@jamesfredley
Copy link
Copy Markdown
Contributor

Doc Examples

Files: namespaces.adoc, tagReturnValue.adoc
In namespaces.adoc, the new method-based tag was added but the old closure-based tag was also left in place, :

class SimpleTagLib {
    static namespace = "my"
    def example() {        // ← new method added
    def example = { attrs ->  // ← old closure NOT removed
        //...
    }

Same issue in tagReturnValue.adoc - def content() was inserted but def content = { attrs, body -> remains on the next line. Both are incomplete edits that will produce broken examples in the published docs.

isTagMethodCandidate Is Broad

Any public, non-static, non-getter/setter method on a TagLib class is treated as a tag candidate. The exclusion list is minimal:

  • afterPropertiesSet
  • get*() (zero-arg) / set*(x) (one-arg)
  • invokeMethod, methodMissing, propertyMissing
  • Methods declared on Object or GroovyObject
    This means toString(), hashCode(), equals(), other Spring lifecycle methods (e.g. destroy(), onApplicationEvent()), and any custom utility methods will all be registered as invokable tags. A TagLib with a helper method like def formatDate(Date d) would silently become a <g:formatDate> tag.

Namespace Property Guard Removed

registerNamespaceMetaProperty previously had a guard:

if (!metaClass.hasProperty(namespace) && !doesMethodExist(metaClass, getGetterName(namespace), ...)) {
    registerPropertyMissingForTag(...)
}

That guard was removed - namespace dispatchers now always overwrite. This could shadow real properties on TagLibs that happen to share a name with a registered namespace (e.g. a TagLib with a String my property when "my" is also a namespace).

registerTagMetaMethods Now Defaults overrideMethods = true

The signature changed to:

static void registerTagMetaMethods(MetaClass emc, TagLibraryLookup lookup, String namespace, boolean overrideMethods = true)

For the taglib's own namespace, tags now always override existing metaclass methods, whereas previously overrideMethods was false. This could break TagLibs that intentionally define a method sharing a name with a default-namespace tag (the local method would get silently overwritten by the tag dispatcher).

ThreadLocal Push Outside try Block

In GroovyPage.invokeTagLibMethod():

TagMethodContext.push(attrs, actualBody);          // ← outside try
Object tagResult = TagMethodInvoker.invokeTagMethod(tagLib, tagName, attrs, actualBody);
outputTagResult(returnsObject, tagResult);
} finally {
    TagMethodContext.pop();                         // ← inside finally

If an exception occurs between push() and the try block (or if invokeTagMethod throws before entering the try), the push has already happened but pop may not execute in the right scope. The push should be moved inside the try to guarantee the finally always cleans up exactly what was pushed.

Single-Parameter Fallback Heuristic in toMethodArguments

When a tag method has a single parameter and attrs has a single entry, the code uses the first value from the map regardless of whether the parameter name matches:

if (value == null && parameters.length == 1 && attrs != null && attrs.size() == 1) {
    value = attrs.values().iterator().next();
}

This is a magic heuristic that could silently bind the wrong attribute. For example, <g:myTag foo="bar"/> calling def myTag(String name) would bind "bar" to name even though the attribute is foo, not name. This makes debugging attribute-binding issues difficult.

davydotcom and others added 8 commits February 26, 2026 13:56
…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
@jdaugherty
Copy link
Copy Markdown
Contributor

@davydotcom I made some attempts at fixing the test pollution, but I think there is still more work required here.

@jdaugherty
Copy link
Copy Markdown
Contributor

From my AI Results:

  1. Generic test-isolation fix at the Spock extension level                                                                                                                                                       
                                                                                                                                                                                                                     
    Tests using UrlMappingsUnitTest no longer need per-test cleanup boilerplate. The fix lives in three places:                                                                                                      
                                                                                                                                                                                                                     
    - grails-testing-support-web/src/main/groovy/grails/testing/web/UrlMappingsUnitTest.groovy — mockArtefact() now clears artefactInfo and destroys the cached grailsUrlMappingsHolder singleton before
    re-registering. Added resetUrlMappingsForFeature() and cleanupUrlMappingsAfterFeature() helpers.                                                                                                                 
    - grails-testing-support-web/src/main/groovy/org/grails/testing/spock/UrlMappingSetupSpecInterceptor.groovy — now handles both setupSpec (mocking controllers) and setup (re-registering URL mappings before each
     feature method).                                                                                                                                                                                                
    - grails-testing-support-web/src/main/groovy/org/grails/testing/spock/UrlMappingCleanupInterceptor.groovy (new) — clears the URL mapping artefact registry after each feature method so non-UrlMappingsUnitTest
    specs running later in the same JVM don't inherit foreign mappings.                                                                                                                                              
    - grails-testing-support-web/src/main/groovy/org/grails/testing/spock/WebTestingSupportExtension.groovy — wires the new setup/cleanup interceptors.
    - grails-gsp/plugin/src/test/groovy/org/grails/web/mapping/RestfulReverseUrlRenderingTests.groovy — removed the now-redundant per-test setup/cleanup workaround.                                                 

@jdaugherty
Copy link
Copy Markdown
Contributor

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
pre-existing URL-mapping selection issue in DefaultUrlMappingsHolder — the holder contains the right mappings (verified via debug) but the lookup intermittently picks /$id? over /$dir/$id?. It's unrelated to
the test-isolation problem the PR's CI was hitting.

@jdaugherty
Copy link
Copy Markdown
Contributor

jdaugherty commented Apr 27, 2026

Update on this, from the AI:

          ⏺ The smoking gun! ReverseUrlMappingTests' setup updated linkGen=1573001442 but the test is using linkGen=1931205834 (from RestfulReverseUrlRenderingTests). Tag library state is shared across specs. 

So the tag library state isn't being cleaned up correctly.

Here's the code it used:
image

# 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
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.
@jdaugherty
Copy link
Copy Markdown
Contributor

@jamesfredley I think i've addressed all of the issues you raised.

@bito-code-review
Copy link
Copy Markdown

This question isn’t related to the pull request. I can only help with questions about the PR’s code or comments.

@jdaugherty jdaugherty force-pushed the feature/taglib-method-actions branch from 91be40c to ddeb082 Compare May 17, 2026 20:03
@jdaugherty jdaugherty added this to the grails:8.0.0-M2 milestone May 18, 2026
Copy link
Copy Markdown
Contributor

@jamesfredley jamesfredley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will push a small documentation commit in a few minutes. Looks good to go!!!

Adds three targeted upgrade-guide additions covering edge cases of the
method-based TagLib handler work in this PR that are not currently
surfaced to users:

upgrading70x.adoc section 16.1
- Add diagnostic guidance for the overrideMethods=true behavior change.
  The TagLibraryMetaUtils dispatcher logs each override at DEBUG, but
  shadowed user methods are otherwise silent at runtime. Document how to
  enable the logger in logback.xml so users can audit which of their
  TagLib methods are being replaced by tag dispatchers.

upgrading70x.adoc section 16.2 (new)
- Document the now-symmetric parameter-name rule for Map and Closure
  parameters in method-based tag handlers. A Closure parameter named
  anything other than `body` is treated as a named attribute rather
  than the tag body, matching the existing behavior for Map parameters
  named other than `attrs`. Includes a worked rename example and a
  pointer to the preserveParameterNames Gradle extension for users
  that opt out of parameter-name preservation.

upgrading80x.adoc section 21
- Clarify that the preserveParameterNames Gradle extension only governs
  Groovy compilation. Java sources receive -parameters transitively
  from the Spring Boot Gradle plugin (default since Spring Boot 3.2).
- Add an explicit recipe for users that compile TagLib classes outside
  the Grails Gradle plugin (standalone Groovy modules, plain Spring
  Boot apps without the Boot plugin, or builds that disable the Boot
  plugin's compiler configuration), and call out the silent
  MissingMethodException failure mode that occurs without the flags.

No code changes; documentation only.
Assisted-by: claude-code:claude-opus-4-7
The method-based TagLib handler feature ships in Grails 8.0 and is the
subject of this PR. Documentation for it was incorrectly placed in the
7.x upgrade guide (likely a carry-over from when this work originally
targeted the 7.x line). Move the entire feature umbrella to the 8.x
upgrade guide and consolidate the related sections.

upgrading70x.adoc
- Remove section 16 (Method-based TagLib Handlers) and its subsections
  16.1 (Behavior change: tag method registration) and 16.2 (Behavior
  change: parameter-name binding for Map and Closure parameters). None
  of this content describes a change that landed in Grails 7.

upgrading80x.adoc
- Replace section 21 (formerly "Grails Gradle Extension Preserves
  Parameter Names By Default") with a consolidated section 21
  (Method-based TagLib Handlers) covering the feature end to end:
  - 21. intro: feature overview, @tag annotation, @NotATag opt-out,
    closure-tag deprecation warning system property.
  - 21.1. Behavior change: tag method registration (overrideMethods
    default flipped to true, plus the DEBUG logging guidance for
    auditing shadowed user methods).
  - 21.2. Behavior change: parameter-name binding for Map and Closure
    parameters (symmetric reserved-name rule for `attrs` and `body`).
  - 21.3. Grails Gradle Extension Preserves Parameter Names By Default
    (the previous section 21 content, plus the Spring Boot
    -parameters note and the JavaCompile/GroovyCompile recipe for
    builds outside the Grails Gradle plugin).

No code changes; documentation only.

Assisted-by: claude-code:claude-opus-4-7
@jdaugherty
Copy link
Copy Markdown
Contributor

@codeconsole can you please confirm you are good to merge? James & I believe this is ready

@jdaugherty
Copy link
Copy Markdown
Contributor

@codeconsole do you have time to review again? @jamesfredley do you agree his feedback is implemented? should we merge or wait for his second review?

@jamesfredley
Copy link
Copy Markdown
Contributor

jamesfredley commented May 29, 2026

@jdaugherty I reviewed all feedback from everyone and believe it has been addressed. My two concerns on docs were handled in the two commits. I will merge 8.0.x in now so it actually runs the full CI.

@testlens-app
Copy link
Copy Markdown

testlens-app Bot commented May 29, 2026

✅ All tests passed ✅

🏷️ Commit: ac449c8
▶️ Tests: 40144 executed
⚪️ Checks: 35/35 completed


Learn more about TestLens at testlens.app.

Copy link
Copy Markdown
Contributor

@jamesfredley jamesfredley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

@jdaugherty
Copy link
Copy Markdown
Contributor

Asking @codeconsole again to confirm he's ok with these changes. I believe they've been addressed, and @jamesfredley I think agrees with me. My assumption is we'll discuss in the weekly if @codeconsole doesn't respond.

@jamesfredley jamesfredley merged commit a11ff6c into 8.0.x May 30, 2026
36 checks passed
@jamesfredley jamesfredley deleted the feature/taglib-method-actions branch May 30, 2026 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

4 participants