jdaugherty commented on code in PR #16491:
URL: https://github.com/apache/grails-core/pull/16491#discussion_r4174763773
##########
grails-web-databinding/src/main/groovy/org/grails/web/databinding/BindingIncludeLists.java:
##########
@@ -101,11 +101,13 @@ public static List forType(final Class type, final
boolean denyByDefault) {
// target's constraints and would otherwise run on every bind
of a cached class.
final List runtimeBindableNames = denyByDefault ?
bindablePropertyNames(type) : null;
includeList = runtimeBindableNames;
- final Field legacyWhiteListField = getField(type,
DefaultASTDatabindingHelper.LEGACY_DATABINDING_WHITELIST);
+ // Compatibility metadata describes its declaring class, not
an unenhanced subclass.
+ // Match Grails 7's class-local lookup so a parent's generated
list cannot hide new properties.
+ final Field legacyWhiteListField =
getPublicDeclaredField(type,
DefaultASTDatabindingHelper.LEGACY_DATABINDING_WHITELIST);
final Field defaultWhiteListField = denyByDefault ?
getPairedField(type,
DefaultASTDatabindingHelper.DEFAULT_DATABINDING_WHITELIST,
DefaultASTDatabindingHelper.LEGACY_DATABINDING_WHITELIST) :
- getField(type,
DefaultASTDatabindingHelper.DEFAULT_DATABINDING_WHITELIST);
+ getPublicDeclaredField(type,
DefaultASTDatabindingHelper.DEFAULT_DATABINDING_WHITELIST);
Review Comment:
Fixed in e5bbc7dad7, keeping the class-local lookup for a real command
subclass:
1. `BindingIncludeLists.inheritsConstraintsMap` detects a class that answers
`getConstraintsMap()` with an accessor its superclass declares.
`constrainedProperties` and the binder's `bindable: false` lookup then evaluate
the runtime class's constraints, which include the inherited ones. The same
cause also hid the child's own `bindable: true` properties in secure mode; that
is fixed by the same change.
2. `DataBindingUtils.getBindingIncludeList` resolves a proxy through the
mapping context's `ProxyHandler` (`isProxy` / `getProxiedClass`) and binds it
with the persistent class's include list. A mapping context that GORM has not
initialized yet is treated as having no proxies.
3. When `propertyNames()` returns `null`, `GrailsModelConverter` now marks
`BindingIncludeLists.unbindablePropertyNames(type)` read only.
##########
grails-test-suite-uber/src/test/groovy/grails/test/mixin/InheritedCommandBindingSpec.groovy:
##########
@@ -0,0 +1,281 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package grails.test.mixin
+
+import java.time.LocalDate
+
+import spock.lang.Specification
+import spock.lang.Unroll
+
+import grails.artefact.Artefact
+import grails.testing.web.controllers.ControllerUnitTest
+import grails.validation.Validateable
+import grails.web.databinding.DataBindingUtils
+
+/**
+ * Reproduces binding a dynamically constructed command whose superclass was
enhanced as an action parameter.
+ */
+class InheritedCommandBindingSpec extends Specification implements
ControllerUnitTest<InheritedCommandBindingController> {
+
+ private static final LocalDate DATE_VALUE = LocalDate.of(2000, 1, 2)
+
+ void setup() {
+ grailsApplication.config.grails.databinding.denyByDefault = false
+ params.putAll(requestValues())
+ }
+
+ void 'bindData binds subclass fields when only the superclass is an action
parameter'() {
+ when:
+ def command = controller.bindDynamic().command
+
+ then: 'the inherited and subclass properties are all bindable in
compatibility mode'
+ verifyAll(command) {
+ baseValues == ['item-1']
+ textValue == 'updated text'
+ dateValue == DATE_VALUE
+ enabled
+ }
+ }
+
+ void 'DataBindingUtils also binds fields declared by the dynamically
constructed subclass'() {
+ given:
+ def command = new InheritedBindingDynamicCommand()
+
+ when:
+ DataBindingUtils.bindObjectToInstance(command, requestValues())
+
+ then:
+ verifyAll(command) {
+ baseValues == ['item-1']
+ textValue == 'updated text'
+ dateValue == DATE_VALUE
+ enabled
+ }
+ }
+
+ void 'an explicit include list can bind the same subclass fields'() {
+ when:
+ def command = controller.bindDynamicExplicitly().command
+
+ then:
+ verifyAll(command) {
+ baseValues == ['item-1']
+ textValue == 'updated text'
+ dateValue == DATE_VALUE
+ enabled
+ }
+ }
+
+ void 'a subclass declared as an action parameter binds its own fields'() {
+ when:
+ def command = controller.bindDeclared().command
+
+ then:
+ verifyAll(command) {
+ baseValues == ['item-1']
+ textValue == 'updated text'
+ dateValue == DATE_VALUE
+ enabled
+ }
+ }
+
+ void 'the same properties bind without an enhanced superclass'() {
+ when:
+ def command = controller.bindStandalone().command
+
+ then:
+ verifyAll(command) {
+ baseValues == ['item-1']
+ textValue == 'updated text'
+ dateValue == DATE_VALUE
+ enabled
+ }
+ }
+
+ void 'an explicit include list still restricts subclass binding'() {
+ when:
+ def command = controller.bindDynamicRestricted().command
+
+ then:
+ command.baseValues == ['item-1']
+ command.textValue == 'updated text'
+ command.dateValue == null
+ !command.enabled
+ }
+
+ void 'an empty explicit include list still binds no properties'() {
+ when:
+ def command = controller.bindDynamicEmpty().command
+
+ then:
+ command.baseValues == null
+ command.textValue == null
+ command.dateValue == null
+ !command.enabled
+ }
+
+ @Unroll
+ void 'bindable false remains enforced with secure=#secure and explicit
includes=#explicit'() {
+ given:
+ grailsApplication.config.grails.databinding.denyByDefault = secure
+
+ when:
+ def command = explicit ? controller.bindProtectedFields().command :
controller.bindDynamic().command
+
+ then:
+ command.protectedBaseValue == 'base value'
+ command.protectedChildValue == 'child value'
+
+ where:
+ secure | explicit
+ false | false
+ false | true
+ true | false
+ true | true
+ }
+
+ void 'secure mode permits only explicitly bindable inherited and subclass
properties'() {
+ given:
+ grailsApplication.config.grails.databinding.denyByDefault = true
+
+ when:
+ def command = controller.bindDynamic().command
+
+ then:
+ command.baseValues == ['item-1']
+ command.textValue == 'updated text'
+ command.dateValue == null
+ !command.enabled
+ }
+
+ void 'switching modes does not reuse the other modes cached include
list'() {
+ expect:
+ controller.bindDynamic().command.dateValue == DATE_VALUE
+
+ when:
+ grailsApplication.config.grails.databinding.denyByDefault = true
+
+ then:
+ controller.bindDynamic().command.dateValue == null
+
+ when:
+ grailsApplication.config.grails.databinding.denyByDefault = false
+
+ then:
+ controller.bindDynamic().command.dateValue == DATE_VALUE
+ }
+
+ private static Map requestValues() {
+ [baseValues: ['item-1'], textValue: 'updated text', dateValue:
DATE_VALUE, enabled: true,
+ protectedBaseValue: 'changed', protectedChildValue: 'changed']
+ }
+}
+
+@Artefact('Controller')
+class InheritedCommandBindingController {
+
+ // Referencing the base type generates its binding metadata without
enhancing a dynamically constructed subclass.
+ def bindBase(InheritedBindingBaseCommand command) {
+ [command: command]
+ }
+
+ def bindDynamic() {
+ def command = new InheritedBindingDynamicCommand()
+ bindData(command, params)
+ [command: command]
+ }
+
+ def bindDynamicExplicitly() {
+ def command = new InheritedBindingDynamicCommand()
+ bindData(command, params, [include: ['baseValues', 'textValue',
'dateValue', 'enabled']])
+ [command: command]
+ }
+
+ def bindDynamicRestricted() {
+ def command = new InheritedBindingDynamicCommand()
+ bindData(command, params, [include: ['baseValues', 'textValue']])
+ [command: command]
+ }
+
+ def bindDynamicEmpty() {
+ def command = new InheritedBindingDynamicCommand()
+ bindData(command, params, [include: []])
+ [command: command]
+ }
+
+ def bindProtectedFields() {
+ def command = new InheritedBindingDynamicCommand()
+ bindData(command, params, [include: ['protectedBaseValue',
'protectedChildValue']])
+ [command: command]
+ }
+
+ def bindDeclared(InheritedBindingDeclaredCommand command) {
+ [command: command]
+ }
+
+ def bindStandalone() {
+ def command = new InheritedBindingStandaloneCommand()
+ bindData(command, params)
+ [command: command]
+ }
+}
+
+trait InheritedBindingNullable extends Validateable implements Serializable {
+ static boolean defaultNullable() {
+ true
+ }
+}
+
+class InheritedBindingBaseCommand implements InheritedBindingNullable {
+ List<String> baseValues
+ String protectedBaseValue = 'base value'
+
+ static constraints = {
+ baseValues bindable: true
+ protectedBaseValue bindable: false
+ }
+}
+
+abstract class InheritedBindingIntermediateCommand extends
InheritedBindingBaseCommand {
+}
+
+class InheritedBindingDynamicCommand extends
InheritedBindingIntermediateCommand implements InheritedBindingNullable {
Review Comment:
Added in e5bbc7dad7:
- `InheritedValidateableCommand` extends the intermediate class without
reimplementing the trait. Through both `bindData` and
`DataBindingUtils.bindObjectToInstance`, its ordinary properties bind while the
inherited and child-local `bindable: false` properties stay unchanged. A
secure-mode case checks that its own `bindable: true` property binds.
- Hibernate proxy coverage in the H7 and H5 `gorm` example apps
(`DirtyCheckBindingSpec`): a record loaded with `load()` in a fresh session is
bound with `bindData` over HTTP and with
`DataBindingUtils.bindObjectToInstance` directly, and `id` and `version` stay
unchanged. Both features fail without the proxy resolution (`id` 102, `version`
5).
- `DomainProxyBindingSpec` in the uber suite covers GORM's javassist
proxies, and `DataBindingUtilsSpec` covers proxy resolution and binding before
GORM has initialized.
##########
grails-doc/src/en/ref/Controllers/bindData.adoc:
##########
@@ -65,6 +65,8 @@ Arguments:
If no `include` list is supplied, `bindData` uses the target class default
binding behavior. By default, statically typed instance properties bind for
compatibility unless they are marked `bindable: false`. Existing `bindable:
true` declarations and explicit `include` lists continue to bind exactly the
properties they name without configuration changes. An empty `include` list
binds no properties.
+In compatibility mode, a command subclass's own properties remain bindable
when the subclass is constructed dynamically, even if only its superclass is
declared as a controller action parameter. Declaring a superclass as an action
parameter does not restrict such a subclass to the superclass's properties.
Inherited and locally declared `bindable: false` constraints still apply.
Review Comment:
With the runtime subclass's constraints resolved in e5bbc7dad7, the sentence
now holds. I made it explicit that it covers a subclass that inherits
`Validateable`, added the secure-mode `bindable: true` behavior, and noted that
a proxy returned by `load()` binds exactly as its domain class does.
##########
grails-web-databinding/src/test/groovy/grails/web/databinding/DataBindingUtilsSpec.groovy:
##########
@@ -98,22 +98,22 @@ class DataBindingUtilsSpec extends Specification {
command.version == null
}
- void 'test a whitelist declared by a super class also restricts a sub
class'() {
+ void 'test a superclass whitelist does not restrict an unenhanced subclass
in compatibility mode'() {
given:
def command = new SubclassOfWhitelistedCommand()
when:
DataBindingUtils.bindObjectToInstance(command, [name: 'Grails',
version: '8'])
- then: 'the inherited whitelist applies to the sub class as well'
+ then: 'only a whitelist declared on the bound class describes its
eligible properties'
command.name == 'Grails'
- command.version == null
+ command.version == '8'
}
void 'test the include list of a type is the one its instances are bound
with'() {
expect:
BindingIncludeLists.propertyNames(WhitelistedCommand) == ['name']
- BindingIncludeLists.propertyNames(SubclassOfWhitelistedCommand) ==
['name']
+ BindingIncludeLists.propertyNames(SubclassOfWhitelistedCommand) == null
Review Comment:
Covered in e5bbc7dad7. When `propertyNames()` is `null`,
`GrailsModelConverter` now marks the type's `bindable: false` properties read
only. `CommandObjectSpec` adds a document-generation regression:
`PickupAddressCommand` is an action parameter, and its subclass
`DeliveryAddressCommand` (inherits `Validateable`, no list of its own) is
nested in another command. The eligible inherited and child properties are
writable, and the inherited and child-local `bindable: false` properties are
read only. The feature fails without the converter change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
