jamesfredley commented on code in PR #16491:
URL: https://github.com/apache/grails-core/pull/16491#discussion_r4174958218
##########
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:
Resolved in e5bbc7dad. Class-local lookup stays for a real command subclass.
A child that inherits Validateable without reimplementing it now has its own
constraints evaluated, a proxy is bound as the persistent class, and the
OpenAPI read-only gap is handled in GrailsModelConverter. The remaining case is
separate, and I will comment on that line.
##########
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:
Resolved in e5bbc7dad. InheritedValidateableCommand does not reimplement the
trait, and both bindData and DataBindingUtils cover the child-local bindable:
false case.
##########
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:
Resolved in e5bbc7dad. The sentence now covers a subclass that inherits
Validateable, and it no longer treats a proxy as an unenhanced subclass.
##########
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:
Resolved in e5bbc7dad. A null include list now still marks bindable: false
properties read-only, and CommandObjectSpec covers an unenhanced subclass.
##########
grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java:
##########
@@ -150,6 +176,10 @@ static List getBindablePropertyNames(final Object object) {
}
static List getUnbindablePropertyNames(final Object object) {
+ if (BindingIncludeLists.inheritsConstraintsMap(object.getClass())) {
+ // The inherited accessor holds the superclass's constraints, so
the class's own are evaluated once.
+ return getUnbindablePropertyNames(object.getClass());
Review Comment:
This treats any inherited `getConstraintsMap()` as generated `Validateable`
metadata. A subclass that inherits an ordinary instance getter, including the
getter Groovy generates for a superclass `Map constraintsMap` property, now
skips that instance map and uses the class-level evaluator instead.
The comment immediately below says those instance maps may differ per
instance and must not be replaced by a class-level result. A public
`bindObjectToInstance` check confirmed the regression: the previous code left
the protected property unchanged, and this commit overwrote it. The same early
return is at line 205.
`BindingIncludeLists.inheritsConstraintsMap` (`BindingIncludeLists.java:70`)
is the predicate that is too broad. It is true whenever
`getMethod("getConstraintsMap").getDeclaringClass()` is not the runtime class,
which is also true of a normal inherited instance getter.
Limit this fallback to the generated static `Validateable` accessor. Add a
public-API regression with a child that inherits an instance `constraintsMap`,
including two instances with different rules, and assert a `bindable: false`
property stays unchanged.
--
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]
