jdaugherty commented on code in PR #15568:
URL: https://github.com/apache/grails-core/pull/15568#discussion_r3221568446
##########
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GrailsCodeStylePlugin.groovy:
##########
@@ -32,33 +37,405 @@ import org.gradle.api.plugins.quality.CheckstylePlugin
import org.gradle.api.plugins.quality.CodeNarc
import org.gradle.api.plugins.quality.CodeNarcExtension
import org.gradle.api.plugins.quality.CodeNarcPlugin
-
-@CompileStatic
+import org.gradle.api.plugins.quality.Pmd
+import org.gradle.api.plugins.quality.PmdExtension
+import org.gradle.api.plugins.quality.PmdPlugin
+
+import com.github.spotbugs.snom.Confidence
+import com.github.spotbugs.snom.Effort
+import com.github.spotbugs.snom.SpotBugsExtension
+import com.github.spotbugs.snom.SpotBugsPlugin
+import com.github.spotbugs.snom.SpotBugsTask
+import org.gradle.api.tasks.testing.Test
+import org.gradle.testing.jacoco.plugins.JacocoPlugin
+import org.gradle.testing.jacoco.plugins.JacocoPluginExtension
+import org.gradle.testing.jacoco.tasks.JacocoReport
+
+/**
+ * Convention plugin for Grails code style enforcement.
+ */
+@CompileDynamic
class GrailsCodeStylePlugin implements Plugin<Project> {
static String CHECKSTYLE_DIR_PROPERTY = 'grails.codestyle.dir.checkstyle'
+ static String CHECKSTYLE_ENABLED_PROPERTY =
'grails.codestyle.enabled.checkstyle'
static String CHECKSTYLE_CONFIG_FILE_NAME = 'checkstyle.xml'
static String CHECKSTYLE_SUPPRESSION_CONFIG_FILE_NAME =
'checkstyle-suppressions.xml'
+ static String PMD_DIR_PROPERTY = 'grails.codestyle.dir.pmd'
+ static String PMD_ENABLED_PROPERTY = 'grails.codestyle.enabled.pmd'
+ static String PMD_CONFIG_FILE_NAME = 'pmd.xml'
+
static String CODENARC_DIR_PROPERTY = 'grails.codestyle.dir.codenarc'
+ static String CODENARC_ENABLED_PROPERTY =
'grails.codestyle.enabled.codenarc'
static String CODENARC_CONFIG_FILE_NAME = 'codenarc.groovy'
+ static String CODENARC_FIX_PROPERTY = 'grails.codestyle.codenarc.fix'
+
+ static String SPOTBUGS_ENABLED_PROPERTY =
'grails.codestyle.enabled.spotbugs'
+
+ static String JACOCO_ENABLED_PROPERTY = 'grails.codestyle.enabled.jacoco'
+
+ static String IGNORE_FAILURES_PROPERTY = 'grails.codestyle.ignoreFailures'
+
+ static String TEST_STYLING_PROPERTY = 'grails.codestyle.enabled.tests'
+
static String BASE_RESOURCE_PATH =
'/META-INF/org.apache.grails.buildsrc.codestyle'
@Override
void apply(Project project) {
initExtension(project)
configureCodeStyle(project)
- doNotApplyStylingToTests(project)
+ configureAggregation(project)
+
+ boolean jacocoEnabled = GradleUtils.lookupProperty(project,
JACOCO_ENABLED_PROPERTY, false)
+ if (jacocoEnabled) {
+ configureJacoco(project)
+ if (project == project.rootProject) {
+ project.logger.info("JaCoCo enabled globally, applying to
subprojects")
+ project.subprojects.each { subproject ->
+ subproject.pluginManager.withPlugin('java') {
+ configureJacoco(subproject)
+ }
+ subproject.pluginManager.withPlugin('groovy') {
+ configureJacoco(subproject)
+ }
+ }
+ }
+ }
+ }
+
+ static void configureJacoco(Project project) {
+ project.logger.info("Configuring JaCoCo for project: ${project.name}")
+ project.pluginManager.apply(JacocoPlugin)
+
+ project.extensions.configure(JacocoPluginExtension) {
+ it.toolVersion = "0.8.14"
+ }
+
+ project.tasks.withType(Test).configureEach {
+ it.finalizedBy 'jacocoTestReport'
+ }
+
+ project.tasks.withType(JacocoReport).configureEach {
+ it.dependsOn project.tasks.withType(Test)
+ it.reports {
+ it.xml.required = true
+ it.html.required = true
+ it.csv.required = true
+ }
+ }
+ }
+
+ private static void configureAggregation(Project project) {
+ Project root = project.rootProject
+ if (root.tasks.findByName('aggregateStyleViolations')) {
+ return
+ }
+
+ root.tasks.register('aggregateStyleViolations') { task ->
+ task.group = 'verification'
+ task.description = 'Aggregates all code style violations into
separate reports'
+
+ boolean checkTests = GradleUtils.lookupProperty(project,
TEST_STYLING_PROPERTY, false)
+ boolean jacocoEnabled = GradleUtils.lookupProperty(project,
JACOCO_ENABLED_PROPERTY, false)
+
+ // Dependencies: all check tasks in all subprojects
+ root.subprojects.each { subproject ->
+ // CodeNarc (prod only unless test styling is enabled)
+ task.dependsOn(subproject.tasks.withType(CodeNarc).matching {
t ->
+ checkTests || (!t.name.toLowerCase().contains('test') &&
!t.name.toLowerCase().contains('integrationtest'))
+ })
+
+ // Checkstyle (prod only unless test styling is enabled)
+ task.dependsOn(subproject.tasks.withType(Checkstyle).matching
{ t ->
+ checkTests || (!t.name.toLowerCase().contains('test') &&
!t.name.toLowerCase().contains('integrationtest'))
+ })
+
+ if (GradleUtils.lookupProperty(project, PMD_ENABLED_PROPERTY,
false)) {
+ task.dependsOn(subproject.tasks.withType(Pmd).matching { t
->
+ checkTests || (!t.name.toLowerCase().contains('test')
&& !t.name.toLowerCase().contains('integrationtest'))
+ })
+ }
+
+ if (GradleUtils.lookupProperty(project,
SPOTBUGS_ENABLED_PROPERTY, false)) {
+
task.dependsOn(subproject.tasks.withType(SpotBugsTask).matching { t ->
+ checkTests || (!t.name.toLowerCase().contains('test')
&& !t.name.toLowerCase().contains('integrationtest'))
+ })
+ }
+
+ if (jacocoEnabled) {
+ task.dependsOn(subproject.tasks.withType(JacocoReport))
+ }
+ }
+
+ def reportsDir =
project.extensions.getByType(GrailsCodeStyleExtension).reportsDirectory
+ task.inputs.dir(reportsDir).optional()
+
task.outputs.file(root.layout.projectDirectory.file('CODENARC_VIOLATIONS.md'))
Review Comment:
I'm ok with making reports easier to read, but gradle has a built in way to
store reports - under build/reports. Why are we putting these at the root?
##########
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/CompilePlugin.groovy:
##########
@@ -58,8 +59,13 @@ class CompilePlugin implements Plugin<Project> {
}
private static void configureJavaVersion(Project project) {
+ Integer javaVersion = lookupPropertyByType(project, 'javaVersion',
Integer)
project.tasks.withType(JavaCompile).configureEach {
- it.options.release.set(lookupPropertyByType(project,
'javaVersion', Integer))
+ it.options.release.set(javaVersion)
+ }
+ project.tasks.withType(AbstractCompile).configureEach {
Review Comment:
Why are the abstractCompile changes needed? GSP & other tasks extend from
abstract compile. Isn't source/target specific to the compile implementation?
##########
build.gradle:
##########
@@ -25,6 +30,13 @@ import javax.inject.Inject
import org.apache.tools.ant.taskdefs.condition.Os
ext {
+ if (file('local.properties').exists()) {
Review Comment:
You can already overwrite any gradle property by using environment
variables. i.e. `groovyVersion` would be overridden by the environment
variable `ORG_GRADLE_groovyVersion`. I'm ok adding this but it seems like we
should modify the shared property plugin for this instead.
##########
grails-data-hibernate5/core/src/test/groovy/grails/gorm/specs/HibernateGormDatastoreSpec.groovy:
##########
@@ -0,0 +1,158 @@
+/*
+ * 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.gorm.specs
+
+import org.apache.grails.data.hibernate5.core.GrailsDataHibernate5TckManager
+import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
+import org.grails.datastore.mapping.model.PersistentEntity
+import org.grails.orm.hibernate.AbstractHibernateSession
+import org.grails.orm.hibernate.HibernateDatastore
+import org.grails.orm.hibernate.cfg.GrailsDomainBinder
+import org.grails.orm.hibernate.cfg.HibernateMappingContext
+import org.grails.orm.hibernate.cfg.HibernatePersistentEntity
+import org.grails.orm.hibernate.query.HibernateQuery
+
+import org.hibernate.boot.MetadataSources
+import org.hibernate.boot.internal.BootstrapContextImpl
+import org.hibernate.boot.internal.InFlightMetadataCollectorImpl
+import org.hibernate.boot.internal.MetadataBuilderImpl
+import org.hibernate.boot.registry.BootstrapServiceRegistry
+import org.hibernate.boot.registry.StandardServiceRegistryBuilder
+import org.hibernate.boot.registry.classloading.spi.ClassLoaderService
+import org.hibernate.dialect.H2Dialect
+import org.hibernate.internal.SessionFactoryImpl
+import org.hibernate.service.spi.ServiceRegistryImplementor
+import org.hibernate.boot.spi.MetadataContributor
+
+/**
+ * The original GormDataStoreSpec destroyed the setup
+ * between tests instead of at the end of all tests
+ * It also wqs default configured for H2 which
Review Comment:
typo
##########
grails-data-hibernate7/README.md:
##########
@@ -0,0 +1,114 @@
+<!--
+ 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.
+-->
Review Comment:
Double license header
##########
grails-data-hibernate5/core/src/test/groovy/grails/gorm/specs/multitenancy/MultiTenancyBidirectionalManyToManySpec.groovy:
##########
@@ -58,14 +57,17 @@ class MultiTenancyBidirectionalManyToManySpec extends
Specification {
@Shared @AutoCleanup HibernateDatastore datastore
-
- void setup() {
+ void setupSpec() {
System.setProperty(SystemPropertyTenantResolver.PROPERTY_NAME, "oci")
datastore = new
HibernateDatastore(DatastoreUtils.createPropertyResolver(config),
getClass().getPackage() )
departmentService = datastore.getService(DepartmentService)
userService = datastore.getService(UserService)
}
+ void setup() {
+ System.setProperty(SystemPropertyTenantResolver.PROPERTY_NAME, "oci")
Review Comment:
We should use `@RestoreSystemProperties` instead
##########
grails-test-examples/hibernate7/grails-hibernate/src/integration-test/groovy/functional/tests/BookControllerSpec.groovy:
##########
@@ -24,7 +24,7 @@ import functional.tests.pages.BookShowPage
import grails.plugin.geb.ContainerGebSpec
import grails.testing.mixin.integration.Integration
-@Integration
+@Integration(applicationClass = Application)
Review Comment:
Isn't this redundant? Can we revert this?
##########
grails-test-examples/hibernate7/grails-hibernate/src/test/groovy/functional/tests/BookControllerUnitSpec.groovy:
##########
@@ -97,7 +100,7 @@ class BookControllerUnitSpec extends HibernateSpec
implements ControllerUnitTest
when:"A domain instance is passed to the show action"
populateValidParams(params)
- def book = new Book(params.subMap(['title']))
+ def book = new Book(params)
Review Comment:
These tests were binding a specific value before and now are binding more
than that. What new field was required to be set?
##########
grails-test-examples/gorm/src/integration-test/groovy/gorm/TransactionalWhereQueryVariableScopeSpec.groovy:
##########
@@ -45,8 +45,8 @@ class TransactionalWhereQueryVariableScopeSpec extends
Specification {
WhereQueryVariableScopeService whereQueryVariableScopeService
def setup() {
- Book.executeUpdate('delete from Book')
- Author.executeUpdate('delete from Author')
+ Book.executeUpdate('delete from Book', [:])
Review Comment:
Is the empty map necessary?
##########
grails-test-examples/gorm/src/integration-test/groovy/gorm/GormDataServicesSpec.groovy:
##########
@@ -48,8 +48,8 @@ class GormDataServicesSpec extends Specification {
def setup() {
// Clean up and create fresh test data
- Book.executeUpdate('delete from Book')
- Author.executeUpdate('delete from Author')
+ Book.executeUpdate('delete from Book', [:])
Review Comment:
Is the empty map necessary? (repeated for the below)
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/OptimisticLockingSpec.groovy:
##########
@@ -19,24 +19,31 @@
package org.apache.grails.data.testing.tck.tests
import spock.lang.IgnoreIf
+import spock.lang.Requires
import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
import org.apache.grails.data.testing.tck.domains.OptLockNotVersioned
import org.apache.grails.data.testing.tck.domains.OptLockVersioned
+import org.springframework.dao.OptimisticLockingFailureException
+
import org.grails.datastore.mapping.core.OptimisticLockingException
/**
* @author Burt Beckwith
*/
+@Requires({ System.getProperty('hibernate5.gorm.suite') == 'true' ||
System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
This seems like a mistake - why are we limiting this now when previously it
wasn't?
##########
gradle/test-config.gradle:
##########
@@ -81,6 +89,13 @@ tasks.withType(Test).configureEach {
System.out.print('.')
System.out.flush()
}
+ // Bridge Geb-specific properties (e.g. from local.properties) to the Test
JVM
+ // to allow local overrides of Docker images and other settings.
+ project.ext.properties.each { key, value ->
Review Comment:
The shared property plugin would handle this if we move the logic there.
##########
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GrailsCodeStylePlugin.groovy:
##########
@@ -32,33 +37,405 @@ import org.gradle.api.plugins.quality.CheckstylePlugin
import org.gradle.api.plugins.quality.CodeNarc
import org.gradle.api.plugins.quality.CodeNarcExtension
import org.gradle.api.plugins.quality.CodeNarcPlugin
-
-@CompileStatic
+import org.gradle.api.plugins.quality.Pmd
+import org.gradle.api.plugins.quality.PmdExtension
+import org.gradle.api.plugins.quality.PmdPlugin
+
+import com.github.spotbugs.snom.Confidence
+import com.github.spotbugs.snom.Effort
+import com.github.spotbugs.snom.SpotBugsExtension
+import com.github.spotbugs.snom.SpotBugsPlugin
+import com.github.spotbugs.snom.SpotBugsTask
+import org.gradle.api.tasks.testing.Test
+import org.gradle.testing.jacoco.plugins.JacocoPlugin
+import org.gradle.testing.jacoco.plugins.JacocoPluginExtension
+import org.gradle.testing.jacoco.tasks.JacocoReport
+
+/**
+ * Convention plugin for Grails code style enforcement.
+ */
+@CompileDynamic
class GrailsCodeStylePlugin implements Plugin<Project> {
Review Comment:
The codestyle plugin was voted on to be moved to a separate repository, but
the aggregation logic is coupled to it. Codecoverage does not relate to
codestyle in anyway. Can we move this logic to it's own plugin?
##########
.gitignore:
##########
@@ -63,3 +64,9 @@ tmp/
!etc/bin
etc/bin/results
.vscode/
+STATE_SNAPSHOT.xml
Review Comment:
I agree with the local-* gradle files, but for the text & markdown files can
we create a dedicated directory for this stuff and just merge that root
directory instead?
##########
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GrailsTestPlugin.groovy:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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 org.apache.grails.buildsrc
+
+import java.time.LocalDateTime
+import java.time.format.DateTimeFormatter
+import groovy.xml.XmlSlurper
+import org.gradle.api.Plugin
+import org.gradle.api.Project
+import org.gradle.api.tasks.testing.Test
+import groovy.transform.CompileDynamic
+
+class GrailsTestPlugin implements Plugin<Project> {
Review Comment:
This is poorly named. It seems to be creating aggregate test reports.
##########
build-logic/plugins/src/main/groovy/org/apache/grails/buildsrc/GrailsTestPlugin.groovy:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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 org.apache.grails.buildsrc
+
+import java.time.LocalDateTime
+import java.time.format.DateTimeFormatter
+import groovy.xml.XmlSlurper
+import org.gradle.api.Plugin
+import org.gradle.api.Project
+import org.gradle.api.tasks.testing.Test
+import groovy.transform.CompileDynamic
+
+class GrailsTestPlugin implements Plugin<Project> {
+
+ @Override
+ void apply(Project project) {
+ if (project != project.rootProject) {
+ return
+ }
+
+ project.tasks.register('aggregateTestFailures') { task ->
Review Comment:
Gradle has a built in way to do test aggregation, why are we reinventing the
wheel here?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/FindByMethodSpec.groovy:
##########
@@ -18,16 +18,32 @@
*/
package org.apache.grails.data.testing.tck.tests
-import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
-import org.apache.grails.data.testing.tck.domains.Book
+import spock.lang.Requires
+
+import org.apache.grails.data.testing.tck.domains.Book as TckBook
import org.apache.grails.data.testing.tck.domains.Highway
import org.apache.grails.data.testing.tck.domains.Person
+import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
+import org.grails.datastore.mapping.core.exceptions.ConfigurationException
+import spock.lang.Unroll
/**
+ * TCK Spec for Dynamic Finders.
+ *
* @author graemerocher
*/
class FindByMethodSpec extends GrailsDataTckSpec {
+ @Override
+ void setupSpec() {
+ manager.addAllDomainClasses([Person, TckBook, Highway])
+ }
+
+ @Requires({
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/transaction/HibernateJtaTransactionManagerAdapter.java:
##########
@@ -1,235 +0,0 @@
-/*
- * 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 org.grails.orm.hibernate.transaction;
-
-import javax.transaction.xa.XAResource;
-
-import jakarta.transaction.RollbackException;
-import jakarta.transaction.Status;
-import jakarta.transaction.Synchronization;
-import jakarta.transaction.SystemException;
-import jakarta.transaction.Transaction;
-import jakarta.transaction.TransactionManager;
-
-import org.springframework.transaction.PlatformTransactionManager;
-import org.springframework.transaction.TransactionDefinition;
-import org.springframework.transaction.TransactionStatus;
-import org.springframework.transaction.support.DefaultTransactionDefinition;
-import org.springframework.transaction.support.TransactionSynchronization;
-import
org.springframework.transaction.support.TransactionSynchronizationManager;
-
-/**
- * Adapter for adding transaction controlling hooks for supporting
- * Hibernate's org.hibernate.engine.transaction.Isolater class's interaction
with transactions
- *
- * This is required when there is no real JTA transaction manager in use and
Spring's
- * {@link org.springframework.jdbc.datasource.TransactionAwareDataSourceProxy}
is used.
- *
- * Without this solution, using Hibernate's TableGenerator identity strategies
will fail to support transactions.
- * The id generator will commit the current transaction and break
transactional behaviour.
- *
- * The javadoc of Hibernate's {@code TableHiLoGenerator} states this. However
this isn't mentioned in the javadocs of other TableGenerators.
- *
- * @author Lari Hotari
- */
-public class HibernateJtaTransactionManagerAdapter implements
TransactionManager {
Review Comment:
Can you help me understand why this was removed?
##########
grails-data-hibernate5/core/src/test/groovy/grails/gorm/specs/UniqueConstraintHibernateSpec.groovy:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.gorm.specs
+
+import grails.gorm.annotation.Entity
+import org.apache.grails.data.testing.tck.domains.GroupWithin
+import org.apache.grails.data.testing.tck.domains.UniqueGroup
+import org.grails.datastore.gorm.GormEntity
+import org.grails.orm.hibernate.HibernateDatastore
+import org.springframework.transaction.PlatformTransactionManager
+import spock.lang.AutoCleanup
+import spock.lang.Ignore
+import spock.lang.Shared
+import spock.lang.Specification
+
+/**
+ * Tests the unique constraint
+ */
+/**
+ *
+ * NOTE: This test is disabled because in order for the test suite to run
quickly we need to run each test in a transaction.
Review Comment:
We added `@DatabaseCleanup` so you can run the test without a transaction.
You'll have to create a session to save data though. I don't think we should
disable these
##########
grails-data-hibernate5/core/src/test/groovy/grails/gorm/specs/UniqueConstraintHibernateSpec.groovy:
##########
@@ -0,0 +1,146 @@
+/*
+ * 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.gorm.specs
+
+import grails.gorm.annotation.Entity
+import org.apache.grails.data.testing.tck.domains.GroupWithin
+import org.apache.grails.data.testing.tck.domains.UniqueGroup
+import org.grails.datastore.gorm.GormEntity
+import org.grails.orm.hibernate.HibernateDatastore
+import org.springframework.transaction.PlatformTransactionManager
+import spock.lang.AutoCleanup
+import spock.lang.Ignore
+import spock.lang.Shared
+import spock.lang.Specification
+
+/**
Review Comment:
Can we remove the double comments and collapse into one?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/FirstAndLastMethodSpec.groovy:
##########
@@ -168,10 +168,10 @@ class FirstAndLastMethodSpec extends GrailsDataTckSpec {
)
void "Test first and last method with composite key"() {
given:
- assert new PersonWithCompositeKey(firstName: 'Steve', lastName:
'Harris', age: 56).save()
- assert new PersonWithCompositeKey(firstName: 'Dave', lastName:
'Murray', age: 54).save()
- assert new PersonWithCompositeKey(firstName: 'Adrian', lastName:
'Smith', age: 55).save()
- assert new PersonWithCompositeKey(firstName: 'Bruce', lastName:
'Dickinson', age: 53).save()
+ assert new PersonWithCompositeKey(firstName: 'Steve', lastName:
'Harris', age: 56).save(failOnError: true)
Review Comment:
Did the default for this app get changed? failOnError is the default but
it's being overridden here?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/DomainEventsSpec.groovy:
##########
@@ -155,6 +159,7 @@ class DomainEventsSpec extends GrailsDataTckSpec {
1 == PersonEvent.STORE.afterDelete
}
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-data-hibernate5/core/src/test/groovy/grails/gorm/specs/dirtychecking/HibernateDirtyCheckingSpec.groovy:
##########
@@ -77,6 +77,7 @@ class HibernateDirtyCheckingSpec extends Specification {
when: 'the name is changed'
person.address.street = "New Town"
+ person.markDirty('address')
Review Comment:
Why are you forcing the mark dirty? Isn't this a bug?
##########
grails-datamapping-core/src/main/groovy/grails/gorm/DetachedCriteria.groovy:
##########
@@ -514,24 +518,8 @@ class DetachedCriteria<T> extends
AbstractDetachedCriteria<T> implements GormOpe
* @return The count
*/
Number count(Map args = Collections.emptyMap(),
@DelegatesTo(DetachedCriteria) Closure additionalCriteria = null) {
- if (!projections.isEmpty()) {
- // When user-defined projections exist (e.g. groupProperty +
count),
- // a simple count() projection returns incorrect results because it
- // appends to the existing projections rather than replacing them.
- // Fall back to counting the grouped result rows.
- // This will be resolved properly in Grails 8 with Hibernate 7's
- // JpaSelectCriteria.from(Subquery) support for derived tables.
- log.warn('DetachedCriteria.count() with user-defined projections
cannot use a SQL count query ' +
Review Comment:
Isn't this still a bug in hibernate 5? Did we just push this down?
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/OrderBySpec.groovy:
##########
@@ -42,6 +48,9 @@ class OrderBySpec extends
GrailsDataTckSpec<GrailsDataCoreTckManager> {
45 == result.age
}
+ @Requires({ System.getProperty('hibernate5.gorm.suite') == 'true' ||
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-data-hibernate5/core/src/test/groovy/org/grails/orm/hibernate/connections/SchemaMultiTenantSpec.groovy:
##########
@@ -57,7 +58,13 @@ class SchemaMultiTenantSpec extends Specification {
'hibernate.hbm2ddl.auto': 'create',
]
- HibernateDatastore datastore = new
HibernateDatastore(DatastoreUtils.createPropertyResolver(config),
SingleTenantAuthor )
+ datastore = new
HibernateDatastore(DatastoreUtils.createPropertyResolver(config),
SingleTenantAuthor )
+ }
+
+ void "Test a database per tenant multi tenancy"() {
+ given:"A configuration for multiple data sources"
+ System.setProperty(SystemPropertyTenantResolver.PROPERTY_NAME, "")
Review Comment:
Adopt `@RestoreSystemProperties`?
##########
grails-data-hibernate5/core/src/test/groovy/org/grails/orm/hibernate/connections/SingleTenantSpec.groovy:
##########
@@ -59,7 +62,12 @@ class SingleTenantSpec extends Specification {
'dataSources.moreBooks':[url:"jdbc:h2:mem:moreBooks;LOCK_TIMEOUT=10000"]
]
- HibernateDatastore datastore = new
HibernateDatastore(DatastoreUtils.createPropertyResolver(config),Book,
SingleTenantAuthor )
+ datastore = new
HibernateDatastore(DatastoreUtils.createPropertyResolver(config),Book,
SingleTenantAuthor )
+ }
+
+ void "Test a database per tenant multi tenancy"() {
+ given:"A configuration for multiple data sources"
+ System.setProperty(SystemPropertyTenantResolver.PROPERTY_NAME, "")
Review Comment:
Adopt `@RestoreSystemProperties` for this method as well?
##########
grails-data-hibernate5/grails-plugin/src/main/groovy/grails/orm/bootstrap/HibernateDatastoreSpringInitializer.groovy:
##########
@@ -194,7 +194,6 @@ class HibernateDatastoreSpringInitializer extends
AbstractDatastoreInitializer {
}
}
}
- return beanDefinitions
Review Comment:
If we're going to remove the return, we should remove the variable
assignment for clarity too
##########
grails-data-hibernate5/core/src/test/groovy/grails/gorm/specs/multitenancy/MultiTenancyUnidirectionalOneToManySpec.groovy:
##########
@@ -48,7 +51,13 @@ class MultiTenancyUnidirectionalOneToManySpec extends
Specification {
'hibernate.hbm2ddl.auto' : 'create',
]
- HibernateDatastore datastore = new
HibernateDatastore(DatastoreUtils.createPropertyResolver(config),
getClass().getPackage())
+ datastore = new
HibernateDatastore(DatastoreUtils.createPropertyResolver(config),
getClass().getPackage())
+ }
+
+ @Issue('https://github.com/apache/grails-data-mapping/issues/954')
+ void "test multi-tenancy with unidirectional one-to-many"() {
+ given: "A configuration for schema based multi-tenancy"
+ System.setProperty(SystemPropertyTenantResolver.PROPERTY_NAME, "")
Review Comment:
Why not adopt `@RestoreSystemProperties` instead?
##########
grails-data-hibernate5/core/src/test/resources/simplelogger.properties:
##########
@@ -18,5 +18,6 @@
#
#org.slf4j.simpleLogger.defaultLogLevel=debug
-#org.slf4j.simpleLogger.log.org.hibernate=trace
-#org.slf4j.simpleLogger.log.org.hibernate.SQL=debug
\ No newline at end of file
+org.slf4j.simpleLogger.log.org.hibernate=trace
Review Comment:
We can comment these back out to reduce output, yes?
##########
grails-data-hibernate7/AGENTS.md:
##########
@@ -0,0 +1,232 @@
+<!--
+ 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.
+-->
+<!--
Review Comment:
We do not need a double header , let's remov ethe long form?
##########
grails-data-hibernate7/AGENTS.md:
##########
@@ -0,0 +1,232 @@
+<!--
+ 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.
+-->
+<!--
+SPDX-License-Identifier: Apache-2.0
+
+Licensed 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.
+-->
+
+# HIBERNATE7-UPGRADE-PROGRESS.md
+
+## Completed: GrailsPropertyBinder Simplification
+
+**Objective:** Refactor the `GrailsPropertyBinder` class to consolidate the
binder application logic into a single, unified conditional structure, reducing
redundancy and improving code readability.
+
+**Status: COMPLETED**
+
+The `bindProperty` method in `GrailsPropertyBinder.java` has been successfully
refactored. The core binder application logic is now contained within a single
primary conditional block that dispatches to specific binders based on the GORM
property type.
+
+**Key Changes:**
+- **Consolidated Dispatcher:** Introduced a single `if-else if` chain in
`GrailsPropertyBinder.bindProperty` that returns a Hibernate `Value`.
+- **Centralized Property Creation:** The creation and addition of the
Hibernate `Property` have been moved to the callers (e.g.,
`ClassPropertiesBinder`, `ComponentUpdater`, `CompositeIdBinder`) using the
`PropertyFromValueCreator` utility. This ensures a single, unified entry point
for property creation across different binding scenarios.
+- **Redundancy Removed:** Replaced scattered `createProperty` and
`addProperty` calls with a consistent pattern, significantly improving
maintainability.
+
+## GrailsDomainBinder Analysis
+
+**Objective:** Document the core components and dependencies used by
`GrailsDomainBinder` during the Hibernate 7 mapping process.
+
+`GrailsDomainBinder` is the central entry point for binding Grails domain
classes to the Hibernate meta-model. It coordinates various specialized binder
classes to handle entities, properties, identifiers, and collections.
+
+### Core Dependencies (org.grails.orm.hibernate.cfg.*)
+
+#### Logical Domain Mapping (cfg package)
+* **GrailsHibernatePersistentEntity**: Core interface extending
`PersistentEntity` with Hibernate-specific mapping capabilities
(discriminators, data sources, etc.).
+* **HibernatePersistentEntity**: Default implementation of
`GrailsHibernatePersistentEntity`.
+* **Mapping**: Groovy DSL representation of GORM mapping configurations.
+* **HibernateMappingContext**: Specialized `MappingContext` for Hibernate.
+* **HibernateMappingContextConfiguration**: Coordinates the creation of
Hibernate `Metadata` and `SessionFactory` using GORM entities.
+* **PersistentEntityNamingStrategy**: Strategy interface for resolving
physical names (tables, columns).
+* **NamingStrategyWrapper**: Wraps Hibernate's `PhysicalNamingStrategy` for
GORM usage.
+* **MappingCacheHolder**: Singleton used to cache `Mapping` instances for
entities to avoid repeated DSL evaluation.
+
+#### Property & Value Binding (cfg.domainbinding package)
+* **GrailsPropertyBinder**: Main coordinator for binding individual
persistent properties to Hibernate `Value` objects.
+* **PropertyBinder**: Binds Hibernate `Property` objects, handling
updateable/insertable flags.
+* **SimpleValueBinder**: Binds simple types (String, Integer, etc.) to
Hibernate `BasicValue`.
+* **SimpleValueColumnBinder**: Handles the binding of columns to
`SimpleValue` instances.
+* **ComponentPropertyBinder**: Specialized binder for GORM embedded
components.
+* **ComponentBinder**: Binds Hibernate `Component` instances.
+* **EnumTypeBinder**: Handles the binding of Java Enums using
`GrailsEnumType`.
+* **OneToOneBinder / ManyToOneBinder**: Handle GORM associations and their
corresponding Hibernate mappings.
+* **ManyToOneValuesBinder**: Specifically handles the `Value` binding for
many-to-one associations.
+* **CollectionBinder**: Handles GORM collections (Set, List, Map) and their
Hibernate `Collection` mappings.
+* **PropertyFromValueCreator**: Utility to create Hibernate `Property`
instances from a `Value`.
+
+#### Identifier & Version Binding (cfg.domainbinding package)
+* **IdentityBinder**: Main coordinator for binding entity identifiers
(simple or composite).
+* **SimpleIdBinder**: Binds simple primary keys.
+* **CompositeIdBinder**: Binds composite primary keys.
+* **BasicValueCreator**: Factory for creating identifier `Value` objects and
their generators.
+* **VersionBinder**: Binds the version property used for optimistic locking.
+* **NaturalIdentifierBinder**: Binds properties marked as `naturalId`.
+
+#### ID Generators (cfg.domainbinding.generator package)
+* **GrailsSequenceWrapper**: Wraps Hibernate 7 generator creation.
+* **GrailsSequenceGeneratorEnum**: Enum mapping Grails generator names to
Hibernate 7 `Generator` implementations.
+* **GrailsIdentityGenerator / GrailsIncrementGenerator /
GrailsNativeGenerator / GrailsSequenceStyleGenerator / GrailsTableGenerator**:
GORM-specific extensions of Hibernate 7 generators.
+
+#### Sub-mapping & Collection Types (cfg.domainbinding.collectionType package)
+* **CollectionHolder**: Context object passed through binders to maintain
collection state.
+* **ListCollectionType / SetCollectionType / MapCollectionType /
BagCollectionType**: Metadata classes defining how different GORM collections
are mapped.
+
+#### Second Pass Binding (cfg.domainbinding.secondpass package)
+* **GrailsSecondPass**: Base interface for binding operations that must
occur after all entities are initially processed.
+* **CollectionSecondPassBinder / ListSecondPassBinder /
MapSecondPassBinder**: Implementations handling the final binding of
associations and collection elements.
+
+#### Miscellaneous Utilities
+* **NamespaceNameExtractor**: Extracts schema and catalog information from
Hibernate metadata.
+* **TableNameFetcher**: Resolves the table name for a given entity using the
naming strategy.
+* **DefaultColumnNameFetcher**: Resolves default column names for properties.
+* **ColumnNameForPropertyAndPathFetcher**: Resolves column names considering
embedded paths.
+* **BackticksRemover**: Utility for handling database identifiers with
quotes. Replaced redundant `BackTigsTrimmer`.
+* **ConfigureDerivedPropertiesConsumer**: Applies `derived` flag to
properties based on mapping.
+* **GrailsHibernateUtil**: General utility methods for Hibernate integration.
+
+### Migration Status Breakdown
+
+#### Main Classes
+
+| Class | Package | Status | Notes |
+| :--- | :--- | :--- | :--- |
+| `GrailsDomainBinder` | `org.grails.orm.hibernate.cfg` | Migrated | Main
entry point for domain binding. Implements `AdditionalMappingContributor`,
`TypeContributor`. |
+| `HibernateMappingContext` | `org.grails.orm.hibernate.cfg` | Migrated | |
+| `GrailsHibernatePersistentEntity` | `org.grails.orm.hibernate.cfg` |
Migrated | |
+| `GrailsHibernatePersistentProperty` | `org.grails.orm.hibernate.cfg` |
Migrated | |
+| `GrailsHibernateUtil` | `org.grails.orm.hibernate.cfg` | Migrated | |
+| `MappingCacheHolder` | `org.grails.orm.hibernate.cfg` | Migrated | |
+| `PersistentEntityNamingStrategy` | `org.grails.orm.hibernate.cfg` | Migrated
| |
+| `NamingStrategyWrapper` | `org.grails.orm.hibernate.cfg.domainbinding` |
Migrated | |
+
+#### Binders (`org.grails.orm.hibernate.cfg.domainbinding`)
+
+| Class | Status | Notes |
+| :--- | :--- | :--- |
+| `ClassBinder` | Migrated | Binds `PersistentClass` basic info. |
+| `EnumTypeBinder` | Migrated | |
+| `PropertyFromValueCreator` | Migrated | |
+| `ComponentPropertyBinder` | Migrated | |
+| `GrailsPropertyBinder` | Migrated | Simplified and consolidated. |
+| `CollectionBinder` | Migrated | |
+| `CompositeIdBinder` | Migrated | |
+| `IdentityBinder` | Migrated | |
+| `VersionBinder` | Migrated | |
+| `SimpleValueBinder` | Migrated | |
+| `OneToOneBinder` | Migrated | |
+| `ManyToOneBinder` | Migrated | |
+| `ColumnBinder` | Migrated | |
+| `ColumnConfigToColumnBinder` | Migrated | |
+| `SimpleValueColumnBinder` | Migrated | |
+| `NaturalIdentifierBinder` | Migrated | |
+| `IndexBinder` | Migrated | |
+| `ComponentBinder` | Migrated | |
+| `SimpleIdBinder` | Migrated | |
+| `SimpleValueBinder` | Migrated | |
+
+#### Collection Types
(`org.grails.orm.hibernate.cfg.domainbinding.collectionType`)
+
+| Class | Status | Notes |
+| :--- | :--- | :--- |
+| `CollectionHolder` | Migrated | |
+| `BagCollectionType` | Migrated | |
+| `ListCollectionType` | Migrated | |
+| `MapCollectionType` | Migrated | |
+| `SetCollectionType` | Migrated | |
+| `SortedSetCollectionType` | Migrated | |
+
+#### Second Pass Binders
(`org.grails.orm.hibernate.cfg.domainbinding.secondpass`)
+
+| Class | Status | Notes |
+| :--- | :--- | :--- |
+| `CollectionSecondPassBinder` | Migrated | Unidirectional many-to-many
support implemented. |
+| `GrailsSecondPass` | Migrated | |
+| `ListSecondPass` | Migrated | |
+| `ListSecondPassBinder` | Migrated | |
+| `MapSecondPass` | Migrated | |
+| `MapSecondPassBinder` | Migrated | |
+| `SetSecondPass` | Migrated | |
+
+#### Generators (`org.grails.orm.hibernate.cfg.domainbinding` and `generator`
subpackage)
+
+| Class | Status | Notes |
+| :--- | :--- | :--- |
+| `GrailsIdentityGenerator` | Migrated | |
+| `GrailsIncrementGenerator` | Migrated | Contains reflection hacks for
Hibernate 7, to be removed in Hibernate 8. |
+| `GrailsNativeGenerator` | Migrated | |
+| `GrailsSequenceStyleGenerator` | Migrated | |
+| `GrailsTableGenerator` | Migrated | |
+| `GrailsSequenceGeneratorEnum` | Migrated | In `generator` subpackage. |
+| `GrailsSequenceWrapper` | Migrated | In `generator` subpackage. |
+
+#### Fetchers and Utilities (`org.grails.orm.hibernate.cfg.domainbinding`)
+
+| Class | Status | Notes |
+| :--- | :--- | :--- |
+| `ColumnNameForPropertyAndPathFetcher` | Migrated | |
+| `TableNameFetcher` | Migrated | |
+| `DefaultColumnNameFetcher` | Migrated | |
+| `SimpleValueColumnFetcher` | Migrated | |
+| `CascadeBehaviorFetcher` | Migrated | |
+| `NamespaceNameExtractor` | Migrated | |
+| `ForeignKeyColumnCountCalculator` | Migrated | |
+| `TableForManyCalculator` | Migrated | |
+| `UniqueNameGenerator` | Migrated | |
+| `BackticksRemover` | Migrated | |
+| `BasicValueCreator` | Migrated | |
+
+## Utility Class Refactoring & Mock Compatibility
+
+**Objective:** Modernize utility classes in `domainbinding.util` to use
Hibernate-specific GORM types while maintaining compatibility with Spock mocks.
+
+**Summary of Changes:**
+- **Refactored Utility Classes:** Updated `CreateKeyForProps`,
`TableForManyCalculator`, `DefaultColumnNameFetcher`,
`ConfigureDerivedPropertiesConsumer`, and `NamingStrategyWrapper` to use
`GrailsHibernatePersistentProperty` and `GrailsHibernatePersistentEntity` where
possible.
+- **Mock Compatibility Fixes:** Addressed `ClassCastException` in Spock specs
by:
+ - Reverting public method signatures to use base interfaces
(`PersistentProperty`, `PersistentEntity`) where required by mocks.
+ - Implementing internal safe casting using `instanceof` pattern matching.
+ - Updating test stubs to include `additionalInterfaces:
[GrailsHibernatePersistentProperty]`.
+- **Logic Improvements:**
+ - Updated `getDiscriminatorValue` in `GrailsHibernatePersistentEntity` to
default to `getJavaClass().getSimpleName()` to match GORM conventions and test
expectations.
+ - Fixed `getMultiTenantFilterCondition` to safely handle non-Hibernate
tenantId properties in test environments.
+- **Verification:** Verified that all 1045 tests in
`:grails-data-hibernate7-core` are passing, confirming that the refactorings
and modernizations have not introduced regressions.
+
+## Remaining Known Issues / TODOs
+
+- `GrailsIncrementGenerator`: Reflection hacks for Hibernate 7 (scheduled for
removal in Hibernate 8).
Review Comment:
I assume it's useful to keep this file at this point?
##########
grails-data-hibernate7/ISSUES.md:
##########
@@ -0,0 +1,146 @@
+<!--
Review Comment:
These will be distributed in the end source, shouldn't we have a shared
directory that can be ignored completely for this stuff?
##########
grails-test-examples/hibernate7/grails-schema-per-tenant/src/test/groovy/schemapertenant/SchemaPerTenantSpec.groovy:
##########
@@ -50,6 +51,9 @@ class SchemaPerTenantSpec extends HibernateSpec implements
GrailsUnitTest {
hibernateDatastore.addTenantForSchema("moreBooks")
hibernateDatastore.addTenantForSchema("evenMoreBooks")
}
+ def cleanup() {
Review Comment:
1. Add a new line before cleanup()?
2. Adopt `@RestoreSystemProperties` instead?
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/GrailsHibernateTemplate.java:
##########
@@ -0,0 +1,764 @@
+/*
+ * 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 org.grails.orm.hibernate;
+
+import java.io.Serializable;
+import java.lang.reflect.Proxy;
+import java.sql.Connection;
+import java.sql.SQLException;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Objects;
+
+import javax.sql.DataSource;
+
+import groovy.lang.Closure;
+import org.codehaus.groovy.runtime.DefaultGroovyMethods;
+
+import jakarta.persistence.LockModeType;
+import jakarta.persistence.PersistenceException;
+import jakarta.persistence.criteria.CriteriaBuilder;
+import jakarta.persistence.criteria.CriteriaQuery;
+
+import org.hibernate.FlushMode;
+import org.hibernate.HibernateException;
+import org.hibernate.JDBCException;
+import org.hibernate.LockMode;
+import org.hibernate.Session;
+import org.hibernate.SessionFactory;
+import org.hibernate.engine.jdbc.connections.spi.ConnectionProvider;
+import org.hibernate.engine.spi.SessionFactoryImplementor;
+import org.hibernate.engine.spi.SessionImplementor;
+import org.hibernate.event.spi.EventSource;
+import org.hibernate.exception.GenericJDBCException;
+import org.hibernate.query.Query;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.springframework.dao.DataAccessException;
+import org.springframework.dao.DataAccessResourceFailureException;
+import org.springframework.dao.InvalidDataAccessApiUsageException;
+import org.springframework.jdbc.datasource.ConnectionHolder;
+import org.springframework.jdbc.datasource.DataSourceUtils;
+import org.springframework.jdbc.datasource.TransactionAwareDataSourceProxy;
+import org.springframework.jdbc.support.SQLErrorCodeSQLExceptionTranslator;
+import org.springframework.jdbc.support.SQLExceptionTranslator;
+import org.springframework.transaction.support.TransactionSynchronization;
+import org.springframework.util.Assert;
+
+import org.grails.orm.hibernate.support.hibernate7.DefaultTransactionResources;
+import org.grails.orm.hibernate.support.hibernate7.SessionFactoryUtils;
+import org.grails.orm.hibernate.support.hibernate7.SessionHolder;
+import org.grails.orm.hibernate.support.hibernate7.TransactionResources;
+
+@SuppressWarnings({"PMD.CloseResource", "PMD.DataflowAnomalyAnalysis",
"PMD.CompareObjectsWithEquals", "PMD.EmptyIfStmt"
+})
+public class GrailsHibernateTemplate implements IHibernateTemplate {
+
+ /**
+ * Never flush is a good strategy for read-only units of work.
+ * Hibernate will not track and look
+ * for changes in this case, avoiding any overhead of modification
detection.
+ *
+ * <p>In case of an existing Session, FLUSH_NEVER will turn the flush mode
to NEVER for the scope
+ * of the current operation, resetting the previous flush mode afterwards.
+ *
+ * @see #setFlushMode
+ */
+ public static final int FLUSH_NEVER = 0;
+ /**
+ * Automatic flushing is the default mode for a Hibernate Session. A
session will get flushed on
+ * transaction commit, and on certain find operations that might involve
already modified
+ * instances, but not after each unit of work like with eager flushing.
+ *
+ * <p>In case of an existing Session, FLUSH_AUTO will participate in the
existing flush mode, not
+ * modifying it for the current operation. This in particular means that
this setting will not
+ * modify an existing flush mode NEVER, in contrast to FLUSH_EAGER.
+ *
+ * @see #setFlushMode
+ */
+ public static final int FLUSH_AUTO = 1;
+ /**
+ * Eager flushing leads to immediate synchronization with the database,
even if in a transaction.
+ * This causes inconsistencies to show up and throw a respective exception
immediately, and JDBC
+ * access code that participates in the same transaction will see the
changes as the database is
+ * already aware of them then. But the drawbacks are:
+ *
+ * <ul>
+ * <li>additional communication roundtrips with the database, instead of
a single batch at
+ * transaction commit;
+ * <li>the fact that an actual database rollback is needed if the
Hibernate transaction rolls
+ * back (due to already submitted SQL statements).
+ * </ul>
+ *
+ * <p>In case of an existing Session, FLUSH_EAGER will turn the flush mode
to AUTO for the scope
+ * of the current operation and issue a flush at the end, resetting the
previous flush mode
+ * afterwards.
+ *
+ * @see #setFlushMode
+ */
+ public static final int FLUSH_EAGER = 2;
+ /**
+ * Flushing at commit only is intended for units of work where no
intermediate flushing is
+ * desired, not even for find operations that might involve already
modified instances.
+ *
+ * <p>In case of an existing Session, FLUSH_COMMIT will turn the flush
mode to COMMIT for the
+ * scope of the current operation, resetting the previous flush mode
afterwards. The only
+ * exception is an existing flush mode NEVER, which will not be modified
through this setting.
+ *
+ * @see #setFlushMode
+ */
+ public static final int FLUSH_COMMIT = 3;
+ /**
+ * Flushing before every query statement is rarely necessary. It is only
available for special
+ * needs.
+ *
+ * <p>In case of an existing Session, FLUSH_ALWAYS will turn the flush
mode to ALWAYS for the
+ * scope of the current operation, resetting the previous flush mode
afterwards.
+ *
+ * @see #setFlushMode
+ */
+ public static final int FLUSH_ALWAYS = 4;
+
+ private static final Logger LOG =
LoggerFactory.getLogger(GrailsHibernateTemplate.class);
+ protected boolean exposeNativeSession = true;
+ protected boolean cacheQueries = false;
+ protected SessionFactory sessionFactory;
+ protected DataSource dataSource = null;
+ protected SQLExceptionTranslator jdbcExceptionTranslator;
+ protected int flushMode = FLUSH_AUTO;
+ private boolean osivReadOnly;
+ private boolean passReadOnlyToHibernate = false;
+ private boolean applyFlushModeOnlyToNonExistingTransactions = false;
+ protected TransactionResources txResources = new
DefaultTransactionResources();
+
+ protected GrailsHibernateTemplate() {
+ // for testing
+ }
+
+ public GrailsHibernateTemplate(SessionFactory sessionFactory) {
+ Assert.notNull(sessionFactory, "Property 'sessionFactory' is
required");
+ this.sessionFactory = sessionFactory;
+
+ ConnectionProvider connectionProvider = ((SessionFactoryImplementor)
sessionFactory)
+ .getServiceRegistry()
+ .getService(ConnectionProvider.class);
+ this.dataSource = connectionProvider != null ?
connectionProvider.unwrap(DataSource.class) : null;
+ if (this.dataSource != null) {
+ if (this.dataSource instanceof TransactionAwareDataSourceProxy) {
+ DataSource target = ((TransactionAwareDataSourceProxy)
this.dataSource).getTargetDataSource();
+ if (target != null) {
+ this.dataSource = target;
+ }
+ }
+ jdbcExceptionTranslator = new
SQLErrorCodeSQLExceptionTranslator(this.dataSource);
+ } else {
+ // must be in unit test mode, setup default translator
+ SQLErrorCodeSQLExceptionTranslator
sqlErrorCodeSQLExceptionTranslator =
+ new SQLErrorCodeSQLExceptionTranslator();
+ sqlErrorCodeSQLExceptionTranslator.setDatabaseProductName("H2");
+ jdbcExceptionTranslator = sqlErrorCodeSQLExceptionTranslator;
+ }
+ }
+
+ public GrailsHibernateTemplate(SessionFactory sessionFactory,
HibernateDatastore datastore) {
+ this(sessionFactory);
+ if (datastore != null) {
+ cacheQueries = datastore.isCacheQueries();
+ this.osivReadOnly = datastore.isOsivReadOnly();
+ this.passReadOnlyToHibernate =
datastore.isPassReadOnlyToHibernate();
+ this.flushMode =
hibernateFlushModeToConstant(datastore.getDefaultFlushMode());
+ }
+ }
+
+ public GrailsHibernateTemplate(SessionFactory sessionFactory,
HibernateDatastore datastore, int defaultFlushMode) {
+ this(sessionFactory);
+ if (datastore != null) {
+ cacheQueries = datastore.isCacheQueries();
+ this.osivReadOnly = datastore.isOsivReadOnly();
+ this.passReadOnlyToHibernate =
datastore.isPassReadOnlyToHibernate();
+ }
+ this.flushMode = defaultFlushMode;
+ }
+
+ /** Maps a Hibernate {@link FlushMode} to one of the {@code FLUSH_*}
constants of this class. */
+ static int hibernateFlushModeToConstant(FlushMode mode) {
+ return switch (mode) {
+ case MANUAL -> FLUSH_NEVER;
+ case COMMIT -> FLUSH_COMMIT;
+ case ALWAYS -> FLUSH_ALWAYS;
+ default -> FLUSH_AUTO;
+ };
+ }
+
+ @Override
+ public <T> T execute(Closure<T> callable) {
+ @SuppressWarnings("unchecked")
+ HibernateCallback<T> hibernateCallback =
+ (HibernateCallback<T>) DefaultGroovyMethods.asType(callable,
HibernateCallback.class);
+ return execute(hibernateCallback);
+ }
+
+ @SuppressWarnings("PMD.DataflowAnomalyAnalysis")
+ @Override
+ public <T> T executeWithNewSession(final Closure<T> callable) {
+ SessionHolder sessionHolder = (SessionHolder)
txResources.getResource(sessionFactory);
+ SessionHolder previousHolder = sessionHolder;
+ ConnectionHolder previousConnectionHolder =
+ (ConnectionHolder) txResources.getResource(dataSource);
+ Session newSession = null;
+ boolean previousActiveSynchronization =
txResources.isSynchronizationActive();
+ List<TransactionSynchronization> transactionSynchronizations =
+ previousActiveSynchronization ?
txResources.getSynchronizations() : null;
+ try {
+ // if there are any previous synchronizations active we need to
clear them and restore them
+ // later (see finally block)
+ if (previousActiveSynchronization) {
+ txResources.clearSynchronization();
+ // init a new synchronization to ensure that any opened
database connections are closed by
+ // the synchronization
+ txResources.initSynchronization();
+ }
+
+ // if there are already bound holders, unbind them so they can be
restored later
+ if (sessionHolder != null) {
+ txResources.unbindResource(sessionFactory);
+ if (previousConnectionHolder != null) {
+ txResources.unbindResource(dataSource);
+ }
+ }
+
+ // create and bind a new session holder for the new session
+ newSession = sessionFactory.openSession();
+ applyFlushMode(newSession, false);
+ sessionHolder = new SessionHolder(newSession);
+ txResources.bindResource(sessionFactory, sessionHolder);
+
+ return callable.call(newSession);
+ } finally {
+ try {
+ // if an active synchronization was registered during the life
time of the new session clear
+ // it
+ if (txResources.isSynchronizationActive()) {
+ txResources.clearSynchronization();
+ }
+ // If there is a synchronization active then leave it to the
synchronization to close the
+ // session
+ // Clear any bound sessions and connections
+ txResources.unbindResource(sessionFactory);
+ ConnectionHolder connectionHolder =
+ (ConnectionHolder)
txResources.unbindResourceIfPossible(dataSource);
+ // if there is a connection holder and it holds an open
connection close it
+ try {
+ if (connectionHolder != null &&
+ !(dataSource instanceof
org.grails.datastore.gorm.jdbc.MultiTenantDataSource) &&
+ !connectionHolder.getConnection().isClosed()) {
+ Connection conn = connectionHolder.getConnection();
+ DataSourceUtils.releaseConnection(conn, dataSource);
+ }
+ } catch (SQLException e) {
+ // ignore, connection closed already?
+ if (LOG.isDebugEnabled()) {
+ LOG.debug(
+ "Could not close opened JDBC connection. Did
the application close the connection manually?: " +
+ e.getMessage());
+ }
+ }
+
+ if (newSession != null) {
+ SessionFactoryUtils.closeSession(newSession);
+ }
+ } finally {
+ // if there were previously active synchronizations then
register those again
+ if (previousActiveSynchronization) {
+ txResources.initSynchronization();
+ for (TransactionSynchronization transactionSynchronization
: transactionSynchronizations) {
+
txResources.registerSynchronization(transactionSynchronization);
+ }
+ }
+
+ // now restore any previous state
+ if (previousHolder != null) {
+ txResources.bindResource(sessionFactory, previousHolder);
+ if (previousConnectionHolder != null) {
+ txResources.bindResource(dataSource,
previousConnectionHolder);
+ }
+ }
+ }
+ }
+ }
+
+ @Override
+ public <T1> T1 executeWithExistingOrCreateNewSession(SessionFactory
sessionFactory, Closure<T1> callable) {
+ SessionHolder sessionHolder = (SessionHolder)
txResources.getResource(sessionFactory);
+ if (sessionHolder == null) {
+ return executeWithNewSession(callable);
+ } else {
+ return callable.call(sessionHolder.getSession());
+ }
+ }
+
+ @Override
+ public SessionFactory getSessionFactory() {
+ return sessionFactory;
+ }
+
+ @Override
+ public void applySettings(org.hibernate.query.Query<?> query) {
+ if (exposeNativeSession) {
+ prepareQuery(query);
+ }
+ }
+
+ public boolean isCacheQueries() {
+ return cacheQueries;
+ }
+
+ public void setCacheQueries(boolean cacheQueries) {
+ this.cacheQueries = cacheQueries;
+ }
+
+ @SuppressWarnings("PMD.PreserveStackTrace")
+ public <T> T execute(HibernateCallback<T> action) throws
DataAccessException {
+ return doExecute(action, false);
+ }
+
+ public List<?> executeFind(HibernateCallback<?> action) throws
DataAccessException {
+ Object result = doExecute(action, false);
+ if (result != null && !(result instanceof List)) {
+ throw new InvalidDataAccessApiUsageException(
+ "Result object returned from HibernateCallback isn't a
List: [" + result + "]");
+ }
+ return (List<?>) result;
+ }
+
+ protected boolean shouldPassReadOnlyToHibernate() {
+ if ((passReadOnlyToHibernate || osivReadOnly) &&
+ txResources.hasResource(getSessionFactory())) {
+ if (txResources.isActualTransactionActive()) {
+ return passReadOnlyToHibernate &&
txResources.isCurrentTransactionReadOnly();
+ } else {
+ return osivReadOnly;
+ }
+ } else {
+ return false;
+ }
+ }
+
+ public boolean isOsivReadOnly() {
+ return osivReadOnly;
+ }
+
+ public void setOsivReadOnly(boolean osivReadOnly) {
+ this.osivReadOnly = osivReadOnly;
+ }
+
+ /**
+ * Execute the action specified by the given action object within a
Session.
+ *
+ * @param action callback object that specifies the Hibernate action
+ * @param enforceNativeSession whether to enforce exposure of the native
Hibernate Session to
+ * callback code
+ * @return a result object returned by the action, or <code>null</code>
+ * @throws org.springframework.dao.DataAccessException in case of
Hibernate errors
+ */
+ @SuppressWarnings("PMD.PreserveStackTrace")
+ protected <T> T doExecute(HibernateCallback<T> action, boolean
enforceNativeSession) throws DataAccessException {
+
+ Assert.notNull(action, "Callback object must not be null");
+
+ Session session = getSession();
+ boolean existingTransaction = isSessionTransactional(session);
+ if (existingTransaction) {
+ LOG.debug("Found thread-bound Session for HibernateTemplate");
+ }
+
+ FlushMode previousFlushMode = null;
+ try {
+ previousFlushMode = applyFlushMode(session, existingTransaction);
+ if (shouldPassReadOnlyToHibernate()) {
+ session.setDefaultReadOnly(true);
+ }
+ Session sessionToExpose =
+ (enforceNativeSession || exposeNativeSession ? session :
createSessionProxy(session));
+ T result = action.doInHibernate(sessionToExpose);
+ flushIfNecessary(session, existingTransaction);
+ return result;
+ } catch (HibernateException ex) {
+ throw convertHibernateAccessException(ex);
+ } catch (PersistenceException ex) {
+ if (ex.getCause() instanceof HibernateException
hibernateException) {
+ throw
SessionFactoryUtils.convertHibernateAccessException(hibernateException);
+ }
+ throw ex;
+ } catch (SQLException ex) {
+ throw Objects.requireNonNull(
+ jdbcExceptionTranslator.translate("Hibernate-related JDBC
operation", null, ex));
+ } finally {
+ if (existingTransaction) {
+ LOG.debug("Not closing pre-bound Hibernate Session after
HibernateTemplate");
+ if (previousFlushMode != null) {
+ session.setHibernateFlushMode(previousFlushMode);
+ }
+ } else {
+ SessionFactoryUtils.closeSession(session);
+ }
+ }
+ }
+
+ protected boolean isSessionTransactional(Session session) {
+ SessionHolder sessionHolder = (SessionHolder)
txResources.getResource(sessionFactory);
+ return sessionHolder != null && sessionHolder.getSession() == session;
+ }
+
+ public Session getSession() {
+ try {
+ return sessionFactory.getCurrentSession();
+ } catch (HibernateException ex) {
+ throw new DataAccessResourceFailureException("Could not obtain
current Hibernate Session", ex);
+ }
+ }
+
+ /**
+ * Create a close-suppressing proxy for the given Hibernate Session. The
proxy also prepares
+ * returned Query and Criteria objects.
+ *
+ * @param session the Hibernate Session to create a proxy for
+ * @return the Session proxy
+ * @see org.hibernate.Session#close()
+ * @see #prepareQuery
+ * @see #prepareCriteria
+ */
+ protected Session createSessionProxy(Session session) {
+ Class<?>[] sessionIfcs;
+ Class<?> mainIfc = Session.class;
+ if (session instanceof EventSource) {
+ sessionIfcs = new Class[] {mainIfc, EventSource.class};
+ } else if (session instanceof SessionImplementor) {
+ sessionIfcs = new Class[] {mainIfc, SessionImplementor.class};
+ } else {
+ sessionIfcs = new Class[] {mainIfc};
+ }
+ return (Session) Proxy.newProxyInstance(
+ Thread.currentThread().getContextClassLoader(),
+ sessionIfcs,
+ new CloseSuppressingInvocationHandler(session, this));
+ }
+
+ @Override
+ @Deprecated(since = "7.0", forRemoval = true)
Review Comment:
We probably should rediff the hibernate 5 class with this - these were
removed in 5.
##########
grails-test-examples/hibernate7/grails-partitioned-multi-tenancy/src/test/groovy/example/PartitionedMultiTenancySpec.groovy:
##########
@@ -40,6 +41,10 @@ class PartitionedMultiTenancySpec extends HibernateSpec {
)
}
+ def cleanup() {
+ System.setProperty(SystemPropertyTenantResolver.PROPERTY_NAME, "")
Review Comment:
Adopt `@RestoreSystemProperties` instead?
##########
grails-data-hibernate7/core/src/main/groovy/grails/orm/CriteriaMethods.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * 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.orm;
+
+import groovy.lang.MissingMethodException;
+
+/** Enum representing the supported methods in HibernateCriteriaBuilder. */
+public enum CriteriaMethods {
Review Comment:
So one of the benefits of the criteria builder before was you could extend
it to add behavior. This enum indicates that may not be possible now?
##########
grails-test-examples/hibernate7/grails-multitenant-multi-datasource/grails-app/services/example/MetricService.groovy:
##########
@@ -60,6 +60,6 @@ abstract class MetricService {
* Delete all metrics for the current tenant from the secondary datasource.
*/
void deleteAll() {
- secondaryApi.executeUpdate('delete from Metric')
+ secondaryApi.executeUpdate('delete from Metric', [:])
Review Comment:
Isn't the empty map not necessary since that's the default?
##########
plans/aggregate-style-violations.md:
##########
@@ -0,0 +1,65 @@
+<!--
+SPDX-License-Identifier: Apache-2.0
+
+Licensed 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.
+-->
+# Implementation Plan: Aggregate Style Violations
Review Comment:
Is this plan necessary still?
##########
grails-test-examples/hibernate7/grails-multitenant-multi-datasource/src/integration-test/groovy/functionaltests/MultiTenantMultiDataSourceSpec.groovy:
##########
@@ -46,14 +46,17 @@ import org.grails.orm.hibernate.HibernateDatastore
* @see example.MetricService
*/
@Integration
-@RestoreSystemProperties
class MultiTenantMultiDataSourceSpec extends Specification {
@Autowired
HibernateDatastore hibernateDatastore
MetricService metricService
+ void cleanup() {
Review Comment:
Adopt `@RestoreSystemProperties` instead?
##########
grails-test-examples/hibernate7/grails-hibernate/grails-app/controllers/functional/tests/BookController.groovy:
##########
@@ -43,8 +38,7 @@ class BookController {
}
def create() {
- def book = new Book(params.subMap(bindParams))
- respond book
+ respond new Book(params)
Review Comment:
Can you help me understand why we are binding all params vs a specific one
now? I'm assuming this was testing a specific scenario before?
##########
grails-test-examples/hibernate7/grails-partitioned-multi-tenancy/grails-app/controllers/example/BookController.groovy:
##########
@@ -54,8 +49,7 @@ class BookController {
}
def create() {
- def book = new Book(params.subMap(bindParams))
- respond book
+ respond new Book(params)
Review Comment:
I believe this was explicitly testing only certain fields before? Can you
help me understand why the change?
##########
grails-data-hibernate7/core/build.gradle:
##########
@@ -0,0 +1,159 @@
+/*
+ * 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.
+ */
+
+plugins {
+ id 'groovy'
+ id 'java-library'
+ id 'org.apache.grails.buildsrc.properties'
+ id 'org.apache.grails.buildsrc.dependency-validator'
+ id 'org.apache.grails.buildsrc.compile'
+ id 'org.apache.grails.buildsrc.publish'
+ id 'org.apache.grails.buildsrc.sbom'
+ id 'org.apache.grails.gradle.grails-code-style'
+}
+
+version = projectVersion
+group = 'org.apache.grails.data'
+
+ext {
+ gormApiDocs = true
+ pomTitle = 'Grails GORM Hibernate 7'
+ pomDescription = 'GORM - Grails Data Access Framework - Hibernate 7'
+}
+
+dependencies {
+ // TODO: Clarify and clean up dependencies
+ implementation platform(project(':grails-hibernate7-bom'))
+
+ api 'org.slf4j:slf4j-api'
+
+ api 'org.apache.groovy:groovy'
+ api project(':grails-datamapping-core')
+ api project(':grails-data-hibernate7-spring-orm')
+ api 'org.springframework:spring-orm'
+ compileOnly 'org.springframework:spring-webmvc'
+ compileOnly 'jakarta.servlet:jakarta.servlet-api'
+ implementation "net.bytebuddy:byte-buddy"
+ api 'org.hibernate.orm:hibernate-core', {
+ exclude group:'commons-logging', module:'commons-logging'
+ exclude group:'com.h2database', module:'h2'
+ exclude group:'commons-collections', module:'commons-collections'
+ exclude group:'org.slf4j', module:'jcl-over-slf4j'
+ exclude group:'org.slf4j', module:'slf4j-api'
+ exclude group:'org.slf4j', module:'slf4j-log4j12'
+ exclude group:'xml-apis', module:'xml-apis'
+ }
+ api "org.hibernate.models:hibernate-models"
+ api 'org.hibernate.validator:hibernate-validator', {
+ exclude group:'commons-logging', module:'commons-logging'
+ exclude group:'commons-collections', module:'commons-collections'
+ exclude group:'org.slf4j', module:'slf4j-api'
+ }
+ api 'jakarta.validation:jakarta.validation-api'
+ api 'org.checkerframework:checker-qual'
+
+ api 'io.smallrye:jandex'
+
+ compileOnly 'org.hibernate.orm:hibernate-core'
+ compileOnly 'org.hibernate.orm:hibernate-jcache', {
+ exclude group:'commons-collections', module:'commons-collections'
+ exclude group:'commons-logging', module:'commons-logging'
+ exclude group:'com.h2database', module:'h2'
+ exclude group:'net.sf.ehcache', module:'ehcache'
+ exclude group:'net.sf.ehcache', module:'ehcache-core'
+
+ exclude group:'org.slf4j', module:'jcl-over-slf4j'
+ exclude group:'org.slf4j', module:'slf4j-api'
+ exclude group:'org.slf4j', module:'slf4j-log4j12'
+ exclude group:'xml-apis', module:'xml-apis'
+ }
+
+ testImplementation 'org.testcontainers:testcontainers'
+ testImplementation 'org.postgresql:postgresql'
+ testImplementation 'org.testcontainers:testcontainers-postgresql'
+ testImplementation 'com.mysql:mysql-connector-j'
+ testImplementation 'org.testcontainers:testcontainers-mysql'
+ testImplementation 'org.mariadb.jdbc:mariadb-java-client'
+ testImplementation 'org.testcontainers:testcontainers-mariadb'
+ testImplementation 'com.oracle.database.jdbc:ojdbc11'
+ testImplementation 'org.testcontainers:testcontainers-oracle-free'
+ testImplementation 'org.testcontainers:testcontainers-spock'
+ testImplementation 'org.jsr107.ri:cache-ri-impl'
+
+ testImplementation 'org.objenesis:objenesis'
+
+
+ testImplementation 'com.h2database:h2'
+ testImplementation 'org.junit.platform:junit-platform-suite', {
+ // api: SelectClasses, Suite
+ }
+
+ testImplementation 'org.apache.groovy:groovy-test-junit5'
+ testImplementation 'org.apache.groovy:groovy-sql'
+ testImplementation 'org.apache.groovy:groovy-json'
+ testImplementation 'org.hibernate.orm:hibernate-jcache'
+ testImplementation 'org.spockframework:spock-core'
+ testImplementation "org.hibernate.orm:hibernate-core"
+
+ // groovy proxy fixes bytebuddy to be a bit smarter when it comes to
groovy metaClass
+ testImplementation 'org.yakworks:hibernate-groovy-proxy', {
+ exclude group: 'org.codehaus.groovy', module: 'groovy'
+ exclude group: 'org.hibernate', module: 'hibernate-core'
+ exclude group: 'org.hibernate.orm', module: 'hibernate-core'
+ }
+
+ testImplementation 'org.apache.tomcat:tomcat-jdbc'
+ testImplementation 'org.spockframework:spock-core'
+
+ testRuntimeOnly 'org.slf4j:slf4j-simple'
+ testRuntimeOnly 'org.slf4j:jcl-over-slf4j'
+ testRuntimeOnly 'org.springframework:spring-aop'
+ testRuntimeOnly 'org.mockito:mockito-inline'
+
+}
+
+sourceSets {
+ test {
+ groovy.srcDirs = ['src/test/groovy']
+ }
+}
+
+apply {
+ from
rootProject.layout.projectDirectory.file('gradle/hibernate7-test-config.gradle')
+ from
rootProject.layout.projectDirectory.file('gradle/grails-data-tck-config.gradle')
+ from rootProject.layout.projectDirectory.file('gradle/docs-config.gradle')
+}
+
+// spotbugs {
Review Comment:
Shouldn't we centralize this in the plugin? If not, can we remove the
commented code?
##########
grails-test-examples/hibernate7/grails-database-per-tenant/src/test/groovy/example/DatabasePerTenantSpec.groovy:
##########
@@ -42,6 +43,10 @@ class DatabasePerTenantSpec extends HibernateSpec {
)
}
+ def cleanup() {
+ System.setProperty(SystemPropertyTenantResolver.PROPERTY_NAME, "")
Review Comment:
Adopt `@RestoreSystemProperties` instead?
##########
grails-test-examples/hibernate7/grails-hibernate/build.gradle:
##########
@@ -55,10 +55,8 @@ dependencies {
runtimeOnly 'org.apache.grails:grails-services'
runtimeOnly 'org.apache.grails:grails-url-mappings'
runtimeOnly 'org.apache.grails:grails-fields'
- runtimeOnly 'org.hibernate:hibernate-ehcache', {
- // exclude javax variant of hibernate-core 5.6
- exclude group: 'org.hibernate', module: 'hibernate-core'
- }
+ runtimeOnly 'org.hibernate.orm:hibernate-jcache'
+ // TODO: hibernate-ehcache was icnluded here but historically was replaced
by hibernate-jcache
Review Comment:
I believe I added this TODO, @borinquenkid do you know of specific
configuration that's needed for hibernate 7? I'm guessing this may be our only
test app with caching enabled?
##########
grails-test-examples/gorm/src/integration-test/groovy/gorm/GormCriteriaQueriesSpec.groovy:
##########
@@ -24,21 +24,20 @@ import spock.lang.Unroll
import grails.gorm.DetachedCriteria
import grails.gorm.transactions.Rollback
import grails.testing.mixin.integration.Integration
-
/**
* Tests for GORM Criteria Queries - both createCriteria() and
DetachedCriteria.
*
* Criteria queries provide a type-safe, programmatic way to build
* complex queries without writing HQL strings.
*/
@Rollback
-@Integration
+@Integration(applicationClass = Application)
class GormCriteriaQueriesSpec extends Specification {
def setup() {
// Clean up and create fresh test data
- Book.executeUpdate('delete from Book')
- Author.executeUpdate('delete from Author')
+ Book.executeUpdate('delete from Book', [:])
Review Comment:
Is the empty map necessary? (repeated for the below)
##########
grails-test-examples/hibernate7/grails-schema-per-tenant/grails-app/controllers/schemapertenant/BookController.groovy:
##########
@@ -54,8 +49,7 @@ class BookController {
}
def create() {
- def book = new Book(params.subMap(bindParams))
- respond book
+ respond new Book(params)
Review Comment:
Wasn't line 35 above testing a specific scenario? Now it's binding all
fields instead of a specific one.
##########
grails-test-examples/hibernate7/grails-hibernate-groovy-proxy/grails-app/conf/application.yml:
##########
@@ -30,3 +30,6 @@ dataSource:
dbCreate: create-drop
url: jdbc:h2:mem:books;LOCK_TIMEOUT=10000;DB_CLOSE_ON_EXIT=FALSE
+hibernate:
+ proxy_factory_class:
org.grails.orm.hibernate.proxy.ByteBuddyGroovyProxyFactory
Review Comment:
I noticed that Scott opened a change to the way groovy proxies work. You're
defining a custom proxy factory class here, but did we adjust the default
configuration for this? Does this need to be the default in generated apps?
Is this to test a specific factory?
##########
grails-test-examples/gorm/src/integration-test/groovy/gorm/TransactionPropagationSpec.groovy:
##########
@@ -44,8 +44,8 @@ class TransactionPropagationSpec extends Specification {
def setup() {
// Clean up before each test - delete books first due to FK constraint
Author.withNewTransaction {
- Book.executeUpdate('delete from Book')
- Author.executeUpdate('delete from Author')
+ Book.executeUpdate('delete from Book', [:])
Review Comment:
Is the empty map necessary?
##########
grails-test-examples/gorm/src/integration-test/groovy/gorm/GormEventsSpec.groovy:
##########
@@ -42,7 +42,7 @@ import grails.testing.mixin.integration.Integration
class GormEventsSpec extends Specification {
def setup() {
- AuditedEntity.executeUpdate('delete from AuditedEntity')
+ AuditedEntity.executeUpdate('delete from AuditedEntity', [:])
Review Comment:
Is the empty map necessary?
##########
grails-test-examples/hibernate7/grails-data-service-multi-datasource/src/integration-test/groovy/functionaltests/DataServiceDatasourceInheritanceSpec.groovy:
##########
@@ -34,7 +34,7 @@ class DataServiceDatasourceInheritanceSpec extends
Specification {
void cleanup() {
Product.secondary.withTransaction {
- Product.secondary.executeUpdate('delete from Product')
+ Product.secondary.executeUpdate('delete from Product', [:])
Review Comment:
Is the empty map still necessary?
##########
grails-test-examples/hibernate7/grails-data-service-multi-datasource/src/integration-test/groovy/functionaltests/DataServiceMultiDataSourceSpec.groovy:
##########
@@ -56,7 +56,7 @@ class DataServiceMultiDataSourceSpec extends Specification {
void cleanup() {
Product.secondary.withTransaction {
- Product.secondary.executeUpdate('delete from Product')
+ Product.secondary.executeUpdate('delete from Product', [:])
Review Comment:
Is the empty map still necessary?
##########
grails-test-examples/gorm/src/integration-test/groovy/gorm/GormWhereQueryAdvancedSpec.groovy:
##########
@@ -39,8 +39,8 @@ class GormWhereQueryAdvancedSpec extends Specification {
def setup() {
// Clean up existing data
- Book.executeUpdate('delete from Book')
- Author.executeUpdate('delete from Author')
+ Book.executeUpdate('delete from Book', [:])
Review Comment:
Is the empty map necessary?
##########
grails-test-examples/hibernate7/grails-database-per-tenant/grails-app/controllers/example/BookController.groovy:
##########
@@ -54,8 +49,7 @@ class BookController {
}
def create() {
- def book = new Book(params.subMap(bindParams))
- respond book
+ respond new Book(params)
Review Comment:
I'm repeating this comment in several places, but can you help me understand
why you aren't binding one specific field any longer here?
##########
grails-test-examples/gorm/src/integration-test/groovy/gorm/GormCriteriaQueriesSpec.groovy:
##########
@@ -24,21 +24,20 @@ import spock.lang.Unroll
import grails.gorm.DetachedCriteria
import grails.gorm.transactions.Rollback
import grails.testing.mixin.integration.Integration
-
/**
* Tests for GORM Criteria Queries - both createCriteria() and
DetachedCriteria.
*
* Criteria queries provide a type-safe, programmatic way to build
* complex queries without writing HQL strings.
*/
@Rollback
-@Integration
+@Integration(applicationClass = Application)
Review Comment:
The applicationClass is redundant so we should remove it.
##########
grails-gsp/grails-web-gsp/src/main/groovy/org/grails/web/pages/GroovyPagesServlet.java:
##########
@@ -117,7 +117,7 @@ protected void initFrameworkServlet() throws BeansException
{
context.setAttribute(SERVLET_INSTANCE, this);
final WebApplicationContext webApplicationContext =
getWebApplicationContext();
- grailsAttributes =
GrailsFactoriesLoader.loadFactoriesWithArguments(GrailsApplicationAttributes.class,
getClass().getClassLoader(), new Object[]{context}).get(0);
+ grailsAttributes =
GrailsFactoriesLoader.loadFactoriesWithArguments(GrailsApplicationAttributes.class,
Thread.currentThread().getContextClassLoader(), new Object[]{context}).get(0);
Review Comment:
I am guessing this is one of those PMD bugs, but from prior experience this
has caused issues using the thread class loader. I think this is an ok change,
but it's something we need to be aware of. @davydotcom any feedback here?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/NegationSpec.groovy:
##########
@@ -18,15 +18,20 @@
*/
package org.apache.grails.data.testing.tck.tests
-import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
import org.apache.grails.data.testing.tck.domains.Book
+import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
/**
* @author graemerocher
*/
class NegationSpec extends GrailsDataTckSpec {
- void 'Test negation in dynamic finder'() {
+ void setupSpec() {
+ manager.addAllDomainClasses([Book])
+ }
+
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
These worked in mongo/ simple / graphql before. Isn't this a regression?
I'm assuming adding the Requires should be reverted
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/EnumSpec.groovy:
##########
@@ -127,6 +116,105 @@ class EnumSpec extends GrailsDataTckSpec {
instance3 == null
}
+ @Issue('GPMONGODB-248')
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
+ void "Test findByInList()"() {
+ given:
+
+ new EnumThing(name: 'e1', en: TestEnum.V1).save(failOnError: true)
+ new EnumThing(name: 'e2', en: TestEnum.V1).save(failOnError: true)
+ new EnumThing(name: 'e3', en: TestEnum.V2).save(failOnError: true)
+
+ List instance1
+ List instance2
+ List instance3
+
+ when:
+ instance1 = EnumThing.findAllByEn(TestEnum.V1)
+ instance2 = EnumThing.findAllByEn(TestEnum.V2)
+ instance3 = EnumThing.findAllByEn(TestEnum.V3)
+
+ then:
+ instance1.size() == 2
+ instance1.every { it.en == TestEnum.V1 }
+
+ instance2.size() == 1
+ instance2.every { it.en == TestEnum.V2 }
+
+ instance3.isEmpty()
+
+ when:
+ instance1 = EnumThing.findAllByEnInList([TestEnum.V1])
+ instance2 = EnumThing.findAllByEnInList([TestEnum.V2])
+ instance3 = EnumThing.findAllByEnInList([TestEnum.V3])
+
+ then:
+ instance1.size() == 2
+ instance1.every { it.en == TestEnum.V1 }
+
+ instance2.size() == 1
+ instance2.every { it.en == TestEnum.V2 }
+
+ instance3.isEmpty()
+ }
+
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
+ void "Test findAllBy()"() {
+ given:
+
+ new EnumThing(name: 'e1', en: TestEnum.V1).save(failOnError: true)
+ new EnumThing(name: 'e2', en: TestEnum.V1).save(failOnError: true)
+ new EnumThing(name: 'e3', en: TestEnum.V2).save(failOnError: true)
+
+ List instance1
+ List instance2
+ List instance3
+
+ when:
+ instance1 = EnumThing.findAllByEn(TestEnum.V1)
+ instance2 = EnumThing.findAllByEn(TestEnum.V2)
+ instance3 = EnumThing.findAllByEn(TestEnum.V3)
+
+ then:
+ instance1.size() == 2
+ instance1.every { it.en == TestEnum.V1 }
+
+ instance2.size() == 1
+ instance2.every { it.en == TestEnum.V2 }
+
+ instance3.isEmpty()
+
+ }
+
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/query/Query.java:
##########
@@ -589,6 +616,32 @@ public Object singleResult() {
return results.isEmpty() ? null : results.get(0);
}
+ /**
+ * Counts the rows this query would return, respecting any existing
projections or grouping.
+ * Subclasses may override to provide an optimized implementation (e.g.,
derived-table count).
+ * The default implementation falls back to loading all rows when
user-defined projections
+ * exist, since appending a count projection would produce incorrect
results.
+ *
+ * @return The row count
+ */
+ public Number countResults() {
+ if (!projections.getProjectionList().isEmpty()) {
+ // When user-defined projections exist (e.g. groupProperty +
count),
+ // a simple count() projection returns incorrect results because it
+ // appends to the existing projections rather than replacing them.
+ // Fall back to counting the grouped result rows.
+ // TODO: This needs resolved properly in Grails 8 with Hibernate
7's
Review Comment:
Is this due to a bad merge? @borinquenkid i think this is a bug still in
hibernate 5, but is it fixed in 7? should we update the todo now?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/GormEnhancerSpec.groovy:
##########
@@ -129,7 +134,8 @@ class GormEnhancerSpec extends GrailsDataTckSpec {
t.id == t.ident()
}
- void 'Test dynamic finder with pagination parameters'() {
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/NullValueEqualSpec.groovy:
##########
@@ -18,14 +18,18 @@
*/
package org.apache.grails.data.testing.tck.tests
-import spock.lang.IgnoreIf
-
-import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
import org.apache.grails.data.testing.tck.domains.TestEntity
+import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
+import spock.lang.IgnoreIf
class NullValueEqualSpec extends GrailsDataTckSpec {
- void 'test null value in equal'() {
+ void setupSpec() {
+ manager.addAllDomainClasses([TestEntity])
+ }
+
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Why are we adding these requires now? Did we break the API here?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/InheritanceSpec.groovy:
##########
@@ -30,9 +30,10 @@ import org.apache.grails.data.testing.tck.domains.Practice
class InheritanceSpec extends GrailsDataTckSpec {
void setupSpec() {
- manager.domainClasses += [Practice]
+ manager.addAllDomainClasses([Practice, City, Country, Location])
}
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
we should revert the requires?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/OptimisticLockingSpec.groovy:
##########
@@ -59,67 +66,132 @@ class OptimisticLockingSpec extends GrailsDataTckSpec {
o.version == 1
}
- // hibernate has a customized version of this
- @IgnoreIf({ System.getProperty('hibernate5.gorm.suite') })
+ @IgnoreIf({ System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
This looks like a regression in mongodb, we needt o revert the ignore if? I
guess this was fixed in hibernate5?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/FindByMethodSpec.groovy:
##########
@@ -67,6 +83,11 @@ class FindByMethodSpec extends GrailsDataTckSpec {
1 == cnt
}
+ @Requires({
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/DomainEventsSpec.groovy:
##########
@@ -265,6 +270,7 @@ class DomainEventsSpec extends GrailsDataTckSpec {
1 == PersonEvent.STORE.afterLoad
}
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/OptimisticLockingSpec.groovy:
##########
@@ -59,67 +66,132 @@ class OptimisticLockingSpec extends GrailsDataTckSpec {
o.version == 1
}
- // hibernate has a customized version of this
- @IgnoreIf({ System.getProperty('hibernate5.gorm.suite') })
+ @IgnoreIf({ System.getProperty('mongodb.gorm.suite') == 'true' })
void "Test optimistic locking"() {
given:
def o = new OptLockVersioned(name: 'locked').save(flush: true)
manager.session.clear()
+ manager.transactionManager.commit manager.transactionStatus
+ manager.transactionStatus = null
when:
- o = OptLockVersioned.get(o.id)
-
- Thread.start {
- OptLockVersioned.withNewSession { s ->
- def reloaded = OptLockVersioned.get(o.id)
- assert reloaded
- reloaded.name += ' in new session'
- reloaded.save(flush: true)
+ OptLockVersioned.withTransaction {
+ try {
+ o = OptLockVersioned.get(o.id)
+
+ Thread.start {
+ OptLockVersioned.withTransaction { s ->
+ def reloaded = OptLockVersioned.get(o.id)
+ assert reloaded
+ assert reloaded != o
+ reloaded.name += ' in new session'
+ reloaded.save(flush: true)
+ assert reloaded.version == 1
+ assert o.version == 0
+ }
+
+ }.join()
+
+ o.name += ' in main session'
+ o.save(flush: true)
+
+ manager.session.clear()
+ o = OptLockVersioned.get(o.id)
+ } catch (Throwable e) {
+ System.getProperties().each { key, value ->
+ println "${key}: ${value}"
+ }
+ throw e
}
- }.join()
- sleep(2000) // heisenbug
+ }
+ then:
+ thrown OptimisticLockingFailureException
+ }
- o.name += ' in main session'
+ @IgnoreIf({ System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
This looks like a regression in mongodb, we need to revert the ignore if?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/GormEnhancerSpec.groovy:
##########
@@ -78,7 +82,8 @@ class GormEnhancerSpec extends GrailsDataTckSpec {
'Bob' == bob.name
}
- void 'Test dynamic finder with disjunction'() {
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/GormEnhancerSpec.groovy:
##########
@@ -146,7 +152,8 @@ class GormEnhancerSpec extends GrailsDataTckSpec {
1 == TestEntity.findAllByNameOrAge('Barney', 40, [max: 1]).size()
}
- void 'Test in list query'() {
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/ListOrderBySpec.groovy:
##########
@@ -18,16 +18,21 @@
*/
package org.apache.grails.data.testing.tck.tests
-import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
import org.apache.grails.data.testing.tck.domains.ChildEntity
import org.apache.grails.data.testing.tck.domains.TestEntity
+import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
/**
* @author graemerocher
*/
class ListOrderBySpec extends GrailsDataTckSpec {
- void 'Test listOrderBy property name method'() {
+ void setupSpec() {
+ manager.addAllDomainClasses([TestEntity, ChildEntity])
+ }
+
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
We should revert the requires?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/FindByMethodSpec.groovy:
##########
@@ -93,12 +114,17 @@ class FindByMethodSpec extends GrailsDataTckSpec {
3 == cnt
}
+ @Requires({
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/GormEnhancerSpec.groovy:
##########
@@ -180,7 +188,8 @@ class GormEnhancerSpec extends GrailsDataTckSpec {
results.find { it.name == 'Frank' } != null
}
- void 'Test ilike query'() {
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/GormEnhancerSpec.groovy:
##########
@@ -164,7 +171,8 @@ class GormEnhancerSpec extends GrailsDataTckSpec {
2 == TestEntity.findAllByNameInListOrName(['Joe', 'Frank'],
'Bob').size()
}
- void 'Test like query'() {
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/proxy/GroovyProxyFactory.groovy:
##########
@@ -46,11 +47,10 @@ class GroovyProxyFactory implements ProxyFactory {
getProxyInstanceMetaClass(object) != null
}
- @Override
@Override
Class<?> getProxiedClass(Object o) {
if (isProxy(o)) {
- return o.getClass().getSuperclass()
+ return o.getClass()
Review Comment:
I mentioned this on Scott's PR, but there have been regressions around this
and embedded types. We originally had this change and then reverted it. I
think we need to look into the original change see
https://github.com/apache/grails-core/pull/15650#issuecomment-4423914813 for
details
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormStaticApi.groovy:
##########
@@ -1012,32 +1002,31 @@ class GormStaticApi<D> extends AbstractGormApi<D>
implements GormAllOperations<D
/**
* Creates and binds a new session for the scope of the given closure
*/
- <T> T withNewSession(Closure<T> callable) {
+ public <T> T withNewSession(Closure<T> callable) {
def session = datastore.connect()
try {
- DatastoreUtils.bindNewSession(session)
+ DatastoreUtils.bindNewSession session
return callable?.call(session)
}
finally {
- DatastoreUtils.unbindSession(session)
+ DatastoreUtils.unbindSession session
}
}
/**
* Creates and binds a new session for the scope of the given closure
*/
- <T> T withStatelessSession(Closure<T> callable) {
+ public <T> T withStatelessSession(Closure<T> callable) {
if (datastore instanceof StatelessDatastore) {
def session = datastore.connectStateless()
try {
- DatastoreUtils.bindNewSession(session)
+ DatastoreUtils.bindNewSession session
Review Comment:
There are style changes in this file that don't match our standard - calling
methods should continue to use parens.
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/EnumSpec.groovy:
##########
@@ -127,6 +116,105 @@ class EnumSpec extends GrailsDataTckSpec {
instance3 == null
}
+ @Issue('GPMONGODB-248')
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/NotLikeSpec.groovy:
##########
@@ -27,6 +27,11 @@ import
org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
*/
class NotLikeSpec extends GrailsDataTckSpec<GrailsDataCoreTckManager> {
+ void setupSpec() {
+ manager.addAllDomainClasses([TestEntity])
+ }
+
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/AttachMethodSpec.groovy:
##########
@@ -47,17 +51,11 @@ class AttachMethodSpec extends GrailsDataTckSpec {
!test.attached
when:
- test.attach()
+ test = test.attach()
then:
manager.session.contains(test)
test.isAttached()
test.attached
-
- when:
- test.discard()
Review Comment:
Can you not discard now? Isn't this a valid test?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/domains/Person.groovy:
##########
@@ -38,13 +37,13 @@ class Person implements Serializable, Comparable<Person>,
AsyncEntity<Person> {
lastName == 'Simpson'
}
- Long id
+// Long id
Long version
String firstName
String lastName
Integer age = 0
- Set<Pet> pets = [] as Set
static hasMany = [pets: Pet]
+// SimpleCountry country
Review Comment:
Can we remove the commented code that isn't being used?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/EnumSpec.groovy:
##########
@@ -127,6 +116,105 @@ class EnumSpec extends GrailsDataTckSpec {
instance3 == null
}
+ @Issue('GPMONGODB-248')
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
+ void "Test findByInList()"() {
+ given:
+
+ new EnumThing(name: 'e1', en: TestEnum.V1).save(failOnError: true)
+ new EnumThing(name: 'e2', en: TestEnum.V1).save(failOnError: true)
+ new EnumThing(name: 'e3', en: TestEnum.V2).save(failOnError: true)
+
+ List instance1
+ List instance2
+ List instance3
+
+ when:
+ instance1 = EnumThing.findAllByEn(TestEnum.V1)
+ instance2 = EnumThing.findAllByEn(TestEnum.V2)
+ instance3 = EnumThing.findAllByEn(TestEnum.V3)
+
+ then:
+ instance1.size() == 2
+ instance1.every { it.en == TestEnum.V1 }
+
+ instance2.size() == 1
+ instance2.every { it.en == TestEnum.V2 }
+
+ instance3.isEmpty()
+
+ when:
+ instance1 = EnumThing.findAllByEnInList([TestEnum.V1])
+ instance2 = EnumThing.findAllByEnInList([TestEnum.V2])
+ instance3 = EnumThing.findAllByEnInList([TestEnum.V3])
+
+ then:
+ instance1.size() == 2
+ instance1.every { it.en == TestEnum.V1 }
+
+ instance2.size() == 1
+ instance2.every { it.en == TestEnum.V2 }
+
+ instance3.isEmpty()
+ }
+
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
+ void "Test findAllBy()"() {
+ given:
+
+ new EnumThing(name: 'e1', en: TestEnum.V1).save(failOnError: true)
+ new EnumThing(name: 'e2', en: TestEnum.V1).save(failOnError: true)
+ new EnumThing(name: 'e3', en: TestEnum.V2).save(failOnError: true)
+
+ List instance1
+ List instance2
+ List instance3
+
+ when:
+ instance1 = EnumThing.findAllByEn(TestEnum.V1)
+ instance2 = EnumThing.findAllByEn(TestEnum.V2)
+ instance3 = EnumThing.findAllByEn(TestEnum.V3)
+
+ then:
+ instance1.size() == 2
+ instance1.every { it.en == TestEnum.V1 }
+
+ instance2.size() == 1
+ instance2.every { it.en == TestEnum.V2 }
+
+ instance3.isEmpty()
+
+ }
+
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
+ void "Test findAllBy() with clearing the session"() {
+ given:
+
+ new EnumThing(name: 'e1', en: TestEnum.V1).save(failOnError: true,
flush: true)
+ new EnumThing(name: 'e2', en: TestEnum.V1).save(failOnError: true,
flush: true)
+ new EnumThing(name: 'e3', en: TestEnum.V2).save(failOnError: true,
flush: true)
+ manager.session.clear()
+
+ List instance1
+ List instance2
+ List instance3
+
+ when:
+ instance1 = EnumThing.findAllByEn(TestEnum.V1)
+ instance2 = EnumThing.findAllByEn(TestEnum.V2)
+ instance3 = EnumThing.findAllByEn(TestEnum.V3)
+
+ then:
+ instance1.size() == 2
+ instance1.every { it.en == TestEnum.V1 }
+
+ instance2.size() == 1
+ instance2.every { it.en == TestEnum.V2 }
+
+ instance3.isEmpty()
+ }
+
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/NotNullQuerySpec.groovy:
##########
@@ -24,9 +24,10 @@ import
org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
class NotNullQuerySpec extends GrailsDataTckSpec<GrailsDataCoreTckManager> {
void setupSpec() {
- manager.domainClasses.addAll([NullMe, NullOther])
+ manager.addAllDomainClasses([NullMe, NullOther])
}
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/NotNullQuerySpec.groovy:
##########
@@ -67,6 +68,7 @@ class NotNullQuerySpec extends
GrailsDataTckSpec<GrailsDataCoreTckManager> {
results[0].name == "Bob"
}
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/AsyncReadMethodsSpec.groovy:
##########
@@ -69,6 +71,7 @@ class AsyncReadMethodsSpec extends
GrailsDataTckSpec<GrailsDataCoreTckManager> {
}
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/EmbeddedPropertyQuerySpec.groovy:
##########
@@ -38,6 +43,9 @@ class EmbeddedPropertyQuerySpec extends
GrailsDataTckSpec<GrailsDataCoreTckManag
book != null
}
+ @Requires({ System.getProperty('hibernate5.gorm.suite') == 'true' ||
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/PersistenceEventListenerSpec.groovy:
##########
@@ -195,6 +196,7 @@ class PersistenceEventListenerSpec extends
GrailsDataTckSpec {
1 == listener.PostLoadCount
}
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/OrderBySpec.groovy:
##########
@@ -18,16 +18,23 @@
*/
package org.apache.grails.data.testing.tck.tests
-import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
+import spock.lang.IgnoreIf
+
import org.apache.grails.data.testing.tck.domains.ChildEntity
import org.apache.grails.data.testing.tck.domains.TestEntity
+import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
/**
* Abstract base test for order by queries. Subclasses should do the necessary
setup to configure GORM
*/
+@IgnoreIf({ System.getProperty('core.gorm.suite') == 'true' })
Review Comment:
Why did we have a regression on core.gorm.suite here?
##########
grails-datamapping-core-test/src/test/groovy/grails/gorm/tests/WhereMethodSpec.groovy:
##########
@@ -1502,6 +1503,7 @@ class Project {
results.find { it.firstName == 'Fred' }
}
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-core-test/src/test/groovy/grails/gorm/services/ServiceImplSpec.groovy:
##########
@@ -326,6 +329,7 @@ class ServiceImplSpec extends Specification {
}
+ @Requires({ System.getProperty('hibernate5.gorm.suite') == 'true' ||
System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-core-test/src/test/groovy/grails/gorm/tests/WhereMethodSpec.groovy:
##########
@@ -238,6 +238,7 @@ class Project {
}
@Issue('GRAILS-8256')
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/PagedResultSpecHibernate.groovy:
##########
@@ -0,0 +1,132 @@
+/*
+ * 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 org.apache.grails.data.testing.tck.tests
+
+import spock.lang.IgnoreIf
+
+import org.apache.grails.data.testing.tck.domains.Person
+import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
+
+@IgnoreIf({
+ System.getProperty('mongodb.gorm.suite') == 'true' ||
+ System.getProperty('hibernate5.gorm.suite') == 'true' ||
+ System.getProperty('core.gorm.suite') == 'true'
+})
+class PagedResultSpecHibernate extends GrailsDataTckSpec {
Review Comment:
This class doesn't look specific to hibernate and it's in core. why does it
work with hiberate5 but not 7? Should the IgnoreIf be removed?
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/EmbeddedPropertyQuerySpec.groovy:
##########
@@ -95,6 +115,9 @@ class EmbeddedPropertyQuerySpec extends
GrailsDataTckSpec<GrailsDataCoreTckManag
book != null
}
+ @Requires({ System.getProperty('hibernate5.gorm.suite') == 'true' ||
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/RangeQuerySpec.groovy:
##########
@@ -18,20 +18,24 @@
*/
package org.apache.grails.data.testing.tck.tests
-import groovy.time.TimeCategory
-
-import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
import org.apache.grails.data.testing.tck.domains.ChildEntity
import org.apache.grails.data.testing.tck.domains.Person
import org.apache.grails.data.testing.tck.domains.Publication
import org.apache.grails.data.testing.tck.domains.TestEntity
+import groovy.time.TimeCategory
+import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
/**
* Abstract base test for querying ranges. Subclasses should do the necessary
setup to configure GORM
*/
class RangeQuerySpec extends GrailsDataTckSpec {
- void 'Test between query with dates'() {
+ void setupSpec() {
+ manager.addAllDomainClasses([Publication, TestEntity, Person,
ChildEntity])
+ }
+
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/SizeQuerySpecHibernate.groovy:
##########
@@ -0,0 +1,193 @@
+/*
+ * 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 org.apache.grails.data.testing.tck.tests
+
+import spock.lang.IgnoreIf
+
+import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
+import org.apache.grails.data.testing.tck.domains.Child_BT_Default_P
+import org.apache.grails.data.testing.tck.domains.Owner_Default_Bi_P
+import spock.lang.Unroll
+
+/**
+ * Tests for querying the size of collections etc.
+ */
+@IgnoreIf({ System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Mongo doesn't support this? Shouldn't this at least be PendingFeatureIf?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/PagedResultSpec.groovy:
##########
@@ -18,12 +18,17 @@
*/
package org.apache.grails.data.testing.tck.tests
-import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
import org.apache.grails.data.testing.tck.domains.Person
+import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
[email protected]({ System.getProperty('hibernate5.gorm.suite') == 'true'
|| System.getProperty('mongodb.gorm.suite') == 'true' ||
System.getProperty('core.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-core-test/src/test/groovy/grails/gorm/services/ServiceImplSpec.groovy:
##########
@@ -56,6 +58,7 @@ class ServiceImplSpec extends Specification {
}
+ @Requires({ System.getProperty('hibernate5.gorm.suite') == 'true' ||
System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/RangeQuerySpec.groovy:
##########
@@ -50,7 +54,8 @@ class RangeQuerySpec extends GrailsDataTckSpec {
results.size() == 2
}
- void 'Test between query'() {
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/EmbeddedPropertyQuerySpec.groovy:
##########
@@ -49,6 +57,9 @@ class EmbeddedPropertyQuerySpec extends
GrailsDataTckSpec<GrailsDataCoreTckManag
book != null
}
+ @Requires({ System.getProperty('hibernate5.gorm.suite') == 'true' ||
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-data-hibernate7/core/src/test/groovy/grails/gorm/specs/CompositeIdWithJoinTableSpec.groovy:
##########
@@ -0,0 +1,102 @@
+/*
+ * 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.gorm.specs
+
+import static grails.gorm.hibernate.mapping.MappingBuilder.define
+
+import grails.gorm.annotation.Entity
+import org.jetbrains.annotations.NotNull
+import grails.gorm.transactions.Rollback
+import org.grails.orm.hibernate.HibernateDatastore
+import org.springframework.transaction.PlatformTransactionManager
+import spock.lang.AutoCleanup
+import spock.lang.Shared
+import spock.lang.Specification
+
+import static grails.gorm.hibernate.mapping.MappingBuilder.define
+
+/**
+ * Created by graemerocher on 26/01/2017.
+ */
+//TODO: Failing at MappingModelCreationHelper line 1223
Review Comment:
Remove the TODO Comments?
##########
grails-datamapping-core-test/src/test/groovy/grails/gorm/tests/DeepValidateWithSaveSpec.groovy:
##########
@@ -26,6 +26,7 @@ import org.grails.datastore.gorm.validation.CascadingValidator
class DeepValidateWithSaveSpec extends
GrailsDataTckSpec<GrailsDataCoreTckManager> {
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/ListOrderByHungarianNotationSpec.groovy:
##########
@@ -27,9 +27,10 @@ import
org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
*/
class ListOrderByHungarianNotationSpec extends
GrailsDataTckSpec<GrailsDataCoreTckManager> {
void setupSpec() {
- manager.domainClasses.addAll([ClassWithHungarianNotation])
+ manager.addAllDomainClasses([ClassWithHungarianNotation])
}
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/EmbeddedPropertyQuerySpec.groovy:
##########
@@ -82,6 +99,9 @@ class EmbeddedPropertyQuerySpec extends
GrailsDataTckSpec<GrailsDataCoreTckManag
book != null
}
+ @Requires({ System.getProperty('hibernate5.gorm.suite') == 'true' ||
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/AsyncReadMethodsSpec.groovy:
##########
@@ -48,6 +49,7 @@ class AsyncReadMethodsSpec extends
GrailsDataTckSpec<GrailsDataCoreTckManager> {
results[2].firstName == "Barney"
}
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/UpdateWithProxyPresentSpec.groovy:
##########
@@ -28,10 +30,11 @@ import org.apache.grails.data.testing.tck.domains.PetType
/**
* @author graemerocher
*/
+@IgnoreIf({ System.getProperty('hibernate7.gorm.suite') == 'true' })
Review Comment:
Is this because there's a specific test that implements this in hibernate7
or is this a regression?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/SizeQuerySpec.groovy:
##########
@@ -18,220 +18,190 @@
*/
package org.apache.grails.data.testing.tck.tests
-import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
-import org.apache.grails.data.testing.tck.domains.Country
+import org.apache.grails.data.testing.tck.domains.SimpleCountry
import org.apache.grails.data.testing.tck.domains.Person
+import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
+import spock.lang.IgnoreIf
/**
* Tests for querying the size of collections etc.
*/
+@IgnoreIf({ System.getProperty('hibernate5.gorm.suite') == 'true' ||
System.getProperty('hibernate7.gorm.suite') == 'true' })
Review Comment:
This looks like a regression to me - why was it supported before but not now?
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/CustomAutoTimestampSpec.groovy:
##########
@@ -27,9 +30,12 @@ import
org.grails.datastore.gorm.events.AutoTimestampEventListener
class CustomAutoTimestampSpec extends
GrailsDataTckSpec<GrailsDataCoreTckManager> {
void setupSpec() {
- manager.domainClasses.addAll([AutoTimestampedChildEntity,
AutoTimestampedParentEntity, Image, RecordCustom, RecordWithAliases])
+ manager.addAllDomainClasses([AutoTimestampedChildEntity,
AutoTimestampedParentEntity, Image, RecordCustom, RecordWithAliases])
}
+ @Requires({ System.getProperty('hibernate5.gorm.suite') == 'true' ||
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/AsyncReadMethodsSpec.groovy:
##########
@@ -98,6 +101,7 @@ class AsyncReadMethodsSpec extends
GrailsDataTckSpec<GrailsDataCoreTckManager> {
results[2].firstName == "Barney"
}
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/RLikeSpec.groovy:
##########
@@ -0,0 +1,52 @@
+/*
+ * 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 org.apache.grails.data.testing.tck.tests
+
+import grails.gorm.annotation.Entity
+import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
+import spock.lang.IgnoreIf
+
+@IgnoreIf({ System.getProperty('hibernate7.gorm.suite') == 'true' })
+class RLikeSpec extends GrailsDataTckSpec {
+
+ void setupSpec() {
+ manager.addAllDomainClasses([RlikeFoo])
+ }
+
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
So you have ignore if on the class, but then not here? Why doesn't the core
/ simple repo support this? Can we remove the requires? Graphql may already
support this - but this annotation will prevent us testing that.
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/EmbeddedPropertyQuerySpec.groovy:
##########
@@ -60,6 +71,9 @@ class EmbeddedPropertyQuerySpec extends
GrailsDataTckSpec<GrailsDataCoreTckManag
book != null
}
+ @Requires({ System.getProperty('hibernate5.gorm.suite') == 'true' ||
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-data-mongodb/docs/build.gradle:
##########
@@ -63,7 +63,12 @@ dependencies {
}
rootProject.subprojects
.findAll { it.findProperty('gormApiDocs') }
- .each { documentation project(":$it.name") }
+ .each {
+ // TODO: This needs fixed for hibernate 7, but with the
hibernate version conflicts, we may need to do separate documentation publishing
Review Comment:
I'm not sure what to do on this one. @jamesfredley should we just abandon
the grails gorm docs for specific implementations and try to publish this all
in the root?
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/AsyncReadMethodsSpec.groovy:
##########
@@ -28,6 +28,7 @@ import
org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
*/
class AsyncReadMethodsSpec extends GrailsDataTckSpec<GrailsDataCoreTckManager>
{
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/core/connections/ConnectionSource.java:
##########
@@ -31,7 +31,14 @@ public interface ConnectionSource<T, S extends
ConnectionSourceSettings> extends
/**
* The name of the default connection source
*/
- String DEFAULT = "DEFAULT";
+ String DEFAULT = "default";
Review Comment:
I'm ok with the change, but why change the default datastore name?
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/EmbeddedPropertyQuerySpec.groovy:
##########
@@ -18,15 +18,20 @@
*/
package org.grails.datastore.gorm
+import spock.lang.Requires
+
import grails.persistence.Entity
import org.apache.grails.data.simple.core.GrailsDataCoreTckManager
import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
class EmbeddedPropertyQuerySpec extends
GrailsDataTckSpec<GrailsDataCoreTckManager> {
void setupSpec() {
- manager.domainClasses += [Book2, Author2]
+ manager.addAllDomainClasses([Book2, Author2])
}
+ @Requires({ System.getProperty('hibernate5.gorm.suite') == 'true' ||
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/RangeQuerySpec.groovy:
##########
@@ -78,7 +83,8 @@ class RangeQuerySpec extends GrailsDataTckSpec {
4 == results.size()
}
- void 'Test greater than or equal to and less than or equal to queries'() {
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-tck/src/main/groovy/org/apache/grails/data/testing/tck/tests/PersistenceEventListenerSpec.groovy:
##########
@@ -83,6 +83,7 @@ class PersistenceEventListenerSpec extends GrailsDataTckSpec {
listener.events[-2] instanceof PreDeleteEvent
}
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/DistinctProjectionSpec.groovy:
##########
@@ -24,6 +24,7 @@ import
org.apache.grails.data.testing.tck.base.GrailsDataTckSpec
class DistinctProjectionSpec extends
GrailsDataTckSpec<GrailsDataCoreTckManager> {
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/CoreTestSuite.groovy:
##########
@@ -29,5 +29,6 @@ import org.junit.platform.suite.api.Suite
*/
@Suite
@SelectClasses([NotInListSpec])
[email protected]({ System.getProperty('core.gorm.suite') == 'true' })
Review Comment:
The purpose of this class is to run specific tests from the TCK so it should
never be ignored. By default we picked a passing test (NotInListSpec). It
seems we may have a major regression here.
##########
grails-datamapping-core-test/src/test/groovy/org/grails/datastore/gorm/EmbeddedPropertyQuerySpec.groovy:
##########
@@ -71,6 +85,9 @@ class EmbeddedPropertyQuerySpec extends
GrailsDataTckSpec<GrailsDataCoreTckManag
book != null
}
+ @Requires({ System.getProperty('hibernate5.gorm.suite') == 'true' ||
Review Comment:
Regression - we should revert adding the Requires annotation?
##########
grails-datamapping-core-test/src/test/groovy/grails/gorm/tests/DeepValidateWithSaveSpec.groovy:
##########
@@ -41,6 +42,7 @@ class DeepValidateWithSaveSpec extends
GrailsDataTckSpec<GrailsDataCoreTckManage
1 * mockValidator.validate(entity, _, true)
}
+ @spock.lang.Requires({ System.getProperty('hibernate5.gorm.suite') ==
'true' || System.getProperty('hibernate7.gorm.suite') == 'true' ||
System.getProperty('mongodb.gorm.suite') == 'true' })
Review Comment:
Regression - we should revert adding the Requires annotation?
--
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]
