Skip to content

Clear omitted bindData fields when nullMissing is enabled - #15950

Open
jamesfredley wants to merge 14 commits into
fix/binddata-mass-assignmentfrom
fix/binddata-null-missing-stale-data
Open

Clear omitted bindData fields when nullMissing is enabled#15950
jamesfredley wants to merge 14 commits into
fix/binddata-mass-assignmentfrom
fix/binddata-null-missing-stale-data

Conversation

@jamesfredley

@jamesfredley jamesfredley commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Description

Adds opt-in stale-data clearing to bindData, stacked on the binding base from #15947.

When nullMissing: true is supplied together with an explicit include list, included properties omitted from the source are cleared (reference types to null, primitives to their type default). Existing behavior is unchanged when the option is absent, false, or no explicit include was supplied.

Final contract

Area Behavior
Opt-in bindData(target, source, [include: [...], nullMissing: true])
Default Omitted fields retain their existing value
Authorization Explicit includes and (in secure mode) generated binding allowlists authorize clearing
Denials Excludes, bindable: false, and framework-managed properties remain protected
Primitives Omitted included primitives reset to the type default (e.g. 0 / false)
Failures Clear failures are reported on the returned BindingResult
Paths Nested objects, lists, arrays, maps, indexed paths, and filters are supported
Wildcards Wildcard matching is isolated to nullMissing; it cannot broaden normal request binding
Compatibility Existing public overloads continue to delegate with nullMissing = false
Modes Works under the shipping permissive default and under opt-in secure mode

Implementation notes

Example

bindData book, params, [include: ['title', 'description'], nullMissing: true]

Verification

  • :grails-web-databinding:test
  • :grails-databinding-core:test
  • Focused BindDataMethodTests (default-mode and secure-mode nullMissing paths, primitives, BindingResult errors, nested/indexed/maps, excludes, bindable: false)
  • DefaultASTDatabindingHelperDomainClassSpecialPropertiesSpec (explicit bindable special properties)

Related

PR Relationship
#15947 Binding base (permissive default + opt-in secure mode) merged into this PR
#15808 Earlier combined approach, closed as superseded by #15947 and #15950

Contributor Checklist

  • Tests cover the affected public behavior in the shipping default mode
  • User-facing behavior is documented in the bindData reference and Grails 8 upgrade guide
  • Default behavior and existing overloads remain compatible
  • The PR is labeled ai-generated-starting-point
  • All contributed code is provided under the Apache License 2.0

Assisted-by: Sisyphus:xai/grok-4.5 [gpt-coding]

When nullMissing is true and an include allowlist is provided, omitted
allowlisted properties are set to null. Default remains leave-unchanged.

Assisted-by: Sisyphus:xai/grok-4.5 [gpt-coding]

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

Adds opt-in “null missing” semantics to bindData so that, when nullMissing: true is provided alongside an explicit include allowlist, included properties omitted from the binding source are actively cleared (null) rather than leaving stale values on the target object. This extends Grails’ web data binding behavior to better support typical “edit/update” form semantics without enabling the behavior by default.

Changes:

  • Introduces a nullMissing option plumbed through DataBinderDataBindingUtils, and applies clearing only when an explicit include list is provided.
  • Implements missing-field clearing logic in DataBindingUtils (including nested indexed collection paths and map-indexed paths).
  • Adds test coverage and updates documentation + upgrading notes to describe the new opt-in behavior.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java Adds nullMissing overloads and implements missing included-property clearing logic after binding.
grails-web-databinding/src/main/groovy/grails/web/databinding/DataBinder.groovy Wires nullMissing: true from the bindData options map into the binding call.
grails-test-suite-web/src/test/groovy/org/grails/web/servlet/BindDataMethodTests.groovy Adds new controller-backed specs covering nullMissing clearing, excludes, nested indexed paths, map paths, and bindable whitelist interactions.
grails-doc/src/en/ref/Controllers/bindData.adoc Documents nullMissing usage and constraints (opt-in; requires include).
grails-doc/src/en/guide/upgrading/upgrading80x.adoc Notes the behavior change for Grails 8.x upgrades (opt-in; only with include).

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

@bito-code-review

Copy link
Copy Markdown

The observation regarding isPropertyAllowedByWhitelist is accurate. The current implementation relies on exact matches and simple prefix checks, which fails to account for the wildcard semantics used by the binder (e.g., address.* or address_*). Updating the whitelist evaluation to support these patterns would ensure that nested properties are correctly identified as bindable, allowing nullMissing to function consistently across nested paths.

@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 2.41758% with 444 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.0565%. Comparing base (21e715c) to head (69ed46e).

Files with missing lines Patch % Lines
...ls/web/databinding/NullMissingPropertyClearer.java 0.0000% 418 Missing ⚠️
...roovy/grails/web/databinding/DataBindingUtils.java 16.0000% 14 Missing and 7 partials ⚠️
.../grails/web/databinding/GrailsWebDataBinder.groovy 40.0000% 1 Missing and 2 partials ⚠️
.../groovy/grails/databinding/SimpleDataBinder.groovy 50.0000% 0 Missing and 2 partials ⚠️
Additional details and impacted files

Impacted file tree graph

@@                          Coverage Diff                           @@
##             fix/binddata-mass-assignment     #15950        +/-   ##
======================================================================
- Coverage                         52.2735%   52.0565%   -0.2169%     
- Complexity                          18383      18394        +11     
======================================================================
  Files                                2036       2038         +2     
  Lines                               96856      97300       +444     
  Branches                            16961      17097       +136     
======================================================================
+ Hits                                50630      50651        +21     
- Misses                              38749      39170       +421     
- Partials                             7477       7479         +2     
Files with missing lines Coverage Δ
...ovy/grails/databinding/FrameworkPropertyNames.java 100.0000% <100.0000%> (ø)
.../groovy/grails/databinding/SimpleDataBinder.groovy 74.5050% <50.0000%> (-0.1219%) ⬇️
.../grails/web/databinding/GrailsWebDataBinder.groovy 30.3602% <40.0000%> (+0.1197%) ⬆️
...roovy/grails/web/databinding/DataBindingUtils.java 53.7791% <16.0000%> (-2.7024%) ⬇️
...ls/web/databinding/NullMissingPropertyClearer.java 0.0000% <0.0000%> (ø)

... and 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.

Preserve the strict binding allowlist while adding opt-in stale-data clearing, including indexed paths and explicit include handling.

Assisted-by: opencode:gpt-5.6-sol
@jamesfredley

Copy link
Copy Markdown
Contributor Author

Final update pushed in 018220fb3e.

The branch now incorporates #15947 as its deny-by-default security base and keeps nullMissing as a focused opt-in behavior. Review fixes cover wildcard sibling authorization, indexed excludes and includes, explicit include semantics, null generated allowlists, public boolean overload safety, recursive bindable: false, and framework-managed properties.

Verification passed for :grails-databinding-core:test, :grails-databinding:test, :grails-web-databinding:test, and :grails-test-suite-web:test. The final delta completed three rounds of Oracle and Codex review; all findings within #15950's six-file stacked scope are resolved.

@jamesfredley jamesfredley moved this to Todo in Apache Grails Jul 24, 2026
@borinquenkid

Copy link
Copy Markdown
Member

Thanks for splitting this out — since #15950 is branched directly off #15947, its diff currently duplicates all of #15947's changes on top of the nullMissing additions, which means reviewers end up reviewing the mass-assignment hardening twice.

Would you be open to one of:

  1. Retargeting this PR's base branch to fix/binddata-mass-assignment (the Add opt-in deny-by-default data binding and always honor bindable:false #15947 branch), so this shows only the incremental nullMissing commits — GitHub will auto-retarget it to 8.0.x once Add opt-in deny-by-default data binding and always honor bindable:false #15947 merges, or
  2. Folding this PR's commits into Add opt-in deny-by-default data binding and always honor bindable:false #15947 so it's reviewed as a single PR.

Whichever is easier on your end — just flagging so we don't duplicate review effort.

@borinquenkid borinquenkid added this to the grails:8.0.0-RC1 milestone Jul 28, 2026
@jamesfredley
jamesfredley changed the base branch from 8.0.x to fix/binddata-mass-assignment July 29, 2026 13:45
@jamesfredley

Copy link
Copy Markdown
Contributor Author

PR updated to target fix/binddata-mass-assignment

@jdaugherty jdaugherty 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.

The nullMissing contract is well scoped — opt-in, requires an explicit include, defaults unchanged — and the reference docs read clearly.

Two structural concerns before this lands.

First, the clearing pass re-derives "which included properties were absent from the source" by re-parsing the binding source after binding has already finished. That is why DataBindingUtils grows roughly 640 lines of bespoke path handling, duplicating traversal the binder just performed and already has type information for.

Second, every nullMissing spec inherits legacyBindableDefault=false from the BindDataMethodTests setup(), so the feature has no coverage under the shipping default — even though the docs present it as a plain bindData option with no mention of the binding mode.

Smaller items inline.

…ssing-stale-data

Preserve permissive compatibility binding and the nullMissing stale-data clearing behavior while resolving overlapping documentation, tests, and binding utilities.

Assisted-by: opencode:gpt-5.6-sol
Use one shared property-name set across the core and web binders while preserving the internal bind-all marker across package boundaries.

Assisted-by: opencode:gpt-5.6-sol
Move omitted-property path handling into a dedicated collaborator, preserve existing binding errors, report clear failures, and reset primitive properties to type defaults.

Assisted-by: opencode:gpt-5.6-sol
Remove the duplicate specification cleanup that referenced per-feature state and prevented the merged test source from compiling.

Assisted-by: opencode:gpt-5.6-sol
SimpleDataBinder must only hard-deny intrinsic runtime properties.
Grails-managed id/version/dateCreated/lastUpdated/errors remain
excluded from default allowlists and nullMissing clearing, but can
still bind when explicitly allowed (bindable: true).

Assisted-by: Sisyphus:xai/grok-4.5
@jamesfredley

Copy link
Copy Markdown
Contributor Author

@jdaugherty Addressed after merging the updated #15947 base (50714be10c).

Review responses

Item Resolution
Post-bind path re-walk (~640 lines in DataBindingUtils) Extracted to package-private NullMissingPropertyClearer with coverage through public bindData / DataBindingUtils APIs. Clearing remains intentionally post-bind (documented).
nullMissing only tested under secure mode After the #15947 merge, BindDataMethodTests runs on the unconfigured permissive default. Added/kept default-mode coverage for clear omitted include, leave excluded, bindable: false, nested/indexed/map paths. Secure-mode cases stay explicit via enableSecureBinding().
Swallowed clear failures Primitive includes reset to the type default. Clear failures produce FieldErrors on the returned BindingResult (full nested path preserved); existing conversion errors are kept.
Three disagreeing framework-property lists Single FrameworkPropertyNames with intrinsic runtime vs Grails-managed sets. SimpleDataBinder only hard-denies intrinsic runtime properties so bindable: true on id/dateCreated/lastUpdated still works; nullMissing uses the full managed set.
Missing javadoc on public nullMissing overloads Documented: ignored without a non-null include; clearing runs after bind; listener/BindingResult behavior described.
Public SimpleDataBinder.isPropertyExcluded Removed from public API; exclusion matching lives in the web-layer clearer.

Verification

:grails-databinding-core:test, :grails-web-databinding:test, and focused :grails-test-suite-web:test (BindDataMethodTests, DefaultASTDatabindingHelperDomainClassSpecialPropertiesSpec) all green.

Bring the reviewed deny-by-default setting and listener fixes into the
stacked branch. Preserve nullMissing behavior while documenting its public
overloads and correcting the remaining Java style violations.

Assisted-by: Sisyphus:gpt-5.6-sol [gpt-terra] [codex-review]
@testlens-app

testlens-app Bot commented Aug 6, 2026

Copy link
Copy Markdown

✅ All tests passed ✅

🏷️ Commit: bcb05af
▶️ Tests: 41744 executed
⚪️ Checks: 55/55 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

Labels

None yet

Projects

Status: Todo

Development

Successfully merging this pull request may close these issues.

4 participants