jamesfredley commented on code in PR #16491:
URL: https://github.com/apache/grails-core/pull/16491#discussion_r4174531757
##########
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:
Class-local lookup is the right fix for a genuine command subclass, but
these `getPublicDeclaredField` calls also drop protections the hierarchy walk
provided.
1. A child that inherits `Validateable` without reimplementing it can bind a
child-local `bindable: false` property.
`DataBindingUtils.getConstrainedProperties` accepts the inherited
`getConstraintsMap()`, which contains only the parent constraints, so the child
exclusion never reaches `SimpleDataBinder`. Previously the parent's generated
allowlist excluded the new property. Reproduced: a child property initialized
to `original` with `bindable: false` became `changed`.
2. A Hibernate proxy declares neither generated whitelist, so compatibility
mode now binds `id` and `version`. Reproduced against the previous lookup: this
code set `id` to `2` and `version` to `4`; the previous lookup left them at `1`
and `0`. Resolve proxy metadata against the persistent class, and keep
class-local lookup for a real command subclass.
Returning `null` from `propertyNames()` also makes `GrailsModelConverter`
skip `markUnbound()` (`GrailsModelConverter.groovy` around lines 423 and 445,
outside this diff), so inherited and local `bindable: false` properties lose
their OpenAPI read-only mark.
##########
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:
This fixture reimplements `InheritedBindingNullable`, so the child gets its
own constraints accessor and the inherited-trait `bindable: false` bypass is
masked. Add a child that does not reimplement the trait, and assert through
both `bindData` and `DataBindingUtils.bindObjectToInstance` that ordinary child
properties bind while a child-local `bindable: false` property stays unchanged.
A Hibernate proxy of a domain class needs the same public-API coverage for `id`
and `version`.
##########
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:
This overclaims the current behavior. Inherited and locally declared
`bindable: false` constraints do not still apply when the subclass inherits
`Validateable` without reimplementing it. Please qualify this sentence until
the lookup resolves the runtime subclass's constraints.
##########
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:
`null` is the intended compatibility result for binding an unenhanced
subclass, but `GrailsModelConverter` treats a null include list as "do not mark
unbound properties read-only". A subclass without its own generated metadata
then publishes inherited and local `bindable: false` properties as writable.
That consumer is outside this diff (`GrailsModelConverter.groovy` around lines
423 and 445). Please cover it with a document-generation regression: eligible
child properties writable, prohibited properties read-only.
--
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]
