jamesfredley commented on code in PR #15699:
URL: https://github.com/apache/grails-core/pull/15699#discussion_r3342722702
##########
grails-web-databinding/src/main/groovy/org/grails/web/databinding/DefaultASTDatabindingHelper.java:
##########
@@ -145,9 +145,20 @@ private Set<String>
getPropertyNamesToIncludeInWhiteList(final SourceUnit source
final Set<String> propertyNamesToIncludeInWhiteList = new HashSet<>();
final Set<String> unbindablePropertyNames = new HashSet<>();
final Set<String> bindablePropertyNames = new HashSet<>();
+ final boolean isDomainClass = GrailsASTUtils.isDomainClass(classNode,
sourceUnit);
if (!classNode.getSuperClass().equals(new ClassNode(Object.class))) {
final Set<String> parentClassPropertyNames =
getPropertyNamesToIncludeInWhiteListForParentClass(sourceUnit,
classNode.getSuperClass());
- bindablePropertyNames.addAll(parentClassPropertyNames);
+ for (final String parentPropertyName : parentClassPropertyNames) {
+ // The id, version, dateCreated and lastUpdated properties of
a domain class are never bound by default.
Review Comment:
Good call - simplified to a concise two-liner that states only the *why*: a
domain class never binds its special properties by default, so they shouldn't
be inherited from a non-domain parent such as a `@DirtyCheck` base. Done in
bca8bdcc48.
##########
grails-web-databinding/src/main/groovy/org/grails/web/databinding/DefaultASTDatabindingHelper.java:
##########
@@ -145,9 +145,20 @@ private Set<String>
getPropertyNamesToIncludeInWhiteList(final SourceUnit source
final Set<String> propertyNamesToIncludeInWhiteList = new HashSet<>();
final Set<String> unbindablePropertyNames = new HashSet<>();
final Set<String> bindablePropertyNames = new HashSet<>();
+ final boolean isDomainClass = GrailsASTUtils.isDomainClass(classNode,
sourceUnit);
if (!classNode.getSuperClass().equals(new ClassNode(Object.class))) {
final Set<String> parentClassPropertyNames =
getPropertyNamesToIncludeInWhiteListForParentClass(sourceUnit,
classNode.getSuperClass());
- bindablePropertyNames.addAll(parentClassPropertyNames);
+ for (final String parentPropertyName : parentClassPropertyNames) {
+ // The id, version, dateCreated and lastUpdated properties of
a domain class are never bound by default.
+ // A parent class may legitimately include these in its own
whitelist when it is not itself recognised as
+ // a domain class (for example an abstract @DirtyCheck base
class in src/main/groovy onto which GORM injects
+ // id and version). Such properties must not be inherited into
a domain class' whitelist, otherwise the
+ // exclusion performed by shouldFieldBeInWhiteList is bypassed
via the inherited bindable property names.
+ if (isDomainClass &&
DOMAIN_CLASS_PROPERTIES_TO_EXCLUDE_BY_DEFAULT.contains(parentPropertyName)) {
Review Comment:
Good question - yes, command objects do flow through
`DefaultASTDatabindingHelper`. It's invoked from two transformers:
`ControllerActionTransformer` (for command objects, i.e. controller action
parameters) and `ControllerDomainTransformer` (for domain classes).
The change is safe for command objects because the new filter is guarded by
`isDomainClass`, and `GrailsASTUtils.isDomainClass()` returns `false` for them
- a command object has neither `@Entity`/`@jakarta.persistence.Entity` nor a
source path under `grails-app/domain/`.
This is consistent with the existing design: the
`DOMAIN_CLASS_PROPERTIES_TO_EXCLUDE_BY_DEFAULT` exclusion
(`id`/`version`/`dateCreated`/`lastUpdated`) was already gated on
`isDomainClass` everywhere it is applied - in `shouldFieldBeInWhiteList` (the
field loop) and in the setter-derived branch. So a command object with a field
named `id` or `version` still gets it included in its whitelist, exactly as
before. No behavior change for command objects; the fix only affects actual
GORM domain classes.
##########
grails-test-examples/gorm/src/integration-test/groovy/gorm/DirtyCheckBindingSpec.groovy:
##########
@@ -0,0 +1,67 @@
+/*
+ * 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 gorm
+
+import grails.testing.mixin.integration.Integration
+import spock.lang.Issue
+import spock.lang.Specification
+import spock.lang.Tag
+
+import org.apache.grails.testing.http.client.HttpClientSupport
+
+/**
+ * Functional test reproducing issue 15681 end-to-end: a real Grails
application binds request parameters
+ * (including {@code id} and {@code version}) to a domain class that extends
an abstract {@code @DirtyCheck}
+ * base. The framework must not bind {@code id} or {@code version} by default.
+ */
+@Integration(applicationClass = Application)
Review Comment:
Removed - it's bare `@Integration` now (bca8bdcc48). `gorm.Application` is
auto-discovered (same package, extends `GrailsAutoConfiguration`), and 11 of
the existing integration tests in this module already use bare `@Integration`,
so this matches the module's convention.
--
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]
