Copilot commented on code in PR #15773: URL: https://github.com/apache/grails-core/pull/15773#discussion_r3484195231
########## grails-data-simple/src/test/groovy/org/grails/datastore/mapping/simple/engine/SimpleMapEntityPersisterSpec.groovy: ########## @@ -0,0 +1,190 @@ +/* + * 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 AS + * + * 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 AS 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: The Apache license header at the top of this new spec is corrupted/duplicated (eg "regarding copyright ownership. The AS") which breaks the required ASF header format for new source files. ########## grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/PlaceWithExceptionTest.groovy: ########## @@ -0,0 +1,69 @@ +/* + * 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.datastore.gorm.mongo + +import org.apache.grails.data.mongo.core.GrailsDataMongoTckManager +import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec +import grails.mongodb.geo.* +import grails.persistence.Entity + +@Entity +class PlaceException { + Long id + String name + Point point + Polygon polygon + + static mapping = { + point geoIndex: '2dsphere' + } +} + +class PlaceWithExceptionTest extends GrailsDataTckSpec<GrailsDataMongoTckManager> { + void setupSpec() { + manager.registerDomainClasses(PlaceException) + } + + void "test place with exception handling"() { + when: + def col = new GeometryCollection() + col << Point.valueOf(5, 10) + def p = new PlaceException(name: "Test") // Don't set any GeoJSON fields + p.save(flush: true, validate: false) + println "Saved with id: ${p.id}" + manager.session.clear() + + then: + p.id != null + + when: + try { + p = PlaceException.get(p.id) + println "Retrieved successfully: ${p}" + } catch (Exception e) { + println "Exception during get: ${e.class.name}" + println "Message: ${e.message}" + e.printStackTrace(System.out) + throw e + } Review Comment: This test includes debug `println` statements and a try/catch that prints stack traces before rethrowing. That adds noise to CI logs and doesn't add coverage because Spock will already report the exception; it can be simplified to a direct get() call without printing. ########## grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/PlacePartialTest.groovy: ########## @@ -0,0 +1,72 @@ +/* + * 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.datastore.gorm.mongo + +import org.apache.grails.data.mongo.core.GrailsDataMongoTckManager +import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec +import grails.mongodb.geo.* +import grails.persistence.Entity + +@Entity +class PlacePartial { + Long id + String name + Point point + Polygon polygon + LineString lineString + Box box + Circle circle + Sphere sphere + MultiPoint multiPoint + MultiLineString multiLineString + MultiPolygon multiPolygon + GeometryCollection geometryCollection + + static mapping = { + point geoIndex: '2dsphere' + } +} + +class PlacePartialTest extends GrailsDataTckSpec<GrailsDataMongoTckManager> { + void setupSpec() { + manager.registerDomainClasses(PlacePartial) + } + + void "test place with only one field"() { + when: + def col = new GeometryCollection() + col << Point.valueOf(5, 10) + def p = new PlacePartial(geometryCollection: col) + println "Saving with only geometryCollection set..." + p.save(flush: true, validate: false) + println "Saved with id: ${p.id}" + manager.session.clear() Review Comment: Avoid `println` debugging in committed tests; it adds noise in CI logs. The save+clear sequence works without printing. ########## grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/PlacePartialTest.groovy: ########## @@ -0,0 +1,72 @@ +/* + * 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.datastore.gorm.mongo + +import org.apache.grails.data.mongo.core.GrailsDataMongoTckManager +import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec +import grails.mongodb.geo.* +import grails.persistence.Entity + +@Entity +class PlacePartial { + Long id + String name + Point point + Polygon polygon + LineString lineString + Box box + Circle circle + Sphere sphere + MultiPoint multiPoint + MultiLineString multiLineString + MultiPolygon multiPolygon + GeometryCollection geometryCollection + + static mapping = { + point geoIndex: '2dsphere' + } +} + +class PlacePartialTest extends GrailsDataTckSpec<GrailsDataMongoTckManager> { + void setupSpec() { + manager.registerDomainClasses(PlacePartial) + } + + void "test place with only one field"() { + when: + def col = new GeometryCollection() + col << Point.valueOf(5, 10) + def p = new PlacePartial(geometryCollection: col) + println "Saving with only geometryCollection set..." + p.save(flush: true, validate: false) + println "Saved with id: ${p.id}" + manager.session.clear() + + then: + p.id != null + + when: + println "Retrieving..." + p = PlacePartial.get(p.id) + println "Retrieved: ${p}" Review Comment: Avoid `println` debugging in committed tests; it adds noise in CI logs. The retrieval assertion works without printing. ########## grails-data-hibernate5/core/src/test/groovy/org/grails/orm/hibernate/Hibernate5TenantContextProfilingSpec.groovy: ########## @@ -0,0 +1,108 @@ +/* + * 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 grails.gorm.MultiTenant +import grails.gorm.multitenancy.Tenants +import org.grails.datastore.gorm.GormRegistry +import org.grails.datastore.gorm.multitenancy.TenantDelegatingGormOperations +import org.grails.datastore.mapping.multitenancy.MultiTenantCapableDatastore +import org.grails.datastore.mapping.multitenancy.MultiTenancySettings +import org.grails.datastore.mapping.model.MappingContext +import spock.lang.Specification + +class Hibernate5TenantContextProfilingSpec extends Specification { Review Comment: This profiling spec always passes (`true`) and prints timing to stdout, which makes it unsuitable for the automated test suite. Consider marking it `@Ignore` (or moving it to a benchmark module) so it doesn't run in CI by default. ########## grails-data-neo4j/grails-datastore-gorm-neo4j/src/test/groovy/org/grails/datastore/gorm/neo4j/Neo4jTenantContextProfilingSpec.groovy: ########## @@ -0,0 +1,113 @@ +/* + * 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.datastore.gorm.neo4j + +import grails.gorm.MultiTenant +import grails.gorm.multitenancy.Tenants +import org.grails.datastore.gorm.GormRegistry +import org.grails.datastore.gorm.DatastoreResolver +import org.grails.datastore.gorm.multitenancy.TenantDelegatingGormOperations +import org.grails.datastore.mapping.core.Datastore +import org.grails.datastore.mapping.multitenancy.MultiTenantCapableDatastore +import org.grails.datastore.mapping.multitenancy.MultiTenancySettings +import org.grails.datastore.mapping.model.MappingContext +import org.grails.datastore.gorm.neo4j.api.Neo4jGormStaticApi +import spock.lang.Specification + +class Neo4jTenantContextProfilingSpec extends Specification { Review Comment: This profiling spec always passes (`true`) and prints timing to stdout, which makes it unsuitable for the automated test suite. Consider marking it `@Ignore` (or moving it to a benchmark module) so it doesn't run in CI by default. ########## grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/api/MongoTenantContextProfilingSpec.groovy: ########## @@ -0,0 +1,151 @@ +/* + * 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.datastore.gorm.mongo.api + +import grails.gorm.MultiTenant +import grails.gorm.multitenancy.Tenants +import org.grails.datastore.gorm.GormRegistry +import org.grails.datastore.gorm.DatastoreResolver +import org.grails.datastore.gorm.multitenancy.TenantDelegatingGormOperations +import org.grails.datastore.mapping.core.Datastore +import org.grails.datastore.mapping.multitenancy.MultiTenantCapableDatastore +import org.grails.datastore.mapping.multitenancy.MultiTenancySettings +import org.grails.datastore.mapping.model.MappingContext +import org.bson.Document +import spock.lang.Specification + +class MongoTenantContextProfilingSpec extends Specification { Review Comment: This profiling spec always passes (`true`) and prints timing to stdout, which makes it unsuitable for the automated test suite. Consider marking it `@Ignore` (or moving it to a benchmark module) so it doesn't run in CI by default. ########## grails-data-graphql/core/src/test/groovy/org/grails/gorm/graphql/GraphqlTenantContextProfilingSpec.groovy: ########## @@ -0,0 +1,53 @@ +/* + * 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.gorm.graphql + +import spock.lang.Specification + +import grails.gorm.multitenancy.Tenants +import org.grails.datastore.mapping.multitenancy.MultiTenantCapableDatastore +import org.grails.datastore.mapping.multitenancy.MultiTenancySettings + +class GraphqlTenantContextProfilingSpec extends Specification { Review Comment: This spec is a placeholder/profiling loop that always passes and prints to stdout. It should be `@Ignore` (or moved out of the main test suite) so it doesn't run in CI by default. ########## grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/support/ClosureEventTriggeringInterceptor.java: ########## @@ -139,26 +139,83 @@ public boolean onPreInsert(PreInsertEvent hibernateEvent) { } private void synchronizeHibernateState(PreInsertEvent hibernateEvent, ModificationTrackingEntityAccess entityAccess) { - Map<String, Object> modifiedProperties = entityAccess.getModifiedProperties(); + Object[] state = hibernateEvent.getState(); + EntityPersister persister = hibernateEvent.getPersister(); + Map<String, Object> modifiedProperties = findModifiedProperties(hibernateEvent.getEntity(), persister, state); + modifiedProperties.putAll(entityAccess.getModifiedProperties()); + if (!modifiedProperties.isEmpty()) { - Object[] state = hibernateEvent.getState(); - EntityPersister persister = hibernateEvent.getPersister(); synchronizeHibernateState(persister, state, modifiedProperties); } } private void synchronizeHibernateState(PreUpdateEvent hibernateEvent, ModificationTrackingEntityAccess entityAccess, boolean autoTimestamp) { - Map<String, Object> modifiedProperties = entityAccess.getModifiedProperties(); + Object[] state = hibernateEvent.getState(); + EntityPersister persister = hibernateEvent.getPersister(); + Map<String, Object> modifiedProperties = findModifiedProperties(hibernateEvent.getEntity(), persister, state); + modifiedProperties.putAll(entityAccess.getModifiedProperties()); if (autoTimestamp) { updateModifiedPropertiesWithAutoTimestamp(modifiedProperties, hibernateEvent); } if (!modifiedProperties.isEmpty()) { - Object[] state = hibernateEvent.getState(); - EntityPersister persister = hibernateEvent.getPersister(); synchronizeHibernateState(persister, state, modifiedProperties); + + // Synchronize with ActionQueue for Hibernate 5 EntityUpdateAction + try { + java.lang.reflect.Field actionQueueUpdatesField = org.springframework.util.ReflectionUtils.findField(org.hibernate.engine.spi.ActionQueue.class, "updates"); + if (actionQueueUpdatesField != null) { + actionQueueUpdatesField.setAccessible(true); + org.hibernate.engine.spi.ExecutableList<org.hibernate.action.internal.EntityUpdateAction> updates = (org.hibernate.engine.spi.ExecutableList<org.hibernate.action.internal.EntityUpdateAction>) actionQueueUpdatesField.get(hibernateEvent.getSession().getActionQueue()); + if (updates != null) { + java.lang.reflect.Field entityUpdateActionStateField = org.springframework.util.ReflectionUtils.findField(org.hibernate.action.internal.EntityUpdateAction.class, "state"); + if (entityUpdateActionStateField != null) { + entityUpdateActionStateField.setAccessible(true); + for (org.hibernate.action.internal.EntityUpdateAction updateAction : updates) { + if (updateAction.getInstance() == hibernateEvent.getEntity()) { + Object[] updateState = (Object[]) entityUpdateActionStateField.get(updateAction); + if (updateState != null) { + org.hibernate.tuple.entity.EntityMetamodel entityMetamodel = persister.getEntityMetamodel(); + for (Map.Entry<String, Object> entry : modifiedProperties.entrySet()) { + Integer index = entityMetamodel.getPropertyIndexOrNull(entry.getKey()); + if (index != null) { + updateState[index] = entry.getValue(); + } + } + } + } + } + } + } + } + } catch (Exception e) { + // Ignore + } + } + } + + private Map<String, Object> findModifiedProperties(Object entity, EntityPersister persister, Object[] state) { + Map<String, Object> modifiedProperties = new java.util.HashMap<>(); + PersistentEntity persistentEntity = mappingContext.getPersistentEntity(Hibernate.getClass(entity).getName()); + if (persistentEntity != null) { + org.grails.datastore.mapping.reflect.EntityReflector reflector = persistentEntity.getReflector(); + org.hibernate.tuple.entity.EntityMetamodel entityMetamodel = persister.getEntityMetamodel(); + for (String propertyName : persister.getPropertyNames()) { + if ("version".equals(propertyName)) continue; + Integer index = entityMetamodel.getPropertyIndexOrNull(propertyName); + if (index != null) { + org.grails.datastore.mapping.model.PersistentProperty property = persistentEntity.getPropertyByName(propertyName); + if (property != null) { + Object value = reflector.getProperty(entity, propertyName); + if (state[index] != value) { + modifiedProperties.put(propertyName, value); + } + } Review Comment: `findModifiedProperties` compares values using reference inequality (`state[index] != value`). For most property types this will mark equal-but-distinct values as modified (or miss changes for mutable values with the same reference). Use `Objects.equals` for a semantic comparison. ########## grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormApiFactory.groovy: ########## @@ -0,0 +1,71 @@ +/* + * 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 groovy.transform.CompileStatic + +import org.grails.datastore.gorm.DefaultGormApiFactory +import org.grails.datastore.gorm.DatastoreResolver +import org.grails.datastore.gorm.GormInstanceApi +import org.grails.datastore.gorm.GormRegistry +import org.grails.datastore.gorm.GormStaticApi +import org.grails.datastore.gorm.GormValidationApi +import org.grails.datastore.mapping.model.MappingContext + +/** + * Hibernate-specific factory for creating GORM API objects. + * Creates Hibernate-specific API implementations (HibernateGormStaticApi, etc.) + * instead of generic GORM APIs. + * + * @since 8.0.0 + */ +@CompileStatic +class HibernateGormApiFactory extends DefaultGormApiFactory { + + @Override + <D> GormStaticApi<D> createStaticApi(Class<D> persistentClass, + MappingContext mappingContext, + DatastoreResolver resolver, + String qualifier, + GormRegistry registry) { + def finders = createDynamicFinders(resolver, mappingContext) + return new HibernateGormStaticApi<D>(persistentClass, mappingContext, finders, resolver, qualifier, persistentClass.classLoader) + } + + @Override + <D> GormInstanceApi<D> createInstanceApi(Class<D> persistentClass, + MappingContext mappingContext, + DatastoreResolver resolver, + GormRegistry registry, + boolean failOnError, + boolean markDirty) { + GormInstanceApi<D> instanceApi = new HibernateGormInstanceApi<D>(persistentClass, mappingContext, resolver, persistentClass.classLoader) + instanceApi.failOnError = failOnError + instanceApi.markDirty = markDirty + return instanceApi + } + + @Override + <D> GormValidationApi<D> createValidationApi(Class<D> persistentClass, + MappingContext mappingContext, + DatastoreResolver resolver, + GormRegistry registry) { + return new GormValidationApi<D>(persistentClass, mappingContext, resolver) Review Comment: `createValidationApi` currently returns the generic `GormValidationApi`, which bypasses the Hibernate-specific validation behavior implemented in `HibernateGormValidationApi` (manual flush handling, events, deepValidate/evict args, etc.). This will likely change validation semantics for Hibernate 5 entities. ########## grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/cfg/GrailsDomainBinder.java: ########## @@ -1494,7 +1494,7 @@ protected void addMultiTenantFilterIfNecessary( mappings.addFilterDefinition(new FilterDefinition( GormProperties.TENANT_IDENTITY, filterCondition, - Collections.singletonMap(GormProperties.TENANT_IDENTITY, getProperty(persistentClass, tenantId.getName()).getType()) + Collections.singletonMap(GormProperties.TENANT_IDENTITY, org.hibernate.type.StringType.INSTANCE) )); Review Comment: The multi-tenant filter definition now hard-codes the `tenantId` parameter type to `StringType`. That will break discriminator multi-tenancy when the tenant id property is not a `String` (eg `Long`, `UUID`), since Hibernate uses this type for binding and SQL generation. -- 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]
