Binding empties a raw collection in applications that never opted in to deny-by-default - #16209
Conversation
… Object Deny-by-default binding instantiates a nested type and binds the allowlisted properties into it, instead of handing the map to a constructor that would set whatever it was given. That is the right thing to do for a real nested type, but the element type is not always one. A collection written without a type argument reports Object as its component type: Basic#componentType falls back to Object.class when a property carries no generic signature. Object has a public no-arg constructor, so instantiation succeeds, and Object declares no properties, so binding into it puts the map's contents nowhere. The element is replaced by an empty Object and its data is gone, silently, before anything can fail on it. That is also why it escapes the opt-in: the instantiation happens in the try, while the deny-by-default check guards only the map-constructor fallback in the catch, so an application that never enabled the mode still loses the data. GORM then reports it as a missing codec for java.lang.Object, at a point far from the binding that caused it. Nothing can be mass-assigned through a value that is never used as a property source, so an element whose target type is Object is kept as it stands, which is what these collections did before. Every other type still instantiates and binds through the allowlist, and bindable: false is unaffected.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16209 +/- ##
==================================================
- Coverage 54.1238% 54.1158% -0.0080%
+ Complexity 20307 20304 -3
==================================================
Files 2107 2107
Lines 101144 101146 +2
Branches 17921 17922 +1
==================================================
- Hits 54743 54736 -7
- Misses 38595 38604 +9
Partials 7806 7806
🚀 New features to boost your workflow:
|
A raw Set reaches the same collection branch as a raw List and was corrupted the same way, so it is worth stating rather than leaving to be inferred from the List case. A raw Map is not affected: its branch keeps an element when the component type is assignable from it, and Object is assignable from everything. That is the guard the collection branch was missing, so it is covered here to keep the difference deliberate.
Three places bind an element into a collection, and two of them keep the item when the element type is already assignable from it: the array branch above and the Map branch below both do it, and SimpleDataBinder does it too. Only the collection branch went straight to instantiating, which is why a raw collection lost its data there and nowhere else. Asking the same question there fixes it in the same shape as its neighbours, rather than adding a special case for Object, and needs no change to instantiateAndBindNestedOrUseMapConstructor: a raw collection's component type is Object, Object is assignable from everything, so the element is kept. The SimpleDataBinder guard from the first commit is dropped with this: that class already asks at line 373 and was never affected.
The raw Map and non-domain cases passed before this change and after it, so
they documented behaviour rather than protecting it, and are dropped.
What is added instead all fails without the fix, each for a distinct reason:
- a property declared Collection rather than List or Set, so the branch is
reached through a third declared type,
- elements arriving as a DataBindingSource rather than a Map, which is the
other item shape that branch instantiates,
- deny-by-default enabled with the property allowlisted, which shows the loss
is not gated by the opt-in: the element is an empty Object there too, and
the property binds, so the failure is the data being destroyed rather than
the binding being denied.
| } | ||
| if (persistentInstance == null) { | ||
| if (item instanceof Map || item instanceof DataBindingSource) { | ||
| if (item == null || referencedType.isAssignableFrom(item.getClass())) { |
There was a problem hiding this comment.
IntelliJ complains: "Cannot resolve symbol 'getClass'"
inferred type: ?
| if (item == null || referencedType.isAssignableFrom(item.getClass())) { | |
| if (item == null || referencedType.isAssignableFrom((Object) item.getClass())) { |
There was a problem hiding this comment.
BEFORE 548: if (item == null || referencedType.isAssignableFrom(item.getClass())) {
AFTER 548: if (item == null || referencedType.isAssignableFrom((Object) item.getClass())) {
GrailsWebDataBinder.groovy: 548: [Static type checking] -
Cannot find matching method java.lang.Class#isAssignableFrom(java.lang.Object).
1 error
BUILD FAILED
There was a problem hiding this comment.
Sorry I think the cast must be in parentheses.
There was a problem hiding this comment.
Pushed as f408864 — def listValue → List listValue on line 508, where the value is introduced. That leaves item.getClass() uncast, matching the array branch above and the Map branch below.
Thanks for catching it.
|
Exception downstream |
Ok, just ignore my comment, sorry for not testing it. Must be a false positive in IntelliJ. |
@matrei that wasn't based on your suggestion. I was just adding the more complete stack trace from before the PR What is going on with IntelliJ? why does it report things that are not issues? I see these unrelated 3 errors in the same file. Are we wanting to code to IntelliJ complaints even though gradle compiles everything fine? |
|
@matrei if I change line 508 def listValueto List listValueit get's rid of but I still have the existing So I am just trying to understand why IntellJ complains about things gradle or the groovy compiler does not and whether or not we should care or fix everything IntelliJ complains about or if we need to fix the Groovy Grails plugin |
listValue is assigned from two branches and read a few lines later, and being declared def left its element type to inference. IntelliJ gives up and reports the item as `?`, which makes item.getClass() in the loop below look unresolved even though it compiles. Declaring it List answers that where the value is introduced, rather than casting at the point of use — the array and Map branches ask the same question of their items with no cast, and matching them is the point of this change.
✅ All tests passed ✅Test SummaryCI / Functional Tests (Java 25, indy=false) > :grails-test-examples-scaffolding:integrationTest
🏷️ Commit: f408864 Learn more about TestLens at testlens.app/docs. |
Impact
Silent data loss, in applications that did not opt in to the change that causes it.
List/Set/Collectionhas every element it binds replaced by an emptyjava.lang.Object. The values are gone before validation or persistence can see them.upgrading80x.adoc.grails.databinding.legacyBindableDefault=falseloses the data anyway, because the instantiation happens in thetrywhileisDenyByDefaultEnabled()guards only thecatch. There is a test for both settings.Can't find a codec for … java.lang.Objectfrom the BSON encoder, three modules from the data binder that destroyed the value. Tracing it took eliminating the MongoDB driver version, the embedded-MongoDB module, the codec registry and the datastore bean wiring first.8.0.0-M6 is staged but not yet on Maven Central, so this is still fixable before it becomes immutable.
Problem
A domain property declared as a raw collection loses its data during binding on 8.0.0-M6. Given:
every map element is replaced by an empty
new java.lang.Object(). With GORM for MongoDB the write then fails:The exception is incidental — it is the first thing to notice that the elements are no longer maps. Without a codec in the path the empty objects would simply be persisted, so the data is lost either way. This worked on 8.0.0-M5.
Cause
#15947 changed collection binding so a
Mapelement is instantiated as the component type and bound through the allowlist, rather than passed to a map constructor:That is right for a real nested type, but the component type is not always one.
Basic#componentTypefalls back toObject.classwhen a property carries no generic signature, so a raw collection arrives withreferencedType == Object.Objecthas a public no-arg constructor, sogetDeclaredConstructor().newInstance()succeeds, andObjectdeclares no properties, sobindNestedhas nowhere to put the map's contents.It is not gated by the opt-in, either: the instantiation is in the
try, whileisDenyByDefaultEnabled()guards only the map-constructor fallback in thecatch. The data is destroyed with the hardening on and with it off — there is a test for both.Fix
Three places bind an element into a collection, and the other two already keep the item when the element type is assignable from it:
if (item == null || componentType.isAssignableFrom(item.getClass()))Mapbranch —if (item == null || referencedType.isAssignableFrom(item.getClass()))SimpleDataBinderalso asks, atgenericType.isAssignableFrom(val?.getClass())Only the collection branch went straight to instantiating. This asks the same question there:
Objectis assignable from everything, so a raw collection's elements are kept. No special case forObject, no change toinstantiateAndBindNestedOrUseMapConstructor, and no change for any other component type — aMapelement whose target is a real nested type is not assignable from it and still instantiates and binds through the allowlist.bindable: falseis unaffected.That is the whole production change: one branch, in one file.
Scope
Only GORM domain classes, and only raw
List/Set/Collectionproperties whose elements are maps.A non-domain target (command object,
Validateable) is unaffected:SimpleDataBinderresolves a component type only from aParameterizedType, so a raw field yieldsnull, andGrailsWebDataBinderreachesBasic#componentTypeonly through aPersistentEntity. WithreferencedTypenull the branch is skipped by its own guard. A rawMapproperty is likewise unaffected, because its branch already asks the assignability question.Tests
Five tests in
GrailsWebDataBinderSpec, against aRawCollectionContainerdomain. Every one of them fails on an unmodified8.0.xbinder and passes with the fix, each covering a distinct way into the branch:Listof mapsSetof mapsCollection-typed propertyDataBindingSourceelementsThe failures report exactly the production symptom, e.g.
In the deny-by-default case
size() == 1holds first, so the property does bind and the failure is the data being destroyed rather than the binding being denied.:grails-databinding-core:test,:grails-web-databinding:test(includingDenyByDefaultConfigSpec) and thegrails.web.databinding.*suite ingrails-test-suite-persistenceare all green.Also verified end to end against the application that hit this: with this jar built at
8.0.0-M6and substituted into an otherwise stock M6 app, the raw-collection domain that previously failed boots and persists its maps intact.