Skip to content

Characterize data-binding allowlist through public binding API - #16003

Open
jamesfredley wants to merge 2 commits into
8.0.xfrom
test/databinding-codegen-contract
Open

Characterize data-binding allowlist through public binding API#16003
jamesfredley wants to merge 2 commits into
8.0.xfrom
test/databinding-codegen-contract

Conversation

@jamesfredley

Copy link
Copy Markdown
Contributor

Summary

  • add a test-only characterization spec that pins the compile-time data-binding allowlist contract via OBSERVABLE behavior through the public binding API
  • covers a domain (via DataBindingUtils.bindObjectToInstance) and a controller-action @Validateable command
  • asserts which properties actually bind vs stay null, so the test tracks the generated allowlist's effect rather than an internal field

Behavior characterized (current 8.0.x)

  • Domain: name and nested address.street bind; id/version/dateCreated/lastUpdated and a declared non-allowlisted Object property stay null
  • @Validateable command: declared special properties bind; the non-allowlisted property stays null

Scope note

Test-only; no production changes. Exercises only public data-binding APIs (per the repo's test-through-public-API policy); it does not read the AST-generated allowlist field.

Verification

  • :grails-test-suite-web:test --tests DefaultDatabindingWhitelistBehaviorSpec
  • :grails-test-suite-web:test
  • git diff --check

This is an AI-generated starting point pinning the data-binding code-generation contract.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a characterization test in grails-test-suite-web to pin the observable behavior of the compile-time data-binding allowlist via public data-binding APIs, without inspecting internal AST-generated fields.

Changes:

  • Introduces a new Spock spec that verifies which properties bind vs remain unset for a domain instance when binding via DataBindingUtils.bindObjectToInstance.
  • Adds a controller-action command object scenario (a Validateable command) to assert allowlist behavior through controller command binding, including nested property binding.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov

codecov Bot commented Jul 17, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.1210%. Comparing base (0c63c26) to head (23f7c46).

Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##                8.0.x     #16003        +/-   ##
==================================================
+ Coverage     51.1168%   51.1210%   +0.0042%     
- Complexity      17597      17600         +3     
==================================================
  Files            2041       2041                
  Lines           95493      95493                
  Branches        16587      16587                
==================================================
+ Hits            48813      48817         +4     
+ Misses          39394      39390         -4     
  Partials         7286       7286                

see 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Long version
Date dateCreated
Date lastUpdated
Object ignored

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

"Unlisted" / "non-allowlisted" slightly mischaracterizes why this property doesn't bind: shouldFieldBeInWhiteList (DefaultASTDatabindingHelper.java:277) excludes it specifically because its static type is Object without generics. There is no general "unlisted" category — every eligible typed, non-transient, non-special property is auto-whitelisted, so e.g. an extra String property here would bind. Suggest naming this Object untypedProperty (or a def field) and wording the test name as "excludes special and Object/def-typed properties" so the pinned rule reads as what it is.

Date lastUpdated
Object ignored

static hasOne = [address: WhitelistAddress]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

hasOne declares a bidirectional association whose foreign key lives on the child, and WhitelistAddress has no back-reference — GORM would reject this mapping if the domain were ever registered with a mapping context (it isn't here, since the spec uses ControllerUnitTest without DataTest, which is why nothing complains). The typed address property alone is what puts address/address.* in the generated whitelist, so hasOne adds nothing to the contract under test. Suggest dropping it (and possibly the field initializer tells the nested-binding story on its own).


class DefaultDatabindingWhitelistBehaviorSpec extends Specification implements ControllerUnitTest<WhitelistBehaviorController> {

void 'domain binding includes simple and association properties but excludes special and unlisted properties'() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Partial overlap: DefaultASTDatabindingHelperDomainClassSpecialPropertiesSpec in this same package already pins the domain special-property exclusions (id/version via #15681, dateCreated/lastUpdated via GRAILS-11173), also through observable binding behavior. The genuinely new coverage here is the Object-typed exclusion, nested association binding, and the Validateable command including id/version/dateCreated/lastUpdated. Worth a cross-reference comment, or trimming this first test to the non-duplicated assertions, so the two specs don't drift apart when the contract changes.

@jamesfredley jamesfredley moved this to Todo in Apache Grails Jul 24, 2026
- Rename the Object-typed field to untypedProperty and reword test
  names: the property is excluded because it is raw Object-typed
  (DefaultASTDatabindingHelper#shouldFieldBeInWhiteList), not because
  it is generically "unlisted".
- Drop the misleading hasOne association, which adds nothing to the
  contract under test since the typed address field alone puts it in
  the generated whitelist, and the mapping is incomplete (no belongsTo
  back-reference on WhitelistAddress).
- Trim the domain test to its non-duplicated assertions and
  cross-reference DefaultASTDatabindingHelperDomainClassSpecialPropertiesSpec,
  which already pins id/version/dateCreated/lastUpdated exclusion for
  domain classes.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@testlens-app

testlens-app Bot commented Jul 26, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: 1c34ff0
▶️ Tests: 86290 executed
⚪️ Checks: 114/114 completed


Learn more about TestLens at testlens.app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

4 participants