Skip to content

Address review feedback on databinding whitelist characterization spec - #16054

Merged
borinquenkid merged 1 commit into
test/databinding-codegen-contractfrom
fix/databinding-whitelist-review-feedback
Jul 26, 2026
Merged

Address review feedback on databinding whitelist characterization spec#16054
borinquenkid merged 1 commit into
test/databinding-codegen-contractfrom
fix/databinding-whitelist-review-feedback

Conversation

@borinquenkid

Copy link
Copy Markdown
Member

Stacked on #16003.

Addresses review comments from @jdaugherty:

  • Renames the Object-typed field to untypedProperty and rewords test names — the property is excluded because it is raw Object-typed (DefaultASTDatabindingHelper#shouldFieldBeInWhiteList), not because it is generically "unlisted".
  • Drops the misleading hasOne association, which added nothing to the tested contract and declares an incomplete mapping (no belongsTo back-reference on WhitelistAddress).
  • Trims the domain test to its non-duplicated assertions and cross-references DefaultASTDatabindingHelperDomainClassSpecialPropertiesSpec, which already pins id/version/dateCreated/lastUpdated exclusion for domain classes.

Verification

  • :grails-test-suite-web:test --tests DefaultDatabindingWhitelistBehaviorSpec
  • ./gradlew codeStyle

- 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>
Copilot AI review requested due to automatic review settings July 26, 2026 04:11

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 updates the DefaultDatabindingWhitelistBehaviorSpec test to better characterize the databinding allowlist contract by clarifying why certain properties are excluded (raw Object/def typing) and by removing unrelated/duplicative assertions from the domain portion of the test.

Changes:

  • Renamed the previously “ignored” Object property to untypedProperty and updated test descriptions to reflect the actual exclusion rule.
  • Simplified the domain binding scenario by dropping special-property assertions and an unnecessary/misleading hasOne association mapping.
  • Added an inline cross-reference to the existing spec that already pins domain special-property exclusions.

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

@borinquenkid

Copy link
Copy Markdown
Member Author

Folding this directly into #16003 instead — no need for a separate stacked PR.

@borinquenkid
borinquenkid merged commit 1c34ff0 into test/databinding-codegen-contract Jul 26, 2026
48 checks passed
@borinquenkid
borinquenkid deleted the fix/databinding-whitelist-review-feedback branch July 26, 2026 04:15
@codecov

codecov Bot commented Jul 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 51.1147%. Comparing base (23f7c46) to head (1c34ff0).
⚠️ Report is 1 commits behind head on test/databinding-codegen-contract.

Additional details and impacted files

Impacted file tree graph

@@                             Coverage Diff                             @@
##             test/databinding-codegen-contract     #16054        +/-   ##
===========================================================================
- Coverage                              51.1210%   51.1147%   -0.0063%     
  Complexity                               17600      17600                
===========================================================================
  Files                                     2041       2041                
  Lines                                    95493      95493                
  Branches                                 16587      16587                
===========================================================================
- Hits                                     48817      48811         -6     
- Misses                                   39390      39399         +9     
+ Partials                                  7286       7283         -3     

see 4 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.

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

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

2 participants