Fix bindData binding id/version when domain extends a @DirtyCheck base (#15681)

grails-test-examples/gorm/build.gradle

@@ -61,6 +61,7 @@ dependencies {
     console 'org.apache.grails:grails-console'
 
     integrationTestImplementation testFixtures('org.apache.grails:grails-geb')
+    integrationTestImplementation project(':grails-testing-support-http-client')
 }
 
 apply {

grails-test-examples/gorm/grails-app/controllers/gorm/DirtyCheckBindingController.groovy

@@ -0,0 +1,34 @@
+/*
+ *  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
+
+/**
+ * Controller used by the functional test for issue 15681. It binds the incoming request parameters to a new
+ * {@link DirtyCheckedRecord} and renders the resulting {@code id}, {@code version} and {@code description} so the
+ * test can assert over HTTP that {@code id}/{@code version} were not bound by default.
+ */
+class DirtyCheckBindingController {
+
+    def bind() {
+        def record = new DirtyCheckedRecord()
+        bindData(record, params)
+        render "id=${record.id}|version=${record.version}|description=${record.description}"
+    }
+}

grails-test-examples/gorm/grails-app/domain/gorm/DirtyCheckedRecord.groovy

@@ -0,0 +1,32 @@
+/*
+ *  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
+
+/**
+ * Concrete GORM domain class (located in {@code grails-app/domain}) that extends an abstract
+ * {@code @DirtyCheck} base defined in {@code src/main/groovy}. Data binding must never bind
+ * {@code id} or {@code version} on this class by default. See issue 15681.
+ */
+class DirtyCheckedRecord extends AbstractDirtyCheckedRecord {
+
+    static constraints = {
+        description nullable: true
+    }
+}

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)
+@Tag('http-client')
+class DirtyCheckBindingSpec extends Specification implements HttpClientSupport {
+
+    private static final String FORM = 'application/x-www-form-urlencoded'
+
+    @Issue('https://github.com/apache/grails-core/issues/15681')
+    void 'bindData over HTTP does not bind id or version on a domain extending a @DirtyCheck base'() {
+        when: 'a form submission posts id, version and a regular property'
+        def response = httpPost('/dirtyCheckBinding/bind', 'id=99&version=5&description=Opening+balance', FORM)
+
+        then: 'the request succeeds'
+        response.assertStatus(200)
+
+        and: 'the regular property is bound'
+        response.assertContains('description=Opening balance')
+
+        and: 'id and version are not bound, matching the documented default behaviour'
+        response.assertContains('id=null')
+        response.assertContains('version=null')
+    }
+
+    @Issue('https://github.com/apache/grails-core/issues/15681')
+    void 'binding only a regular property over HTTP leaves id and version null'() {
+        when:
+        def response = httpPost('/dirtyCheckBinding/bind', 'description=Closing+balance', FORM)
+
+        then:
+        response.assertStatus(200)
+        response.assertContains('description=Closing balance')
+        response.assertContains('id=null')
+        response.assertContains('version=null')
+    }
+}

grails-test-examples/gorm/src/main/groovy/gorm/AbstractDirtyCheckedRecord.groovy

@@ -0,0 +1,36 @@
+/*
+ *  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.gorm.dirty.checking.DirtyCheck
+import groovy.transform.CompileStatic
+
+/**
+ * Abstract base class living in {@code src/main/groovy} (i.e. not itself a domain class) annotated with
+ * {@link DirtyCheck}. GORM injects {@code id} and {@code version} onto the furthest unresolved parent in the
+ * hierarchy, which is this class. This reproduces the conditions of
+ * <a href="https://github.com/apache/grails-core/issues/15681">issue 15681</a> where {@code id} and
+ * {@code version} would incorrectly leak into the data binding whitelist of the concrete domain subclass.
+ */
+@DirtyCheck
+@CompileStatic
+abstract class AbstractDirtyCheckedRecord {
+    String description
+}

grails-test-suite-web/src/test/groovy/org/grails/web/binding/DefaultASTDatabindingHelperDomainClassSpecialPropertiesSpec.groovy

@@ -18,7 +18,9 @@
  */
 package org.grails.web.binding
 
+import grails.gorm.dirty.checking.DirtyCheck
 import grails.persistence.Entity
+import groovy.transform.CompileStatic
 import org.grails.validation.ConstraintEvalUtils
 import spock.lang.Issue
 import spock.lang.Specification
@@ -55,6 +57,80 @@ class DefaultASTDatabindingHelperDomainClassSpecialPropertiesSpec extends
         obj.dateCreated == now
         obj.lastUpdated == now
     }
+
+    @Issue('https://github.com/apache/grails-core/issues/15681')
+    void 'Test id and version are not bound when a domain class extends a @DirtyCheck abstract base class'() {
+        when: 'a domain class that extends a @DirtyCheck abstract base is bound with id and version'
+        def obj = new MyDirtyCheckedDomain(id: 99L, version: 7L, name: 'Grace')
+
+        then: 'the regular property is bound but id and version are not'
+        obj.name == 'Grace'
+        obj.id == null
+        obj.version == null
+    }
+
+    @Issue('https://github.com/apache/grails-core/issues/15681')
+    void 'Test dateCreated and lastUpdated declared on a @DirtyCheck abstract base are not bound in the domain subclass'() {
+        when: 'a domain subclass inheriting timestamp properties from a @DirtyCheck base is bound'
+        def now = new Date()
+        def obj = new DomainWithInheritedTimestamps(id: 1L, version: 2L, name: 'Alan', title: 'Mathematician', dateCreated: now, lastUpdated: now)
+
+        then: 'regular inherited and declared properties are bound'
+        obj.name == 'Alan'
+        obj.title == 'Mathematician'
+
+        and: 'the default-excluded special properties are not bound regardless of where they are declared'
+        obj.id == null
+        obj.version == null
+        obj.dateCreated == null
+        obj.lastUpdated == null
+    }
+
+    @Issue('https://github.com/apache/grails-core/issues/15681')
+    void 'Test a regular property declared on a @DirtyCheck abstract base is still bound in the domain subclass'() {
+        when: 'only regular properties are bound'
+        def obj = new DomainWithInheritedTimestamps(name: 'Grace', title: 'Admiral')
+
+        then: 'all regular properties are bound and the special properties remain null'
+        obj.name == 'Grace'
+        obj.title == 'Admiral'
+        obj.id == null
+        obj.version == null
+    }
+
+    @Issue('https://github.com/apache/grails-core/issues/15681')
+    void 'Test explicit bindable:true on inherited special properties re-enables binding without affecting id and version'() {
+        when: 'a domain subclass declares an explicit bindable:true constraint for inherited timestamp properties'
+        def now = new Date()
+        def obj = new DomainWithInheritedBindableTimestamps(id: 5L, version: 3L, name: 'Edsger', title: 'Scientist', dateCreated: now, lastUpdated: now)
+
+        then: 'the explicitly bindable timestamps are bound'
+        obj.dateCreated == now
+        obj.lastUpdated == now
+
+        and: 'regular properties are bound'
+        obj.name == 'Edsger'
+        obj.title == 'Scientist'
+
+        and: 'id and version remain excluded as there is no explicit override for them'
+        obj.id == null
+        obj.version == null
+    }
+
+    @Issue('https://github.com/apache/grails-core/issues/15681')
+    void 'Test id and version are not bound across multiple levels of abstract inheritance'() {
+        when: 'a domain class extends a plain abstract class which extends a @DirtyCheck abstract base'
+        def obj = new DomainExtendingMultiLevelHierarchy(id: 42L, version: 9L, grandparentName: 'g', parentName: 'p', childName: 'c')
+
+        then: 'all regular properties throughout the hierarchy are bound'
+        obj.grandparentName == 'g'
+        obj.parentName == 'p'
+        obj.childName == 'c'
+
+        and: 'id and version are not bound even though they are injected several levels up the hierarchy'
+        obj.id == null
+        obj.version == null
+    }
 }
 
 @Entity
@@ -73,3 +149,55 @@ class SomeDomainClassWithExplicitBindableRules {
         lastUpdated bindable: true
     }
 }
+
+@DirtyCheck
+@CompileStatic
+abstract class AbstractDirtyCheckedBase {
+    String name
+}
+
+@Entity
+class MyDirtyCheckedDomain extends AbstractDirtyCheckedBase {
+    static constraints = {
+        name nullable: false
+    }
+}
+
+@DirtyCheck
+@CompileStatic
+abstract class AbstractDirtyCheckedBaseWithTimestamps {
+    String name
+    Date dateCreated
+    Date lastUpdated
+}
+
+@Entity
+class DomainWithInheritedTimestamps extends AbstractDirtyCheckedBaseWithTimestamps {
+    String title
+}
+
+@Entity
+class DomainWithInheritedBindableTimestamps extends AbstractDirtyCheckedBaseWithTimestamps {
+    String title
+
+    static constraints = {
+        dateCreated bindable: true
+        lastUpdated bindable: true
+    }
+}
+
+@DirtyCheck
+@CompileStatic
+abstract class AbstractDirtyCheckedGrandparent {
+    String grandparentName
+}
+
+@CompileStatic
+abstract class AbstractPlainParent extends AbstractDirtyCheckedGrandparent {
+    String parentName
+}
+
+@Entity
+class DomainExtendingMultiLevelHierarchy extends AbstractPlainParent {
+    String childName
+}