diff --git a/.github/workflows/gradle.yml b/.github/workflows/gradle.yml index 8a3b2ab4452..7f39c1ada66 100644 --- a/.github/workflows/gradle.yml +++ b/.github/workflows/gradle.yml @@ -371,6 +371,9 @@ jobs: include-hidden-files: true path: grails-forge/tmp1/cli/**/* if-no-files-found: 'error' + # Every test job from here through neo4jFunctional sets timeout-minutes to twice the slowest run that + # finished on its own, passed or failed, over the 160 CI runs of 2026-09-03 to 2026-09-13, rounded up to + # the half hour. Without a limit, a test JVM that never exits holds its job for GitHub's six-hour default. functional: name: "Functional Tests (${{ matrix.job_name }})" if: ${{ !contains(github.event.head_commit.message, '[skip tests]') }} @@ -527,6 +530,7 @@ jobs: security: name: "Spring Security Tests (${{ matrix.job_name }})" if: ${{ !contains(github.event.head_commit.message, '[skip tests]') }} + timeout-minutes: 150 # slowest run: 71 min strategy: fail-fast: false matrix: @@ -572,6 +576,7 @@ jobs: securityConfig: name: "Spring Security Config Functional Tests" if: ${{ !contains(github.event.head_commit.message, '[skip tests]') }} + timeout-minutes: 90 # slowest run: 39 min strategy: fail-fast: false matrix: @@ -611,6 +616,7 @@ jobs: casSecurityConfig: name: "Spring Security CAS Functional Tests" if: ${{ !contains(github.event.head_commit.message, '[skip tests]') }} + timeout-minutes: 90 # slowest run: 35 min strategy: fail-fast: false matrix: @@ -652,6 +658,7 @@ jobs: redis: name: "Redis Tests" if: ${{ !contains(github.event.head_commit.message, '[skip tests]') }} + timeout-minutes: 180 # slowest run: 86 min runs-on: ubuntu-24.04 strategy: fail-fast: false @@ -687,6 +694,7 @@ jobs: mongodbFunctional: if: ${{ !contains(github.event.head_commit.message, '[skip tests]') }} name: "Mongodb Functional Tests (Java ${{ matrix.java }}, MongoDB ${{ matrix.mongodb-version }}, indy=${{ matrix.indy }})" + timeout-minutes: 150 # slowest run: 70 min runs-on: ubuntu-24.04 strategy: fail-fast: false @@ -750,6 +758,7 @@ jobs: hibernate5Functional: if: ${{ !contains(github.event.head_commit.message, '[skip tests]') }} name: "Hibernate5 Functional Tests (Java ${{ matrix.java }}, indy=${{ matrix.indy }})" + timeout-minutes: 120 # slowest run: 58 min runs-on: ubuntu-24.04 strategy: fail-fast: false @@ -799,6 +808,7 @@ jobs: hibernate7Functional: if: ${{ !contains(github.event.head_commit.message, '[skip tests]') }} name: "Hibernate7 Functional Tests (${{ matrix.job_name }})" + timeout-minutes: 210 # slowest run: 104 min, which failed; the slowest that passed took 86 runs-on: ubuntu-24.04 strategy: fail-fast: false @@ -869,6 +879,7 @@ jobs: neo4jFunctional: if: ${{ !contains(github.event.head_commit.message, '[skip tests]') }} name: "Neo4j Functional Tests (Java ${{ matrix.java }}, indy=${{ matrix.indy }})" + timeout-minutes: 180 # slowest run: 90 min runs-on: ubuntu-24.04 timeout-minutes: 105 strategy: diff --git a/gradle/hibernate5-test-config.gradle b/gradle/hibernate5-test-config.gradle index 1cf18720583..7261139fb58 100644 --- a/gradle/hibernate5-test-config.gradle +++ b/gradle/hibernate5-test-config.gradle @@ -40,6 +40,7 @@ tasks.withType(Test).configureEach { 'onlyHibernate7Tests', 'skipHibernate5Tests', 'onlyMongodbTests', + 'onlyNeo4jTests', 'onlyRedisTests', 'onlySpringSecurityTests', 'onlyCoreTests', diff --git a/gradle/hibernate7-test-config.gradle b/gradle/hibernate7-test-config.gradle index 6e5a16ff982..37bc27a0da2 100644 --- a/gradle/hibernate7-test-config.gradle +++ b/gradle/hibernate7-test-config.gradle @@ -34,6 +34,7 @@ tasks.withType(Test).configureEach { 'onlyHibernate5Tests', 'skipHibernate7Tests', 'onlyMongodbTests', + 'onlyNeo4jTests', 'onlyRedisTests', 'onlySpringSecurityTests', 'onlyCoreTests', diff --git a/gradle/mongodb-test-config.gradle b/gradle/mongodb-test-config.gradle index abb4cbb727b..46d3259c920 100644 --- a/gradle/mongodb-test-config.gradle +++ b/gradle/mongodb-test-config.gradle @@ -44,6 +44,7 @@ tasks.withType(Test).configureEach { 'onlyHibernate5Tests', 'onlyHibernate7Tests', 'skipMongodbTests', + 'onlyNeo4jTests', 'onlyRedisTests', 'onlySpringSecurityTests', 'onlyCoreTests', diff --git a/gradle/redis-test-config.gradle b/gradle/redis-test-config.gradle index 2d861ea005c..b7c99015d76 100644 --- a/gradle/redis-test-config.gradle +++ b/gradle/redis-test-config.gradle @@ -30,6 +30,7 @@ tasks.withType(Test).configureEach { 'onlyHibernate5Tests', 'onlyHibernate7Tests', 'onlyMongodbTests', + 'onlyNeo4jTests', 'onlySpringSecurityTests', 'onlyCoreTests', 'skipTests' diff --git a/gradle/spring-security-test-config.gradle b/gradle/spring-security-test-config.gradle index 37d5261b5d7..8122e9f4f39 100644 --- a/gradle/spring-security-test-config.gradle +++ b/gradle/spring-security-test-config.gradle @@ -30,6 +30,7 @@ tasks.withType(Test).configureEach { 'onlyHibernate5Tests', 'onlyHibernate7Tests', 'onlyMongodbTests', + 'onlyNeo4jTests', 'onlyRedisTests', 'onlyCoreTests', 'skipTests' diff --git a/grails-data-graphql/core/src/main/groovy/org/grails/gorm/graphql/Schema.groovy b/grails-data-graphql/core/src/main/groovy/org/grails/gorm/graphql/Schema.groovy index 7d583af2eef..14d702a6fdd 100644 --- a/grails-data-graphql/core/src/main/groovy/org/grails/gorm/graphql/Schema.groovy +++ b/grails-data-graphql/core/src/main/groovy/org/grails/gorm/graphql/Schema.groovy @@ -381,7 +381,7 @@ class Schema { DataFetcher countFetcher = dataFetcherManager.getReadingFetcher(entity, COUNT).orElse(new CountEntityDataFetcher(entity)) final String countFieldName = namingConvention.getCount(entity) - final GraphQLOutputType countOutputType = (GraphQLOutputType) typeManager.getType(Integer) + final GraphQLOutputType countOutputType = (GraphQLOutputType) typeManager.getType(Long) GraphQLFieldDefinition.Builder queryCount = newFieldDefinition() .name(countFieldName) diff --git a/grails-data-graphql/core/src/main/groovy/org/grails/gorm/graphql/fetcher/impl/CountEntityDataFetcher.groovy b/grails-data-graphql/core/src/main/groovy/org/grails/gorm/graphql/fetcher/impl/CountEntityDataFetcher.groovy index a030d33d7c8..e49ba483a99 100644 --- a/grails-data-graphql/core/src/main/groovy/org/grails/gorm/graphql/fetcher/impl/CountEntityDataFetcher.groovy +++ b/grails-data-graphql/core/src/main/groovy/org/grails/gorm/graphql/fetcher/impl/CountEntityDataFetcher.groovy @@ -34,15 +34,15 @@ import org.grails.gorm.graphql.fetcher.ReadingGormDataFetcher */ @CompileStatic @InheritConstructors -class CountEntityDataFetcher extends DefaultGormDataFetcher implements ReadingGormDataFetcher { +class CountEntityDataFetcher extends DefaultGormDataFetcher implements ReadingGormDataFetcher { - protected Integer queryCount() { + protected Long queryCount() { staticApi.count() } @Override - Integer get(DataFetchingEnvironment environment) { - (Integer) withTransaction(true) { + Long get(DataFetchingEnvironment environment) { + (Long) withTransaction(true) { queryCount() } } diff --git a/grails-data-graphql/core/src/test/groovy/org/grails/gorm/graphql/SchemaSpec.groovy b/grails-data-graphql/core/src/test/groovy/org/grails/gorm/graphql/SchemaSpec.groovy index 76eb909980f..27d0543bb18 100644 --- a/grails-data-graphql/core/src/test/groovy/org/grails/gorm/graphql/SchemaSpec.groovy +++ b/grails-data-graphql/core/src/test/groovy/org/grails/gorm/graphql/SchemaSpec.groovy @@ -19,6 +19,7 @@ package org.grails.gorm.graphql +import graphql.scalars.ExtendedScalars import graphql.schema.* import org.grails.datastore.mapping.config.Settings import org.grails.datastore.mapping.core.DatastoreUtils @@ -67,6 +68,15 @@ class SchemaSpec extends Specification implements GraphQLSchemaSpec { prefix + normalizeType(type) } + void "count query fields use the 64-bit Long scalar"() { + given: + List countFields = queryType.fieldDefinitions.findAll { it.name.endsWith('Count') } + + expect: 'the schema matches what count() returns, and the Long totalCount already in paged results' + !countFields.isEmpty() + countFields.every { it.type == ExtendedScalars.GraphQLLong } + } + void "test ComplexOperation"() { given: GraphQLObjectType type = schema.getType('AwesomeType') diff --git a/grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/AbstractHibernateGormStaticApi.groovy b/grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/AbstractHibernateGormStaticApi.groovy index 231f311146b..da9d7ca909a 100644 --- a/grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/AbstractHibernateGormStaticApi.groovy +++ b/grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/AbstractHibernateGormStaticApi.groovy @@ -196,8 +196,8 @@ abstract class AbstractHibernateGormStaticApi extends GormStaticApi { } @Override - Integer count() { - (Integer) hibernateTemplate.execute({ Session session -> + Long count() { + (Long) hibernateTemplate.execute({ Session session -> CriteriaBuilder criteriaBuilder = session.getCriteriaBuilder() CriteriaQuery criteriaQuery = criteriaBuilder.createQuery(Long) criteriaQuery.select(criteriaBuilder.count(criteriaQuery.from(persistentEntity.javaClass))) @@ -211,7 +211,7 @@ abstract class AbstractHibernateGormStaticApi extends GormStaticApi { } hibernateTemplate.applySettings(criteria) def result = hibernateHqlQuery.singleResult() - Number num = result == null ? 0 : (Number)result + Number num = result == null ? 0L : (Number)result return num }) } diff --git a/grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormStaticApi.groovy b/grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormStaticApi.groovy index f9a5e811935..39a4862c2d6 100644 --- a/grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormStaticApi.groovy +++ b/grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormStaticApi.groovy @@ -231,9 +231,9 @@ class HibernateGormStaticApi extends GormStaticApi { } @Override - Integer count() { + Long count() { String entity = persistentEntity.name - doSingleInternal("select count(*) from $entity" as String, [:], [], [:], false) as Integer + doSingleInternal("select count(*) from $entity" as String, [:], [], [:], false) as Long } @Override diff --git a/grails-data-mongodb/bson/src/main/groovy/org/grails/datastore/bson/codecs/encoders/BasicCollectionTypeEncoder.groovy b/grails-data-mongodb/bson/src/main/groovy/org/grails/datastore/bson/codecs/encoders/BasicCollectionTypeEncoder.groovy index 0693b776a3c..3c994c8cfbd 100644 --- a/grails-data-mongodb/bson/src/main/groovy/org/grails/datastore/bson/codecs/encoders/BasicCollectionTypeEncoder.groovy +++ b/grails-data-mongodb/bson/src/main/groovy/org/grails/datastore/bson/codecs/encoders/BasicCollectionTypeEncoder.groovy @@ -29,7 +29,6 @@ import org.bson.conversions.Bson import org.grails.datastore.bson.codecs.PropertyEncoder import org.grails.datastore.mapping.dirty.checking.DirtyCheckable -import org.grails.datastore.mapping.dirty.checking.DirtyCheckingMap import org.grails.datastore.mapping.dirty.checking.DirtyCheckingSupport import org.grails.datastore.mapping.engine.EntityAccess import org.grails.datastore.mapping.engine.internal.MappingUtils @@ -75,7 +74,7 @@ class BasicCollectionTypeEncoder implements PropertyEncoder { def propertyName = property.name parentAccess.setPropertyNoConversion( propertyName, - new DirtyCheckingMap(value, parent, propertyName) + DirtyCheckingSupport.wrap(value, parent, propertyName) ) } } diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy index 2a0691ba99f..27be405629e 100644 --- a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy +++ b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoCodecSession.groovy @@ -24,6 +24,7 @@ import groovy.transform.CompileStatic import jakarta.persistence.FlushModeType +import com.mongodb.DBRef import com.mongodb.WriteConcern import com.mongodb.bulk.BulkWriteResult import com.mongodb.client.FindIterable @@ -59,9 +60,13 @@ import org.grails.datastore.mapping.model.MappingContext import org.grails.datastore.mapping.model.PersistentEntity import org.grails.datastore.mapping.model.config.GormProperties import org.grails.datastore.mapping.model.types.Association +import org.grails.datastore.mapping.model.types.Embedded +import org.grails.datastore.mapping.model.types.ManyToMany +import org.grails.datastore.mapping.model.types.OneToMany import org.grails.datastore.mapping.model.types.ToOne import org.grails.datastore.mapping.mongo.engine.MongoCodecEntityPersister import org.grails.datastore.mapping.mongo.engine.MongoEntityPersister +import org.grails.datastore.mapping.mongo.config.MongoAttribute import org.grails.datastore.mapping.mongo.engine.MongoIdCoercion import org.grails.datastore.mapping.mongo.engine.codecs.PersistentEntityCodec import org.grails.datastore.mapping.mongo.query.MongoQuery @@ -200,7 +205,9 @@ class MongoCodecSession extends AbstractMongoSession { if (delete.vetoed) continue final Object k = coerceIdToStoredType(delete.nativeKey, persistentEntity) - if (k) { + // Groovy truthiness would skip an empty assigned String id, which is a + // valid BSON _id -- the document would silently survive the delete. + if (k != null) { nativeKeys << k final List cascadeOperations = delete.cascadeOperations addPostFlushOperations(cascadeOperations) @@ -337,6 +344,23 @@ class MongoCodecSession extends AbstractMongoSession { } } + /** + * The identifier an updateAll association value stands for. A lazy proxy keeps its id in the + * proxy handler, so -- like ToOneEncoder -- the proxy factory is asked first. An instance of + * the associated class is reflected. Anything else is taken to be the identifier itself, so + * updateAll(project: project) and updateAll(project: project.id) write the same value. + */ + private Object identifierOf(Object value, PersistentEntity associatedEntity) { + def proxyFactory = mappingContext.proxyFactory + if (proxyFactory.isProxy(value)) { + return proxyFactory.getIdentifier(value) + } + if (associatedEntity.javaClass.isInstance(value)) { + return associatedEntity.reflector.getIdentifier(value) + } + return value + } + @Override long updateAll(QueryableCriteria criteria, Map properties) { final PersistentEntity entity = criteria.persistentEntity @@ -344,16 +368,87 @@ class MongoCodecSession extends AbstractMongoSession { final MongoCollection collection = getCollection(entity) final updateOptions = new UpdateOptions() updateOptions.upsert(false) + // Normalise into a copy: the caller's map is theirs, and may be immutable. Writing the + // encoded reference back into it replaced their domain object with an ObjectId or + // DBRef, and threw UnsupportedOperationException for a Map.of/singletonMap argument. + Map updateProperties = new LinkedHashMap(properties) for (Association association in entity.associations) { String associationName = association.name - if (association instanceof ToOne && properties.containsKey(associationName)) { - def value = properties.get(associationName) + // Embedded extends ToOne, but an embedded value is a subdocument with no + // identity of its own -- normal persistence encodes it through the embedded + // path, not ToOneEncoder. Reflecting an id from one yields null. + // hasOne keeps the foreign key on the child, so ToOneEncoder writes nothing on the + // owner for it -- see its !isForeignKeyInChild() guard. Normalizing it here would + // put an id field on a document that never carries one. + if (association instanceof ToOne && !(association instanceof Embedded) + && ((ToOne) association).isForeignKeyInChild() + && updateProperties.containsKey(associationName)) { + throw new UnsupportedOperationException( + "Cannot updateAll the hasOne association [${entity.name}.${associationName}]: " + + "its foreign key is held by [${association.associatedEntity?.name}], " + + 'so update the inverse side instead') + } + if (association instanceof ToOne && !(association instanceof Embedded) + && updateProperties.containsKey(associationName)) { + def value = updateProperties.get(associationName) if (value != null) { - properties.put(associationName, association.associatedEntity.reflector.getIdentifier(value)) + // Write the reference exactly as ToOneEncoder does on the normal + // persistence path: in the target's stored _id type, as a DBRef where the + // mapping asks for one. Otherwise a bulk update leaves a reference that + // association queries and external clients cannot match. + def associatedEntity = association.associatedEntity + def associationId = MongoIdCoercion.coerceIdToStoredType( + identifierOf(value, associatedEntity), associatedEntity) + MongoAttribute attr = (MongoAttribute) association.mapping.mappedForm + if (attr?.isReference()) { + updateProperties.put(associationName, + new DBRef(getCollectionName(associatedEntity), associationId)) + } + else { + updateProperties.put(associationName, associationId) + } + } + } + // A bidirectional one-to-many keeps its foreign key on the inverse side, so there + // is no field on this document to update -- OneToManyEncoder's shouldEncodeIds + // skips it for the same reason, and the decoder never reads one. Left in the $set + // the value is written as raw subdocuments and the owner stops decoding, so this + // says so rather than corrupting the document or silently doing nothing. + // ManyToMany extends ToMany, not OneToMany, so it never reaches this branch. + else if (association instanceof OneToMany && association.isBidirectional() + && updateProperties.containsKey(associationName)) { + throw new UnsupportedOperationException( + "Cannot updateAll the bidirectional one-to-many [${entity.name}.${associationName}]: " + + "its foreign key is held by [${association.associatedEntity?.name}], " + + 'so update the inverse side instead') + } + // Unidirectional OneToMany and ManyToMany carry a collection of associated instances. + // Normal persistence stores their ids -- DBRefs where the mapping asks for it -- so + // the bulk path does the same rather than sending domain objects through $set. + // Only those two kinds: Basic also extends ToMany but is a collection of simple + // values with no associated entity, and must pass through untouched. + else if ((association instanceof OneToMany || association instanceof ManyToMany) + && association.associatedEntity != null + && updateProperties.containsKey(associationName)) { + def value = updateProperties.get(associationName) + if (value instanceof Collection) { + def associatedEntity = association.associatedEntity + MongoAttribute attr = (MongoAttribute) association.mapping.mappedForm + def ids = value.collect { element -> + element == null ? null : + MongoIdCoercion.coerceIdToStoredType(identifierOf(element, associatedEntity), associatedEntity) + } + // Exactly OneToManyEncoder's shape: nulls are dropped before wrapping, + // never turned into DBRef(collection, null), and a non-reference list + // keeps whatever the caller passed. + def encoded = attr?.isReference() + ? ids.findAll { it != null }.collect { new DBRef(getCollectionName(associatedEntity), it) } + : ids + updateProperties.put(associationName, encoded) } } } - final UpdateResult updateResult = updateMany(collection, nativeQuery, new Document(MONGO_SET_OPERATOR, properties), updateOptions) + final UpdateResult updateResult = updateMany(collection, nativeQuery, new Document(MONGO_SET_OPERATOR, updateProperties), updateOptions) if (updateResult.wasAcknowledged()) { try { return updateResult.modifiedCount diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java index 1be229be4e9..0f27b577cd8 100644 --- a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java +++ b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/MongoDatastore.java @@ -43,6 +43,8 @@ import org.bson.codecs.configuration.CodecProvider; import org.bson.codecs.configuration.CodecRegistries; import org.bson.codecs.configuration.CodecRegistry; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.MessageSource; @@ -150,6 +152,8 @@ public class MongoDatastore extends AbstractDatastore implements MappingContext. private static final int INDEX_OPTIONS_CONFLICT_CODE = 85; public static final String CODEC_ENGINE = MongoConstants.CODEC_ENGINE; + private static final Logger LOG = LoggerFactory.getLogger(MongoDatastore.class); + /** * Not final because {@link #start()} replaces it after a CRaC restore. Everything other * than construction reaches it through {@link #getMongoClient()}, so a replacement is @@ -204,6 +208,11 @@ public MongoDatastore(final ConnectionSources((Document) k)); @@ -288,7 +306,13 @@ public void delete(Iterable objects) { for (final PersistentEntity persistentEntity : toDelete.keySet()) { final MongoQuery query = new MongoQuery(this, persistentEntity); - query.in(MongoEntityPersister.MONGO_ID_FIELD, toDelete.get(persistentEntity)); + // Filters on the literal "_id" field, not the logical identity name, so the + // criterion preprocessing in MongoQuery does not recognise it as an id. + final List deleteKeys = new ArrayList(); + for (Object k : toDelete.get(persistentEntity)) { + deleteKeys.add(MongoIdCoercion.coerceIdToStoredType(k, persistentEntity)); + } + query.in(MongoEntityPersister.MONGO_ID_FIELD, deleteKeys); final Document mongoQuery = query.getMongoQuery(); final EntityPersister persister = (EntityPersister) getPersister(persistentEntity); addPendingDelete(new PendingDeleteAdapter(persistentEntity, mongoQuery, null) { @@ -343,6 +367,25 @@ public long deleteAll(QueryableCriteria criteria) { } } + /** + * The identifier an updateAll association value stands for. A lazy proxy keeps its id in the + * proxy handler, so -- like ToOneEncoder -- the proxy factory is asked first. An instance of + * the associated class is reflected. Anything else is taken to be the identifier itself, so + * updateAll(project: project) and updateAll(project: project.id) write the same value. + * Before this normalization existed this engine sent the raw value through $set, so passing + * an identifier worked; reflecting it unconditionally would have broken that. + */ + private Object identifierOf(Object value, PersistentEntity associatedEntity) { + final ProxyFactory proxyFactory = getMappingContext().getProxyFactory(); + if (proxyFactory.isProxy(value)) { + return proxyFactory.getIdentifier(value); + } + if (associatedEntity.getJavaClass().isInstance(value)) { + return getMappingContext().getEntityReflector(associatedEntity).getIdentifier(value); + } + return value; + } + @Override public long updateAll(QueryableCriteria criteria, Map properties) { final PersistentEntity entity = criteria.getPersistentEntity(); @@ -350,7 +393,104 @@ public long updateAll(QueryableCriteria criteria, Map properties final com.mongodb.client.MongoCollection collection = getCollection(entity); final UpdateOptions updateOptions = new UpdateOptions(); updateOptions.upsert(false); - final UpdateResult updateResult = updateMany(collection, nativeQuery, new Document("$set", properties), updateOptions); + // Encode to-one association values the way normal persistence does: the target's + // identifier, in the type its _id is stored as, wrapped in a DBRef where the mapping + // asks for one. Without this a bulk update writes the domain object itself. + // Normalised into a copy -- the caller's map is theirs, and may be immutable. + final Map updateProperties = new LinkedHashMap(properties); + for (Association association : entity.getAssociations()) { + final String associationName = association.getName(); + // Embedded extends ToOne, but an embedded value is a subdocument with no + // identity of its own -- normal persistence encodes it through the embedded + // path, not ToOneEncoder. Reflecting an id from one yields null. + // hasOne keeps the foreign key on the child, so ToOneEncoder writes nothing on the + // owner for it -- see its !isForeignKeyInChild() guard. Normalizing it here would + // put an id field on a document that never carries one. + if (association instanceof ToOne && !(association instanceof Embedded) && + ((ToOne) association).isForeignKeyInChild() && + updateProperties.containsKey(associationName)) { + throw new UnsupportedOperationException( + "Cannot updateAll the hasOne association [" + entity.getName() + "." + + associationName + "]: its foreign key is held by the inverse side, " + + "so update that instead"); + } + if (association instanceof ToOne && !(association instanceof Embedded) && + updateProperties.containsKey(associationName)) { + final Object value = updateProperties.get(associationName); + if (value != null) { + final PersistentEntity associatedEntity = association.getAssociatedEntity(); + final Object associationId = MongoIdCoercion.coerceIdToStoredType( + identifierOf(value, associatedEntity), associatedEntity); + final MongoAttribute attr = (MongoAttribute) association.getMapping().getMappedForm(); + if (attr != null && attr.isReference()) { + updateProperties.put(associationName, + new DBRef(getCollectionName(associatedEntity), associationId)); + } + else { + updateProperties.put(associationName, associationId); + } + } + } + // A bidirectional one-to-many keeps its foreign key on the inverse side, so there + // is no field on this document to update -- OneToManyEncoder's shouldEncodeIds + // skips it for the same reason, and nothing reads one back. This says so rather + // than leaving a stray field or silently doing nothing. + // ManyToMany extends ToMany, not OneToMany, so it never reaches this branch. + else if (association instanceof OneToMany && association.isBidirectional() && + updateProperties.containsKey(associationName)) { + throw new UnsupportedOperationException( + "Cannot updateAll the bidirectional one-to-many [" + entity.getName() + "." + + associationName + "]: its foreign key is held by the inverse side, " + + "so update that instead"); + } + // Unidirectional OneToMany and ManyToMany carry a collection of associated instances. + // Normal persistence stores their ids -- DBRefs where the mapping asks for it -- so + // the bulk path does the same rather than sending domain objects through $set. + // Only those two kinds: Basic also extends ToMany but is a collection of simple + // values with no associated entity, and must pass through untouched. + else if ((association instanceof OneToMany || association instanceof ManyToMany) && + association.getAssociatedEntity() != null && + updateProperties.containsKey(associationName)) { + final Object value = updateProperties.get(associationName); + if (value instanceof Collection) { + final PersistentEntity associatedEntity = association.getAssociatedEntity(); + final MongoAttribute attr = (MongoAttribute) association.getMapping().getMappedForm(); + final List ids = new ArrayList(); + for (Object element : (Collection) value) { + if (element == null) { + ids.add(null); + continue; + } + ids.add(MongoIdCoercion.coerceIdToStoredType( + identifierOf(element, associatedEntity), associatedEntity)); + } + if (association instanceof ManyToMany) { + // setManyToMany stores plain identifiers under a suffixed key and + // getManyToManyKeys reads them back the same way -- never DBRefs -- so + // writing the plain name, or wrapping these, would leave the field the + // read path actually uses untouched. + updateProperties.remove(associationName); + updateProperties.put(associationName + "_$$manyToManyIds", ids); + } + else { + // Exactly OneToManyEncoder's shape: nulls are dropped before wrapping, + // never turned into DBRef(collection, null), and a non-reference list + // keeps whatever the caller passed. + List encoded = ids; + if (attr != null && attr.isReference()) { + encoded = new ArrayList(); + for (Object id : ids) { + if (id != null) { + encoded.add(new DBRef(getCollectionName(associatedEntity), id)); + } + } + } + updateProperties.put(associationName, encoded); + } + } + } + } + final UpdateResult updateResult = updateMany(collection, nativeQuery, new Document("$set", updateProperties), updateOptions); if (updateResult.wasAcknowledged()) { try { return updateResult.getModifiedCount(); diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoMappingContext.java b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoMappingContext.java index 7898b416d6d..2d086a599e5 100644 --- a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoMappingContext.java +++ b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoMappingContext.java @@ -140,12 +140,25 @@ public class MongoMappingContext extends DocumentMappingContext { private CodecRegistry codecRegistry; private Map hasCodecCache = new HashMap<>(); + /** + * Storage type applied to {@code String id} domains that declare no explicit + * {@code storedAs}. ObjectId keeps {@code _id} a native BSON type -- smaller on disk and + * in indexes, and directly usable from every other MongoDB client -- while application + * code still sees a {@code String}. Opt back out globally with + * {@code grails.mongodb.stringIds.defaultStoredAs: string}, or per domain with + * {@code static mapping = { id storedAs: String }}. + */ + private static final Class DEFAULT_STRING_ID_STORED_AS = ObjectId.class; + /** * Global default storage type for {@code String id} fields that don't declare an explicit - * {@code id storedAs: ...} in their mapping. Null means "no default — use the declared - * Java type" (current GORM behavior). See {@link MongoSettings#SETTING_STRING_IDS_DEFAULT_STORED_AS}. + * {@code id storedAs: ...} in their mapping. Initialized to + * {@link #DEFAULT_STRING_ID_STORED_AS} so that every constructor -- including the ones + * that read no configuration at all -- registers entities with the same default; the + * config-reading constructors overwrite it from + * {@link MongoSettings#SETTING_STRING_IDS_DEFAULT_STORED_AS}. Null disables coercion. */ - private Class stringIdDefaultStoredAs; + private Class stringIdDefaultStoredAs = DEFAULT_STRING_ID_STORED_AS; private Class portableIdentityType = Long.class; public Class getStringIdDefaultStoredAs() { @@ -198,7 +211,7 @@ public MongoMappingContext(PropertyResolver configuration, Class... classes) { } private static Class parseStoredAs(String value) { - if (value == null) return null; + if (value == null) return DEFAULT_STRING_ID_STORED_AS; switch (value.toLowerCase()) { case "objectid": case "object_id": @@ -207,9 +220,9 @@ private static Class parseStoredAs(String value) { return String.class; default: log.warn("Unrecognized value '{}' for {}; accepted values are 'objectid' or 'string'. " + - "Falling back to default behavior (no coercion).", + "Falling back to the default ('objectid').", value, MongoSettings.SETTING_STRING_IDS_DEFAULT_STORED_AS); - return null; + return DEFAULT_STRING_ID_STORED_AS; } } diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoSettings.groovy b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoSettings.groovy index 4a2bbf15fbd..11bde551739 100644 --- a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoSettings.groovy +++ b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/config/MongoSettings.groovy @@ -93,18 +93,30 @@ interface MongoSettings extends Settings { String SETTING_STATELESS = 'grails.mongodb.stateless' + /** + * Selects the persistence engine. {@code 'codec'} is the default and the recommended + * value; {@code 'mapping'} remains available for compatibility but is deprecated. + * + * @deprecated The non-codec ("mapping") engine this setting can select is deprecated and + * will be removed in a future release, at which point this setting becomes a no-op. + */ + @Deprecated String SETTING_ENGINE = 'grails.mongodb.engine' /** * Global default storage type for {@code String id} fields when no per-domain * {@code id storedAs: ...} mapping is declared. Accepted values are the names (or hex - * aliases) {@code 'string'} (default, current behavior) and {@code 'objectid'}. + * aliases) {@code 'objectid'} (the default) and {@code 'string'}. + * + *

Every domain that declares {@code String id} without an explicit {@code storedAs} + * persists {@code _id} as a BSON ObjectId, while keeping the {@code String} ergonomics + * in application code. Domains that use natural string keys (slug, email, UUID) should + * opt out per-domain via {@code static mapping = { id storedAs: String }}; set this to + * {@code 'string'} to opt the whole application out. * - *

When set to {@code 'objectid'}, every domain that declares {@code String id} - * without an explicit {@code storedAs} will persist {@code _id} as a BSON ObjectId, - * while keeping the {@code String} ergonomics in application code. Domains that use - * natural string keys (slug, email, UUID) should opt out per-domain via - * {@code static mapping = { id storedAs: String }}. + *

The default changed from {@code 'string'} to {@code 'objectid'} in 8.0.0. An + * application upgrading with existing string {@code _id} data must either migrate that + * data or pin {@code 'string'} here. * * @since 7.1.1 */ diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/connections/StringIdSettings.groovy b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/connections/StringIdSettings.groovy index 6a9ac5280b3..85817f657fc 100644 --- a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/connections/StringIdSettings.groovy +++ b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/connections/StringIdSettings.groovy @@ -37,7 +37,7 @@ class StringIdSettings { /** * Global default storage type for {@code String id} fields that don't declare an * explicit {@code id storedAs: ...} in their mapping. Accepted values are - * {@code 'string'} (default, current behavior) or {@code 'objectid'}. + * {@code 'objectid'} (the default since 8.0.0) or {@code 'string'}. * *

Corresponds to the property path * {@code grails.mongodb.stringIds.defaultStoredAs}. diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/AbstractMongoObectEntityPersister.java b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/AbstractMongoObectEntityPersister.java index 393ed31d4de..78476143eca 100644 --- a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/AbstractMongoObectEntityPersister.java +++ b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/AbstractMongoObectEntityPersister.java @@ -62,7 +62,14 @@ * * @author Graeme Rocher * @since 5.0 + * + * @deprecated The non-codec ("mapping") persistence engine is deprecated and will be removed + * in a future release. Use the default codec engine, which is what + * {@code grails.mongodb.engine} selects when unset. This engine reaches MongoDB through a + * separate persister hierarchy that has to be kept in step with the codec one for every + * storage-layer change, and it carries no feature the codec engine lacks. */ +@Deprecated public abstract class AbstractMongoObectEntityPersister extends NativeEntryEntityPersister { public static final String INSTANCE_PREFIX = "instance:"; public static final String MONGO_ID_FIELD = "_id"; @@ -198,10 +205,13 @@ protected void setEmbeddedCollectionKeys(Association association, EntityAccess e List dbRefs = new ArrayList(); boolean reference = isReference(association); for (Object foreignKey : keys) { + // Same as formulateDatabaseReference: the embedded collection's references must + // carry the target's stored _id type. + Object coerced = MongoIdCoercion.coerceIdToStoredType(foreignKey, association.getAssociatedEntity()); if (reference) { - dbRefs.add(new DBRef(getCollectionName(association.getAssociatedEntity()), foreignKey)); + dbRefs.add(new DBRef(getCollectionName(association.getAssociatedEntity()), coerced)); } else { - dbRefs.add(foreignKey); + dbRefs.add(coerced); } } getValueRetrievalStrategy().setValue(embeddedEntry, association.getName(), dbRefs); @@ -327,10 +337,15 @@ protected Object getEntryValue(T nativeEntry, String property) { @Override protected Object formulateDatabaseReference(PersistentEntity persistentEntity, Association association, Serializable associationId) { boolean isReference = isReference(association); + // Write the reference in the type the TARGET's _id is stored as, so it matches the + // document it points at. The codec engine does this in ToOneEncoder; without it here + // a String-id target stored as ObjectId is pointed at by a BSON String, which no + // $lookup, raw driver query or coerced association criterion can match. + Object coerced = MongoIdCoercion.coerceIdToStoredType(associationId, association.getAssociatedEntity()); if (isReference) { - return new DBRef(getCollectionName(association.getAssociatedEntity()), associationId); + return new DBRef(getCollectionName(association.getAssociatedEntity()), coerced); } - return associationId; + return coerced; } @Override @@ -388,7 +403,10 @@ protected void setManyToMany(PersistentEntity persistentEntity, Object obj, PersistentEntity childPersistentEntity = getMappingContext().getPersistentEntity(o.getClass().getName()); EntityAccess entityAccess = createEntityAccess(childPersistentEntity, o); - ids.add(entityAccess.getIdentifier()); + // Stored in the target's _id type for the same reason as every other + // association reference. + ids.add(MongoIdCoercion.coerceIdToStoredType( + entityAccess.getIdentifier(), childPersistentEntity)); } } } @@ -437,11 +455,15 @@ public void preIndex(final Object primaryKey, final List foreignKeys) { if (!association.isBidirectional()) { List dbRefs = new ArrayList(); for (Object foreignKey : foreignKeys) { + // Same as formulateDatabaseReference: the stored reference must carry the + // target's _id type, or raw joins and DBRef consumers cannot match it. + Object coerced = MongoIdCoercion.coerceIdToStoredType( + foreignKey, association.getAssociatedEntity()); if (isReference) { - dbRefs.add(new DBRef(getCollectionName(association.getAssociatedEntity()), foreignKey)); + dbRefs.add(new DBRef(getCollectionName(association.getAssociatedEntity()), coerced)); } else { - dbRefs.add(foreignKey); + dbRefs.add(coerced); } } // update the native entry directly. diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoEntityPersister.java b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoEntityPersister.java index 8faae47a255..7e1a815ca68 100644 --- a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoEntityPersister.java +++ b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoEntityPersister.java @@ -57,8 +57,15 @@ * * @author Graeme Rocher * @since 1.0 + * + * @deprecated The non-codec ("mapping") persistence engine is deprecated and will be removed + * in a future release. Use the default codec engine, which is what + * {@code grails.mongodb.engine} selects when unset. This engine reaches MongoDB through a + * separate persister hierarchy that has to be kept in step with the codec one for every + * storage-layer change, and it carries no feature the codec engine lacks. */ @SuppressWarnings({"rawtypes", "unchecked"}) +@Deprecated public class MongoEntityPersister extends AbstractMongoObectEntityPersister { public static final ValueRetrievalStrategy VALUE_RETRIEVAL_STRATEGY = new ValueRetrievalStrategy<>() { @@ -331,8 +338,11 @@ private Document removeNullEntries(Document nativeEntry) { protected Object storeEntry(final PersistentEntity persistentEntity, final EntityAccess entityAccess, final Object storeId, final Document nativeEntry) { - nativeEntry.put(MONGO_ID_FIELD, storeId); - return nativeEntry.get(MONGO_ID_FIELD); + // Honour the id mapping's storedAs, which the codec engine applies via IdentityEncoder. + // This is the single point where _id is written for this engine. The declared-type + // value is still returned, so the domain keeps the String id it declared. + nativeEntry.put(MONGO_ID_FIELD, MongoIdCoercion.coerceIdToStoredType(storeId, persistentEntity)); + return storeId; } protected String getCollectionName(PersistentEntity persistentEntity, Document nativeEntry) { @@ -410,7 +420,9 @@ protected void deleteEntries(String family, final List keys) { protected Document createDBObjectWithKey(Object key) { Document dbo = new Document(); if (hasNumericalIdentifier || hasStringIdentifier) { - dbo.put(MONGO_ID_FIELD, key); + // Match the type storeEntry wrote: a String-id domain resolved to + // storedAs: ObjectId is filtered by ObjectId, not by the hex String. + dbo.put(MONGO_ID_FIELD, MongoIdCoercion.coerceIdToStoredType(key, getPersistentEntity())); } else { if (key instanceof ObjectId) { diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoIdCoercion.java b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoIdCoercion.java index 9fd867999fd..1425f6846e2 100644 --- a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoIdCoercion.java +++ b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/MongoIdCoercion.java @@ -90,4 +90,28 @@ public static Object coerceIdToStoredType(Object key, PersistentEntity entity) { return key; } } + + /** + * Inverse of {@link #coerceIdToStoredType}: convert a value just read from BSON back to + * the entity's declared identifier type, so that association properties hold the type the + * domain class declares ({@code String}) regardless of how {@code _id} is stored on disk + * ({@code ObjectId}). + * + *

Returns the original {@code value} when it is {@code null}, already an instance of + * the declared type, the entity declares no identity, or the converter throws. + */ + public static Object coerceIdToDeclaredType(Object value, PersistentEntity entity) { + if (value == null || entity == null) return value; + try { + if (entity.getIdentity() == null) return value; + Class declared = entity.getIdentity().getType(); + if (declared == null || declared.isInstance(value)) return value; + ConversionService cs = entity.getMappingContext().getConversionService(); + Object converted = cs.convert(value, declared); + return converted != null ? converted : value; + } + catch (Exception ignored) { + return value; + } + } } diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/codecs/PersistentEntityCodec.groovy b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/codecs/PersistentEntityCodec.groovy index 38fcbd023ea..70fb6d0a6c6 100644 --- a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/codecs/PersistentEntityCodec.groovy +++ b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/engine/codecs/PersistentEntityCodec.groovy @@ -78,6 +78,7 @@ import org.grails.datastore.mapping.mongo.MongoConstants import org.grails.datastore.mapping.mongo.MongoDatastore import org.grails.datastore.mapping.mongo.config.MongoAttribute import org.grails.datastore.mapping.mongo.engine.MongoCodecEntityPersister +import org.grails.datastore.mapping.mongo.engine.MongoIdCoercion import org.grails.datastore.mapping.query.Query import org.grails.datastore.mapping.reflect.FieldEntityAccess @@ -423,7 +424,11 @@ class PersistentEntityCodec extends BsonPersistentEntityCodec { protected void encodeEmbeddedCollectionUpdate(EntityAccess parentAccess, BsonDocument sets, Document unsets, Association association, Object v) { if (v instanceof Collection) { - if ((v instanceof DirtyCheckableCollection) && !((DirtyCheckableCollection) v).hasChangedSize()) { + // Per-element updates are only valid for the decoded collection mutated in place — + // an ASSIGNED wrapper is a wholesale replacement whose layout need not match the + // stored array, so it must fall through to the full re-encode below. + if ((v instanceof DirtyCheckableCollection) && !((DirtyCheckableCollection) v).hasChangedSize() + && !((DirtyCheckableCollection) v).isAssigned()) { int i = 0 for (o in (v as Collection)) { def embeddedUpdate = encodeUpdate(o, createEntityAccess(o), EncoderContext.builder().build(), true) @@ -542,6 +547,9 @@ class PersistentEntityCodec extends BsonPersistentEntityCodec { return it } } + identifiers = identifiers.collect { + MongoIdCoercion.coerceIdToDeclaredType(it, property.associatedEntity) + } def associatedType = property.associatedEntity.javaClass if (SortedSet.isAssignableFrom(type)) { entityAccess.setPropertyNoConversion( @@ -614,10 +622,16 @@ class PersistentEntityCodec extends BsonPersistentEntityCodec { if (updateCollection) { // update existing collection Collection identifiers = (Collection) mongoSession.getAttribute(parentAccess.entity, "${property}.ids") + if (identifiers != null) { + identifiers = identifiers.collect { + MongoIdCoercion.coerceIdToStoredType(it, associatedEntity) + } + } if (identifiers == null) { def entityReflector = FieldEntityAccess.getOrIntializeReflector(associatedEntity) identifiers = ((Collection) value).collect() { - entityReflector.getIdentifier(it) + MongoIdCoercion.coerceIdToStoredType( + entityReflector.getIdentifier(it), associatedEntity) } } writer.writeName(MappingUtils.getTargetKey((PersistentProperty) property)) @@ -660,6 +674,10 @@ class PersistentEntityCodec extends BsonPersistentEntityCodec { def associationAccess = mappingContext.getEntityReflector(associatedEntity) associationId = associationAccess.getIdentifier(value) } + // Write the reference in the type the target's _id is STORED as, so a + // String-id domain whose _id is a BSON ObjectId is pointed at by an + // ObjectId -- keeping $lookup and raw driver queries working. + associationId = MongoIdCoercion.coerceIdToStoredType(associationId, associatedEntity) if (associationId != null) { writer.writeName(MappingUtils.getTargetKey(property)) MongoAttribute attr = (MongoAttribute) property.mapping.mappedForm @@ -704,21 +722,28 @@ class PersistentEntityCodec extends BsonPersistentEntityCodec { associationId = (Serializable) dBRef.get(DB_REF_ID_FIELD) } else { - switch (associatedEntity.identity.type) { - case ObjectId: + // Read whatever BSON type is actually present rather than the type the + // mapping predicts. The two legitimately differ: a non-hex assigned id falls + // back to BSON String even under storedAs: ObjectId, and a collection written + // before a storedAs change holds the old type. Reading by expectation throws + // BsonInvalidOperationException on those documents. + switch (bsonReader.currentBsonType) { + case BsonType.OBJECT_ID: associationId = bsonReader.readObjectId() break - case Long: + case BsonType.INT64: associationId = (Long) bsonReader.readInt64() break - case Integer: - associationId = (Integer) bsonReader.readInt32() + case BsonType.INT32: + associationId = (Integer) bsonReader.readInt32() break default: associationId = bsonReader.readString() } } + associationId = (Serializable) MongoIdCoercion.coerceIdToDeclaredType(associationId, associatedEntity) + if (isLazy) { entityAccess.setPropertyNoConversion( property.name, diff --git a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/query/MongoQuery.java b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/query/MongoQuery.java index d00231eb66a..06f22bfe7e2 100644 --- a/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/query/MongoQuery.java +++ b/grails-data-mongodb/core/src/main/groovy/org/grails/datastore/mapping/mongo/query/MongoQuery.java @@ -173,12 +173,16 @@ public void handle(EmbeddedQueryEncoder queryEncoder, In in, Document query, Per Document inQuery = new Document(); List values = getInListQueryValues(entity, in); - PersistentProperty identityProp = entity.getIdentity(); - boolean isIdInList = identityProp != null && identityProp.getName().equals(in.getProperty()); - if (isIdInList && MongoIdCoercion.resolveStoredAs(entity) != null) { + // getInListQueryValues unwraps association instances to their *declared* + // identifier, so the storage type has to be applied afterwards -- for the + // entity's own identity (findAllByIdInList) and equally for a to-one + // association (`child in [childInstance]`, findAllByChildInList(..)), whose + // ids are governed by the associated entity's mapping, not this one's. + PersistentEntity idTarget = resolveIdCriterionTarget(entity, in.getProperty()); + if (idTarget != null && MongoIdCoercion.resolveStoredAs(idTarget) != null) { List coerced = new ArrayList<>(values.size()); for (Object v : values) { - coerced.add(MongoIdCoercion.coerceIdToStoredType(v, entity)); + coerced.add(MongoIdCoercion.coerceIdToStoredType(v, idTarget)); } values = coerced; } @@ -731,6 +735,8 @@ public static void populateMongoQuery(final EmbeddedQueryEncoder queryEncoder, D subList.add(dbo); } + coerceIdCriterion(criterion, entity); + if (criterion instanceof PropertyCriterion && !(criterion instanceof GeoCriterion)) { PropertyCriterion pc = (PropertyCriterion) criterion; PersistentProperty property = entity.getPropertyByName(pc.getProperty()); @@ -749,6 +755,61 @@ public static void populateMongoQuery(final EmbeddedQueryEncoder queryEncoder, D } } + /** + * Sends identifier-bearing criteria in the type the target's {@code _id} is actually stored + * as, the same way the {@code IdEquals} handler does for {@code _id} itself. + * + *

Two kinds of criterion carry an identifier without being routed through that handler: + * a filter on a to-one association (the associated entity's id, as used by bidirectional + * one-to-many and {@code hasOne} lookups) and a dynamic finder such as + * {@code findAllById(hex)}, which builds {@code Equals('id', ..)} rather than + * {@code IdEquals}. Left uncoerced they send a hex String against an ObjectId and silently + * match nothing. + * + *

Recurses through junctions so criteria nested inside {@code not { }}, + * {@code and { }} and {@code or { }} are covered as well -- the inherited negation handler + * dispatches nested criteria itself, so they never reach this preprocessing otherwise, and + * an uncoerced negated id predicate fails to exclude the document it names. + */ + private static void coerceIdCriterion(Criterion criterion, PersistentEntity entity) { + if (criterion instanceof Junction) { + for (Criterion nested : ((Junction) criterion).getCriteria()) { + coerceIdCriterion(nested, entity); + } + return; + } + if (!(criterion instanceof PropertyCriterion) || + criterion instanceof GeoCriterion || + criterion instanceof SubqueryCriterion) { + return; + } + PropertyCriterion pc = (PropertyCriterion) criterion; + PersistentEntity idTarget = resolveIdCriterionTarget(entity, pc.getProperty()); + if (idTarget == null || MongoIdCoercion.resolveStoredAs(idTarget) == null) { + return; + } + Object raw = pc.getValue(); + if (raw != null) { + pc.setValue(MongoIdCoercion.coerceIdToStoredType(raw, idTarget)); + } + } + + /** + * The entity whose identifier mapping governs a criterion on {@code propertyName}: the + * associated entity for a to-one association, the queried entity for its own identity, + * otherwise {@code null}. + */ + private static PersistentEntity resolveIdCriterionTarget(PersistentEntity entity, String propertyName) { + PersistentProperty property = entity.getPropertyByName(propertyName); + if (property instanceof ToOne) { + return ((ToOne) property).getAssociatedEntity(); + } + if (entity.getIdentity() != null && entity.getIdentity().getName().equals(propertyName)) { + return entity; + } + return null; + } + /** * Rewrites a query built against an embedded entity so it applies to the owning document, * qualifying each property name with the embedded property's path. Logical operators such as diff --git a/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/BrokenManyToManyAssociationSpec.groovy b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/BrokenManyToManyAssociationSpec.groovy index 5ad94051fe8..18ed331adcd 100644 --- a/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/BrokenManyToManyAssociationSpec.groovy +++ b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/BrokenManyToManyAssociationSpec.groovy @@ -23,6 +23,7 @@ import grails.persistence.Entity import org.apache.grails.data.mongo.core.GrailsDataMongoTckManager import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec import org.bson.Document +import org.bson.types.ObjectId /** * @author Noam Y. Tenne @@ -44,7 +45,7 @@ class BrokenManyToManyAssociationSpec extends GrailsDataTckSpec + *

  • The defensive re-init {@code if (!entity.shares) entity.shares = []} — true for an + * EMPTY tracked list (Groovy falsy) — used to replace the tracked wrapper installed at + * decode time with a plain untracked list, so the add() that followed never persisted.
    • + *
  • Groovy's {@code removeAll(Closure)} removes through {@code iterator().remove()}, which + * used to bypass the wrapper's change tracking entirely.
    • + * + */ +class EmbeddedCollectionDirtyTrackingSpec extends GrailsDataTckSpec { + + void setupSpec() { + manager.registerDomainClasses(SharedCalendar, CalendarShare) + } + + void "in-place add after a falsy empty re-init is persisted"() { + given: "a persisted entity with an empty embedded collection" + SharedCalendar calendar = new SharedCalendar(name: "My Gym", shares: []).save(flush: true, validate: false) + manager.session.clear() + + when: "the entity is loaded, defensively re-initialised and mutated in place" + calendar = SharedCalendar.get(calendar.id) + // A full application activates change tracking on load (DomainEventListener + // .activateDirtyChecking); without it hasChanged() defaults to true and every save + // writes unconditionally, which would mask the bug this spec guards against. + calendar.trackChanges() + if (!calendar.shares) { + calendar.shares = [] + } + calendar.shares.add(new CalendarShare(userId: "user-1", role: "VIEWER")) + calendar.save(flush: true) + manager.session.clear() + calendar = SharedCalendar.get(calendar.id) + + then: "the added element was persisted" + calendar.shares.size() == 1 + calendar.shares[0].userId == "user-1" + calendar.shares[0].role == "VIEWER" + } + + void "a mutation after borrowing another entity's collection is not silently lost"() { + given: "two calendars whose tag lists have identical content" + SharedCalendar a = new SharedCalendar(name: "A", tags: ["shared"]).save(flush: true, validate: false) + SharedCalendar b = new SharedCalendar(name: "B", tags: ["shared"]).save(flush: true, validate: false) + manager.session.clear() + + when: "B's list is assigned onto A and then mutated through A" + a = SharedCalendar.get(a.id) + a.trackChanges() + b = SharedCalendar.get(b.id) + a.tags = b.tags + a.tags.add("added-via-a") + a.save(flush: true) + manager.session.clear() + a = SharedCalendar.get(a.id) + + // Pre-fix A kept B's wrapper, so the add marked B dirty and left A clean, and the + // assignment could not flag A either: the two lists were equal in content and markDirty + // suppresses an equal-valued assignment. An explicit save() still sets the whole-class + // dirty marker, but the lastUpdated PreUpdate write replaces that marker with a map + // holding lastUpdated alone - so the update carried lastUpdated and nothing else, and + // B silently absorbed the element. SharedCalendar.lastUpdated is load-bearing here: + // without it the class-wide marker survives, every property is re-encoded, and this + // spec passes whether or not the fix is present. + then: "A's own document records the addition" + a.tags == ["shared", "added-via-a"] + } + + /** + * Regression guard: pre-fix this passed only by luck — the wrapper missed the iterator + * removal, but the Mongo persister's size-change net (DirtyCheckableCollection + * .hasChangedSize()) rescued the still-wrapped collection. The wrapper-level bypass is + * proven by DirtyCheckingCollectionSpec; this pins the end-to-end outcome either way. + */ + void "closure-based removeAll on a loaded embedded collection is persisted"() { + given: "a persisted entity with two embedded elements" + SharedCalendar calendar = new SharedCalendar(name: "Work", shares: [ + new CalendarShare(userId: "user-1", role: "VIEWER"), + new CalendarShare(userId: "user-2", role: "EDITOR") + ]).save(flush: true, validate: false) + manager.session.clear() + + when: "an element is removed via the Groovy closure variant (iterator-based)" + calendar = SharedCalendar.get(calendar.id) + calendar.trackChanges() // see above — mirror a full application's on-load activation + calendar.shares.removeAll { it.userId == "user-1" } + calendar.save(flush: true) + manager.session.clear() + calendar = SharedCalendar.get(calendar.id) + + then: "the removal was persisted" + calendar.shares.size() == 1 + calendar.shares[0].userId == "user-2" + } +} + +@Entity +class SharedCalendar { + ObjectId id + String name + List tags + List shares + // Auto-timestamped like real-world domains. The PreUpdate timestamp write destroys the + // whole-class dirty marker that an explicit save() sets (markDirty(String) resets the + // marker map), so the update only contains individually-tracked properties — making the + // persisted outcome depend entirely on the collection wrappers this spec exercises. + Date lastUpdated + + static mapWith = "mongo" + static mapping = { + collection "sharedCalendar" + } + static constraints = { + shares(blank: true, nullable: true) + tags(blank: true, nullable: true) + } + static embedded = ['shares'] +} + +@Entity +class CalendarShare { + ObjectId id + String userId + String role + + static mapWith = "mongo" + static constraints = { + } +} diff --git a/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/EmbeddedWithNonEmbeddedCollectionsSpec.groovy b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/EmbeddedWithNonEmbeddedCollectionsSpec.groovy index c9a7012b97b..f66928456b9 100644 --- a/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/EmbeddedWithNonEmbeddedCollectionsSpec.groovy +++ b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/EmbeddedWithNonEmbeddedCollectionsSpec.groovy @@ -22,6 +22,7 @@ import grails.mongodb.MongoEntity import grails.persistence.Entity import org.apache.grails.data.mongo.core.GrailsDataMongoTckManager import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec +import org.bson.types.ObjectId class EmbeddedWithNonEmbeddedCollectionsSpec extends GrailsDataTckSpec { void setupSpec() { @@ -51,12 +52,14 @@ class EmbeddedWithNonEmbeddedCollectionsSpec extends GrailsDataTckSpec { + + void setupSpec() { + manager.registerDomainClasses(ProbeBoard, ProbeMember) + } + + void "an in-place add after re-initialising an empty hasMany is persisted"() { + given: "a board stored with members: [], which decodes as an empty PersistentList" + ProbeBoard board = new ProbeBoard(name: 'board', members: []).save(flush: true, validate: false) + manager.session.clear() + + when: "the defensive re-init runs — an empty collection is falsy in Groovy — then a member is added" + board = ProbeBoard.get(board.id) + board.trackChanges() + if (!board.members) { + board.members = [] + } + board.members.add(new ProbeMember(userId: 'u1')) + board.save(flush: true) + manager.session.clear() + board = ProbeBoard.get(board.id) + + then: "the member reaches the document" + board.members.size() == 1 + board.members[0].userId == 'u1' + } +} + +@Entity +class ProbeBoard { + ObjectId id + String name + List members + // Load-bearing, as in EmbeddedCollectionDirtyTrackingSpec: the PreUpdate timestamp write + // consumes the whole-class dirty marker an explicit save() sets, so the update carries only + // the properties that were individually tracked. + Date lastUpdated + + static mapWith = "mongo" + static hasMany = [members: ProbeMember] + static constraints = { + members nullable: true + } +} + +@Entity +class ProbeMember { + ObjectId id + String userId + + static mapWith = "mongo" +} diff --git a/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/MappingEngineStringIdStorageSpec.groovy b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/MappingEngineStringIdStorageSpec.groovy new file mode 100644 index 00000000000..99888c85027 --- /dev/null +++ b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/MappingEngineStringIdStorageSpec.groovy @@ -0,0 +1,458 @@ +/* + * 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.bugs + +import com.mongodb.client.MongoCollection +import grails.persistence.Entity +import org.apache.grails.data.mongo.core.GrailsDataMongoTckManager +import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec +import org.bson.Document +import org.bson.types.ObjectId +import org.grails.datastore.mapping.core.Session +import grails.gorm.DetachedCriteria +import org.grails.datastore.mapping.mongo.MongoDatastore +import org.grails.datastore.mapping.mongo.MongoSession +import org.grails.datastore.mapping.mongo.config.MongoSettings +import spock.lang.AutoCleanup +import spock.lang.Shared + +/** + * The non-codec ("mapping") engine must store and query {@code _id}, and write association + * references, in the same type as the codec engine. + * + *

    It reaches MongoDB through a different set of classes -- {@code MongoSession}, + * {@code MongoEntityPersister}, {@code AbstractMongoObectEntityPersister} -- each of which + * previously used the *declared* identifier type. With a bare {@code String id} resolving to + * {@code storedAs: ObjectId} that meant documents written with a String {@code _id} while the + * shared {@code MongoQuery} looked them up as ObjectId, and association references that no + * coerced criterion could match. + * + *

    These cases drive the session API directly rather than the GORM statics, which bind to + * whichever datastore registered the class last -- the codec one here. The first case guards + * that: without it, a green run would prove nothing about this engine. + */ +class MappingEngineStringIdStorageSpec extends GrailsDataTckSpec { + + @Shared + @AutoCleanup + MongoDatastore mappingEngineDatastore + + void setupSpec() { + manager.registerDomainClasses(MeVideo, MeOwner, MeAsset, MeTag, MeNote, + MeRole, MeRight, MeBiParent, MeBiChild, MeFace, MeNose) + mappingEngineDatastore = new MongoDatastore( + manager.configuration + [(MongoSettings.SETTING_ENGINE): 'mapping'], + MeVideo, MeOwner, MeAsset, MeTag, MeNote, + MeRole, MeRight, MeBiParent, MeBiChild, MeFace, MeNose) + } + + void "the datastore under test really is the mapping engine"() { + expect: 'a codec session would prove nothing about any of the cases below' + mappingEngineDatastore.connect().getClass().simpleName == 'MongoSession' + } + + void "insert writes _id in the configured storage type"() { + when: + String hex = persist { Session s -> new MeVideo(title: 'Inserted') } + + then: + hex ==~ /[0-9a-f]{24}/ + raw('meVideo').find(new Document('_id', new ObjectId(hex))).first()?.getString('title') == 'Inserted' + raw('meVideo').find(new Document('_id', hex)).first() == null + } + + void "a point read by id resolves"() { + given: + String hex = persist { Session s -> new MeVideo(title: 'Readable') } + + expect: + mappingEngineDatastore.withSession { Session s -> + s.clear() + s.retrieve(MeVideo, hex)?.title + } == 'Readable' + } + + void "an update lands on the row, rather than silently missing it"() { + given: + String hex = persist { Session s -> new MeVideo(title: 'Before') } + + when: 'the flush-time update filter is built from the identifier' + mappingEngineDatastore.withSession { Session s -> + s.clear() + MeVideo v = s.retrieve(MeVideo, hex) + v.title = 'After' + s.persist(v) + s.flush() + } + + then: + raw('meVideo').find(new Document('_id', new ObjectId(hex))).first().getString('title') == 'After' + } + + void "a delete removes the row, rather than leaving it behind"() { + given: + String hex = persist { Session s -> new MeVideo(title: 'Doomed') } + + when: 'the flush-time delete filter is built from the identifier' + mappingEngineDatastore.withSession { Session s -> + s.clear() + s.delete(s.retrieve(MeVideo, hex)) + s.flush() + } + + then: + raw('meVideo').find(new Document('_id', new ObjectId(hex))).first() == null + } + + void "an iterable delete removes every row"() { + given: 'this path filters on the literal _id field, not the logical identity name' + String a = persist { Session s -> new MeVideo(title: 'Batch A') } + String b = persist { Session s -> new MeVideo(title: 'Batch B') } + + when: + mappingEngineDatastore.withSession { Session s -> + s.clear() + s.delete([s.retrieve(MeVideo, a), s.retrieve(MeVideo, b)]) + s.flush() + } + + then: + raw('meVideo').find(new Document('_id', new ObjectId(a))).first() == null + raw('meVideo').find(new Document('_id', new ObjectId(b))).first() == null + } + + void "a to-one association reference is written in the target's stored _id type"() { + given: + String ownerId = persist { Session s -> new MeOwner(name: 'Owner') } + String assetId = null + mappingEngineDatastore.withSession { Session s -> + MeAsset a = new MeAsset(label: 'Owned', owner: s.retrieve(MeOwner, ownerId)) + s.persist(a); s.flush(); assetId = a.id + } + + when: + Document rawAsset = raw('meAsset').find(new Document('_id', new ObjectId(assetId))).first() + + then: 'the reference is an ObjectId matching MeOwner._id, so a raw join resolves' + rawAsset.get('owner') instanceof ObjectId + rawAsset.get('owner') == new ObjectId(ownerId) + raw('meOwner').find(new Document('_id', rawAsset.get('owner'))).first().getString('name') == 'Owner' + } + + void "a to-one association still resolves through GORM"() { + given: + String ownerId = persist { Session s -> new MeOwner(name: 'Traversed') } + String assetId = null + mappingEngineDatastore.withSession { Session s -> + MeAsset a = new MeAsset(label: 'Traversable', owner: s.retrieve(MeOwner, ownerId)) + s.persist(a); s.flush(); assetId = a.id + } + + expect: + mappingEngineDatastore.withSession { Session s -> + s.clear() + s.retrieve(MeAsset, assetId).owner?.name + } == 'Traversed' + } + + void "updateAll encodes a to-one association rather than writing the domain object"() { + given: + String fromId = persist { Session s -> new MeOwner(name: 'From') } + String toId = persist { Session s -> new MeOwner(name: 'To') } + String assetId = null + mappingEngineDatastore.withSession { Session s -> + MeAsset a = new MeAsset(label: 'Reassign', owner: s.retrieve(MeOwner, fromId)) + s.persist(a); s.flush(); assetId = a.id + } + + when: 'called on the mapping session itself -- a GORM static would bind to the codec datastore' + mappingEngineDatastore.withSession { Session s -> + s.clear() + DetachedCriteria criteria = new DetachedCriteria(MeAsset).build { eq 'label', 'Reassign' } + ((MongoSession) s).updateAll(criteria, [owner: s.retrieve(MeOwner, toId)]) + } + Document rawAsset = raw('meAsset').find(new Document('_id', new ObjectId(assetId))).first() + + then: 'the same representation normal persistence writes' + rawAsset.get('owner') instanceof ObjectId + rawAsset.get('owner') == new ObjectId(toId) + } + + void "a unidirectional one-to-many stores keys in the target's stored _id type"() { + given: 'the keys are held on the owning document, written by the association indexer' + String assetId = null + String tagAId = null, tagBId = null + mappingEngineDatastore.withSession { Session s -> + MeTag a = new MeTag(label: 'uni-a'); MeTag b = new MeTag(label: 'uni-b') + s.persist(a); s.persist(b); s.flush() + tagAId = a.id; tagBId = b.id + MeAsset asset = new MeAsset(label: 'Tagged') + asset.tags = [a, b] + s.persist(asset); s.flush(); assetId = asset.id + } + + when: + Document rawAsset = raw('meAsset').find(new Document('_id', new ObjectId(assetId))).first() + + then: + rawAsset.get('tags').every { it instanceof ObjectId } + rawAsset.get('tags') as Set == [new ObjectId(tagAId), new ObjectId(tagBId)] as Set + } + + void "updateAll on a basic collection passes the values through untouched"() { + given: 'Basic extends ToMany but has no associated entity -- this must not enter the id branch' + String noteId = persist { Session s -> new MeNote(title: 'note', labels: ['x']) } + + when: + mappingEngineDatastore.withSession { Session s -> + s.clear() + DetachedCriteria criteria = new DetachedCriteria(MeNote).build { eq 'title', 'note' } + ((MongoSession) s).updateAll(criteria, [labels: ['y', 'z']]) + } + Document rawNote = raw('meNote').find(new Document('_id', new ObjectId(noteId))).first() + + then: + notThrown(NullPointerException) + rawNote.get('labels') == ['y', 'z'] + } + + void "a unidirectional one-to-many reads back through the coerced keys"() { + given: 'guards the retrieveAllEntities conversion, not just what lands on disk' + String assetId = null + mappingEngineDatastore.withSession { Session s -> + MeTag a = new MeTag(label: 'read-a'); MeTag b = new MeTag(label: 'read-b') + s.persist(a); s.persist(b); s.flush() + MeAsset asset = new MeAsset(label: 'Readable tags') + asset.tags = [a, b] + s.persist(asset); s.flush(); assetId = asset.id + } + + expect: + mappingEngineDatastore.withSession { Session s -> + s.clear() + s.retrieve(MeAsset, assetId).tags*.label as Set + } == ['read-a', 'read-b'] as Set + } + + void "updateAll on a many-to-many writes the key this engine actually reads"() { + given: 'this engine keeps many-to-many ids under a suffixed key -- see setManyToMany' + String roleId = null, rightTwoId = null + mappingEngineDatastore.withSession { Session s -> + MeRight one = new MeRight(label: 'right-one') + MeRight two = new MeRight(label: 'right-two') + s.persist(one); s.persist(two); s.flush() + rightTwoId = two.id + MeRole role = new MeRole(label: 'role') + role.rights = [one] + s.persist(role); s.flush(); roleId = role.id + } + + when: + mappingEngineDatastore.withSession { Session s -> + s.clear() + DetachedCriteria criteria = new DetachedCriteria(MeRole).build { eq 'label', 'role' } + ((MongoSession) s).updateAll(criteria, [rights: [s.retrieve(MeRight, rightTwoId)]]) + } + Document rawRole = raw('meRole').find(new Document('_id', new ObjectId(roleId))).first() + + then: 'written under the suffixed key, in the target stored _id type' + rawRole.get('rights_$$manyToManyIds') == [new ObjectId(rightTwoId)] + + and: 'so the change is visible on read back, rather than being a silent no-op' + mappingEngineDatastore.withSession { Session s -> + s.clear() + s.retrieve(MeRole, roleId).rights*.label + } == ['right-two'] + } + + void "updateAll rejects a bidirectional one-to-many"() { + given: + String parentId = persist { Session s -> new MeBiParent(name: 'bi') } + mappingEngineDatastore.withSession { Session s -> + MeBiChild c = new MeBiChild(name: 'kid', parent: s.retrieve(MeBiParent, parentId)) + s.persist(c); s.flush() + } + + when: + mappingEngineDatastore.withSession { Session s -> + s.clear() + DetachedCriteria criteria = new DetachedCriteria(MeBiParent).build { eq 'name', 'bi' } + ((MongoSession) s).updateAll(criteria, [children: []]) + } + + then: + UnsupportedOperationException e = thrown() + e.message.contains('children') + + and: 'nothing stray was written' + raw('meBiParent').find(new Document('_id', new ObjectId(parentId))).first().get('children') == null + } + + void "updateAll accepts an identifier in place of a to-one instance"() { + given: 'this engine used to send the raw value through $set, so passing an id worked' + String fromId = persist { Session s -> new MeOwner(name: 'Id from') } + String toId = persist { Session s -> new MeOwner(name: 'Id to') } + String assetId = null + mappingEngineDatastore.withSession { Session s -> + MeAsset a = new MeAsset(label: 'Assign by id', owner: s.retrieve(MeOwner, fromId)) + s.persist(a); s.flush(); assetId = a.id + } + + when: + mappingEngineDatastore.withSession { Session s -> + s.clear() + DetachedCriteria criteria = new DetachedCriteria(MeAsset).build { eq 'label', 'Assign by id' } + ((MongoSession) s).updateAll(criteria, [owner: toId]) + } + + then: + raw('meAsset').find(new Document('_id', new ObjectId(assetId))).first().get('owner') == new ObjectId(toId) + } + + void "updateAll rejects a hasOne association"() { + given: 'the two engines implement this separately, so each needs its own case' + String faceId = persist { Session s -> new MeFace(name: 'face') } + String noseId = null + mappingEngineDatastore.withSession { Session s -> + MeNose n = new MeNose(size: 'big', face: s.retrieve(MeFace, faceId)) + s.persist(n); s.flush(); noseId = n.id + } + + when: + mappingEngineDatastore.withSession { Session s -> + s.clear() + DetachedCriteria criteria = new DetachedCriteria(MeFace).build { eq 'name', 'face' } + ((MongoSession) s).updateAll(criteria, [nose: s.retrieve(MeNose, noseId)]) + } + + then: + UnsupportedOperationException e = thrown() + e.message.contains('nose') + } + + private String persist(Closure make) { + String id = null + mappingEngineDatastore.withSession { Session s -> + def o = make(s) + s.persist(o) + s.flush() + id = o.id + } + id + } + + private MongoCollection raw(String name) { + manager.mongoClient.getDatabase('test').getCollection(name) + } +} + +@Entity +class MeVideo { + String id + String title + static mapping = { version false } +} + +@Entity +class MeOwner { + String id + String name + static mapping = { version false } +} + +@Entity +class MeAsset { + String id + String label + MeOwner owner + Set tags = [] + static hasMany = [tags: MeTag] + static mapping = { version false } +} + +@Entity +class MeTag { + String id + String label + static mapping = { version false } +} + +@Entity +class MeNote { + String id + String title + List labels = [] + static hasMany = [labels: String] + static mapping = { version false } +} + +@Entity +class MeRole { + String id + String label + Set rights = [] + static hasMany = [rights: MeRight] + static mapping = { version false } +} + +@Entity +class MeRight { + String id + String label + Set roles = [] + static hasMany = [roles: MeRole] + static belongsTo = MeRole + static mapping = { version false } +} + +@Entity +class MeBiParent { + String id + String name + Set children = [] + static hasMany = [children: MeBiChild] + static mapping = { version false } +} + +@Entity +class MeBiChild { + String id + String name + MeBiParent parent + static belongsTo = [parent: MeBiParent] + static mapping = { version false } +} + +@Entity +class MeFace { + String id + String name + MeNose nose + static hasOne = [nose: MeNose] + static mapping = { version false } +} + +@Entity +class MeNose { + String id + String size + MeFace face + static belongsTo = [face: MeFace] + static mapping = { version false } +} diff --git a/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/MongoIdCoercionSpec.groovy b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/MongoIdCoercionSpec.groovy new file mode 100644 index 00000000000..ce9eed3dd26 --- /dev/null +++ b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/MongoIdCoercionSpec.groovy @@ -0,0 +1,161 @@ +/* + * 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.bugs + +import grails.persistence.Entity +import org.bson.types.ObjectId +import org.grails.datastore.mapping.core.DatastoreUtils +import org.grails.datastore.mapping.model.PersistentEntity +import org.grails.datastore.mapping.mongo.config.MongoMappingContext +import org.grails.datastore.mapping.mongo.config.MongoSettings +import org.grails.datastore.mapping.mongo.connections.MongoConnectionSourceSettingsBuilder +import org.grails.datastore.mapping.mongo.engine.MongoIdCoercion +import spock.lang.Specification + +/** + * Unit coverage for {@link MongoIdCoercion}, the helper both directions of the codec and + * {@code MongoQuery} rely on to move identifiers between the type a domain declares and the + * type actually stored in {@code _id}. + * + * Uses {@link MongoMappingContext} directly -- no live MongoDB -- because this is purely + * about type resolution and conversion. The end-to-end behaviour is covered by + * {@link StringIdWithObjectIdStorageSpec} and {@link StringIdAssociationStorageSpec}. + */ +class MongoIdCoercionSpec extends Specification { + + private MongoMappingContext contextFor(Map props, Class... domainClasses) { + def settings = new MongoConnectionSourceSettingsBuilder(DatastoreUtils.createPropertyResolver(props)).build() + new MongoMappingContext(settings, domainClasses) + } + + private PersistentEntity entityFor(Class domainClass, Map props = [:]) { + contextFor(props, domainClass).getPersistentEntity(domainClass.name) + } + + // ---------- resolveStoredAs ---------- + + void "resolveStoredAs reports the storage type a String-id domain resolved to"() { + expect: + MongoIdCoercion.resolveStoredAs(entityFor(CoercionVideo)) == ObjectId + } + + void "resolveStoredAs reports String when the application pins the old default"() { + expect: + MongoIdCoercion.resolveStoredAs( + entityFor(CoercionVideo, [(MongoSettings.SETTING_STRING_IDS_DEFAULT_STORED_AS): 'string'])) == String + } + + void "resolveStoredAs returns null for a null entity rather than throwing"() { + expect: + MongoIdCoercion.resolveStoredAs(null) == null + } + + // ---------- coerceIdToStoredType ---------- + + void "coerceIdToStoredType converts a hex String to the stored ObjectId"() { + given: + String hex = new ObjectId().toHexString() + + when: + Object coerced = MongoIdCoercion.coerceIdToStoredType(hex, entityFor(CoercionVideo)) + + then: + coerced instanceof ObjectId + ((ObjectId) coerced).toHexString() == hex + } + + void "coerceIdToStoredType leaves a value that is already the stored type untouched"() { + given: + ObjectId id = new ObjectId() + + expect: + MongoIdCoercion.coerceIdToStoredType(id, entityFor(CoercionVideo)).is(id) + } + + void "coerceIdToStoredType returns null for a null key"() { + expect: + MongoIdCoercion.coerceIdToStoredType(null, entityFor(CoercionVideo)) == null + } + + void "coerceIdToStoredType keeps a non-hex natural key as-is instead of producing a null filter"() { + given: 'a natural key that cannot become an ObjectId -- the encoder writes it as BSON String' + String naturalKey = 'jsmith@example.com' + + expect: 'the original value, so queries target what the encoder actually wrote' + MongoIdCoercion.coerceIdToStoredType(naturalKey, entityFor(CoercionVideo)) == naturalKey + } + + // ---------- coerceIdToDeclaredType ---------- + + void "coerceIdToDeclaredType converts a stored ObjectId back to the declared hex String"() { + given: + ObjectId id = new ObjectId() + + when: + Object coerced = MongoIdCoercion.coerceIdToDeclaredType(id, entityFor(CoercionVideo)) + + then: 'domain code sees the String it declared, whatever the storage type is' + coerced instanceof String + coerced == id.toHexString() + } + + void "coerceIdToDeclaredType leaves a value that is already the declared type untouched"() { + given: + String hex = new ObjectId().toHexString() + + expect: + MongoIdCoercion.coerceIdToDeclaredType(hex, entityFor(CoercionVideo)).is(hex) + } + + void "coerceIdToDeclaredType returns the input for null value or null entity"() { + expect: + MongoIdCoercion.coerceIdToDeclaredType(null, entityFor(CoercionVideo)) == null + MongoIdCoercion.coerceIdToDeclaredType('x', null) == 'x' + } + + void "coerceIdToDeclaredType leaves an ObjectId-id domain alone"() { + given: + ObjectId id = new ObjectId() + + expect: 'declared type is already ObjectId, so there is nothing to convert' + MongoIdCoercion.coerceIdToDeclaredType(id, entityFor(CoercionObjectIdVideo)).is(id) + } + + void "the two directions round-trip"() { + given: + PersistentEntity entity = entityFor(CoercionVideo) + String hex = new ObjectId().toHexString() + + expect: + MongoIdCoercion.coerceIdToDeclaredType( + MongoIdCoercion.coerceIdToStoredType(hex, entity), entity) == hex + } +} + +@Entity +class CoercionVideo { + String id + String title +} + +@Entity +class CoercionObjectIdVideo { + ObjectId id + String title +} diff --git a/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdAssociationStorageSpec.groovy b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdAssociationStorageSpec.groovy new file mode 100644 index 00000000000..7ac8d0a8019 --- /dev/null +++ b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdAssociationStorageSpec.groovy @@ -0,0 +1,639 @@ +/* + * 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.bugs + +import com.mongodb.DBRef +import com.mongodb.client.MongoCollection +import grails.persistence.Entity +import org.apache.grails.data.mongo.core.GrailsDataMongoTckManager +import org.apache.grails.data.testing.tck.base.GrailsDataTckSpec +import org.bson.Document +import org.bson.types.ObjectId + +/** + * Association identifiers must be written in the type the target's {@code _id} is stored as. + * + *

    Before this was fixed the encoders wrote a reference using the id's *declared* type, so a + * {@code String id} domain whose {@code _id} is a BSON ObjectId was pointed at by a BSON String. + * GORM traversal still worked -- the decoder coerces on read -- so the mismatch was invisible + * from application code while {@code $lookup}, the raw driver and any external client silently + * matched nothing. + * + *

    These cases assert the BSON that actually lands on disk, not just that traversal works, + * because traversal passed even when the storage was wrong. + */ +class StringIdAssociationStorageSpec extends GrailsDataTckSpec { + + void setupSpec() { + manager.registerDomainClasses(RefProject, RefTicket, RefTag, RefPerson, RefNote, RefBiParent, RefBiChild, RefFace, RefNose, RefLinkedProject, RefRole, RefRight) + } + + void "a to-one reference is written as the target's stored _id type"() { + given: + RefProject project = new RefProject(name: 'Apollo').save(flush: true) + RefTicket ticket = new RefTicket(title: 'Boot failure', project: project).save(flush: true) + + when: + manager.session.clear() + Document raw = rawTickets().find(new Document('_id', new ObjectId(ticket.id))).first() + + then: 'the reference is an ObjectId, matching RefProject._id' + raw.get('project') instanceof ObjectId + raw.get('project') == new ObjectId(project.id) + + and: 'so a raw driver join resolves, which is what was broken' + rawProjects().find(new Document('_id', raw.get('project'))).first().getString('name') == 'Apollo' + } + + void "a to-one reference still resolves through GORM"() { + given: + RefProject project = new RefProject(name: 'Gemini').save(flush: true) + String ticketId = new RefTicket(title: 'Telemetry', project: project).save(flush: true).id + + when: + manager.session.clear() + RefTicket loaded = RefTicket.get(ticketId) + + then: 'the decoder hands back the declared String type' + loaded.project.id == project.id + loaded.project.id instanceof String + loaded.project.name == 'Gemini' + } + + void "a to-many reference array is written as the target's stored _id type"() { + given: + RefTag a = new RefTag(label: 'urgent').save(flush: true) + RefTag b = new RefTag(label: 'backend').save(flush: true) + RefProject project = new RefProject(name: 'Mercury') + project.tags = [a, b] + project.save(flush: true) + + when: + manager.session.clear() + Document raw = rawProjects().find(new Document('_id', new ObjectId(project.id))).first() + + then: + raw.get('tags').every { it instanceof ObjectId } + raw.get('tags') as Set == [new ObjectId(a.id), new ObjectId(b.id)] as Set + } + + void "a to-many collection still resolves through GORM"() { + given: + RefTag a = new RefTag(label: 'ui').save(flush: true) + RefProject project = new RefProject(name: 'Vostok') + project.tags = [a] + project.save(flush: true) + + when: + manager.session.clear() + RefProject loaded = RefProject.get(project.id) + + then: + loaded.tags.size() == 1 + loaded.tags.first().label == 'ui' + } + + void "a bidirectional one-to-many resolves, because the foreign-key filter is coerced too"() { + given: 'the inverse side is fetched by querying tickets whose project is this id' + RefProject project = new RefProject(name: 'Soyuz').save(flush: true) + new RefTicket(title: 'One', project: project).save(flush: true) + new RefTicket(title: 'Two', project: project).save(flush: true) + + when: + manager.session.clear() + List found = RefTicket.findAllByProject(RefProject.get(project.id)) + + then: 'without coercion this sent a hex String against an ObjectId FK and returned nothing' + found.size() == 2 + found*.title as Set == ['One', 'Two'] as Set + } + + void "findAllById resolves, having previously bypassed id coercion entirely"() { + given: "a dynamic finder builds Equals('id', ..) rather than IdEquals" + String id = new RefProject(name: 'Voskhod').save(flush: true).id + + when: + manager.session.clear() + List found = RefProject.findAllById(id) + + then: + found.size() == 1 + found[0].name == 'Voskhod' + } + + void "findAllByIdInList resolves the same ids"() { + given: + String one = new RefProject(name: 'Zond').save(flush: true).id + String two = new RefProject(name: 'Luna').save(flush: true).id + + when: + manager.session.clear() + List found = RefProject.findAllByIdInList([one, two]) + + then: + found.size() == 2 + } + + void "a reference stored as a BSON String is still decoded, so pre-existing data keeps working"() { + given: 'a document written before the storage type changed' + RefProject project = new RefProject(name: 'Apollo 13').save(flush: true) + ObjectId legacyTicketId = new ObjectId() + rawTickets().insertOne( + new Document('_id', legacyTicketId) + .append('title', 'Legacy row') + .append('project', project.id)) // hex String, the old format + + when: + manager.session.clear() + RefTicket loaded = RefTicket.get(legacyTicketId.toHexString()) + + then: 'the decoder reads whatever BSON type is present rather than the predicted one' + loaded.title == 'Legacy row' + loaded.project.id == project.id + loaded.project.name == 'Apollo 13' + } + + void "updateAll writes a to-one reference in the target's stored _id type"() { + given: 'the bulk update path builds its own $set document rather than going through the codec' + RefProject from = new RefProject(name: 'Old owner').save(flush: true) + RefProject to = new RefProject(name: 'New owner').save(flush: true) + RefTicket ticket = new RefTicket(title: 'Reassign me', project: from).save(flush: true) + + when: + manager.session.clear() + RefTicket.where { title == 'Reassign me' }.updateAll(project: RefProject.get(to.id)) + Document raw = rawTickets().find(new Document('_id', new ObjectId(ticket.id))).first() + + then: 'the same representation normal persistence writes, not the declared String' + raw.get('project') instanceof ObjectId + raw.get('project') == new ObjectId(to.id) + + and: 'so the relationship is still queryable afterwards' + manager.session.clear() + RefTicket.get(ticket.id).project.name == 'New owner' + } + + void "an id criterion nested in a not block is coerced"() { + given: 'the inherited negation handler dispatches nested criteria itself, bypassing preprocessing' + String keepId = new RefProject(name: 'Keep').save(flush: true).id + String dropId = new RefProject(name: 'Drop').save(flush: true).id + + when: + manager.session.clear() + List found = RefProject.where { not { eq 'id', dropId } }.list() + + then: 'uncoerced, the nested String predicate excludes nothing and Drop comes back' + found*.id.contains(keepId) + !found*.id.contains(dropId) + } + + void "findAllByIdNot excludes the document it names"() { + given: + String keepId = new RefProject(name: 'Retained').save(flush: true).id + String dropId = new RefProject(name: 'Excluded').save(flush: true).id + + when: + manager.session.clear() + List found = RefProject.findAllByIdNot(dropId) + + then: + found*.id.contains(keepId) + !found*.id.contains(dropId) + } + + void "a to-one association criterion nested in a not block is coerced"() { + given: + RefProject a = new RefProject(name: 'Alpha').save(flush: true) + RefProject b = new RefProject(name: 'Beta').save(flush: true) + new RefTicket(title: 'in alpha', project: a).save(flush: true) + new RefTicket(title: 'in beta', project: b).save(flush: true) + + when: + manager.session.clear() + List found = RefTicket.where { not { eq 'project', RefProject.get(a.id) } }.list() + + then: + found*.title == ['in beta'] + } + + void "an IN criterion on a to-one association coerces each unwrapped id"() { + given: 'getInListQueryValues unwraps instances to their declared id' + RefProject a = new RefProject(name: 'In-A').save(flush: true) + RefProject b = new RefProject(name: 'In-B').save(flush: true) + RefProject c = new RefProject(name: 'In-C').save(flush: true) + new RefTicket(title: 'ta', project: a).save(flush: true) + new RefTicket(title: 'tb', project: b).save(flush: true) + new RefTicket(title: 'tc', project: c).save(flush: true) + + when: + manager.session.clear() + List found = RefTicket.where { + project in [RefProject.get(a.id), RefProject.get(b.id)] + }.list() + + then: 'without coercion these went as hex Strings against ObjectId foreign keys' + found*.title as Set == ['ta', 'tb'] as Set + } + + void "findAllByProjectInList resolves association ids"() { + given: + RefProject a = new RefProject(name: 'List-A').save(flush: true) + RefProject b = new RefProject(name: 'List-B').save(flush: true) + new RefTicket(title: 'la', project: a).save(flush: true) + new RefTicket(title: 'lb', project: b).save(flush: true) + + when: + manager.session.clear() + List found = RefTicket.findAllByProjectInList([RefProject.get(a.id)]) + + then: + found*.title == ['la'] + } + + void "updateAll does not mutate the caller's property map"() { + given: + RefProject to = new RefProject(name: 'Target').save(flush: true) + RefTicket ticket = new RefTicket(title: 'Keep my map', project: to).save(flush: true) + RefProject arg = RefProject.get(to.id) + Map updates = [project: arg] + + when: + manager.session.clear() + RefTicket.where { title == 'Keep my map' }.updateAll(updates) + + then: 'the caller still holds their domain object, not an ObjectId or DBRef' + updates.project.is(arg) + } + + void "updateAll accepts an immutable property map"() { + given: + RefProject to = new RefProject(name: 'Immutable target').save(flush: true) + new RefTicket(title: 'Immutable arg', project: to).save(flush: true) + + when: + manager.session.clear() + RefTicket.where { title == 'Immutable arg' } + .updateAll(Collections.singletonMap('project', RefProject.get(to.id))) + + then: 'normalising into a copy means an unmodifiable argument is fine' + noExceptionThrown() + } + + void "updateAll extracts the id from a lazy proxy"() { + given: 'a proxy keeps its id in the proxy handler, not in the reflected field' + RefProject to = new RefProject(name: 'Proxy target').save(flush: true) + RefTicket ticket = new RefTicket(title: 'Proxy update', project: to).save(flush: true) + RefProject other = new RefProject(name: 'Other').save(flush: true) + RefTicket.where { title == 'Proxy update' }.updateAll(project: other) + + when: + manager.session.clear() + RefTicket.where { title == 'Proxy update' }.updateAll(project: RefProject.load(to.id)) + Document raw = rawTickets().find(new Document('_id', new ObjectId(ticket.id))).first() + + then: 'reflecting the proxy would have yielded null' + raw.get('project') == new ObjectId(to.id) + } + + void "updateAll writes an embedded association as a subdocument, not an id"() { + given: 'Embedded extends ToOne, so id-reference normalization must skip it' + RefPerson p = new RefPerson(name: 'Embedded owner', address: new RefAddress(city: 'before')) + p.save(flush: true) + + when: + manager.session.clear() + RefPerson.where { name == 'Embedded owner' }.updateAll(address: new RefAddress(city: 'after')) + Document raw = rawPeople().find(new Document('_id', new ObjectId(p.id))).first() + + then: 'a subdocument, not a scalar id or null' + raw.get('address') instanceof Document + raw.get('address').getString('city') == 'after' + } + + void "updateAll encodes a to-many association as stored ids"() { + given: 'normal persistence stores ids; the bulk path used to send the domain objects' + RefTag a = new RefTag(label: 'bulk-a').save(flush: true) + RefTag b = new RefTag(label: 'bulk-b').save(flush: true) + RefProject project = new RefProject(name: 'Bulk tags') + project.save(flush: true) + + when: + manager.session.clear() + RefProject.where { name == 'Bulk tags' } + .updateAll(tags: [RefTag.get(a.id), RefTag.get(b.id)]) + Document raw = rawProjects().find(new Document('_id', new ObjectId(project.id))).first() + + then: + raw.get('tags').every { it instanceof ObjectId } + raw.get('tags') as Set == [new ObjectId(a.id), new ObjectId(b.id)] as Set + } + + void "updateAll on a basic collection passes the values through untouched"() { + given: 'Basic extends ToMany but has no associated entity -- this must not enter the id branch' + RefNote note = new RefNote(title: 'note', labels: ['x']).save(flush: true) + + when: + manager.session.clear() + RefNote.where { title == 'note' }.updateAll(labels: ['y', 'z']) + Document raw = rawNotes().find(new Document('_id', new ObjectId(note.id))).first() + + then: 'the String list lands as-is, exactly as it did before this branch existed' + notThrown(NullPointerException) + raw.get('labels') == ['y', 'z'] + } + + void "updateAll rejects a bidirectional one-to-many instead of corrupting the owner"() { + given: 'the foreign key lives on the inverse side, so there is no field here to update' + RefBiParent parent = new RefBiParent(name: 'bi-parent').save(flush: true) + RefBiChild child = new RefBiChild(name: 'kid', parent: parent).save(flush: true) + + when: + manager.session.clear() + RefBiParent.where { name == 'bi-parent' }.updateAll(children: [RefBiChild.get(child.id)]) + + then: 'named clearly, rather than writing raw subdocuments the decoder cannot read' + UnsupportedOperationException e = thrown() + e.message.contains('children') + + and: 'nothing was written, so the owner still decodes' + rawBiParents().find(new Document('_id', new ObjectId(parent.id))).first().get('children') == null + + and: + manager.session.clear() + RefBiParent.get(parent.id).name == 'bi-parent' + } + + void "updateAll rejects a hasOne association"() { + given: 'hasOne keeps the foreign key on the child, so the owner carries no field for it' + RefFace face = new RefFace(name: 'face').save(flush: true) + RefNose nose = new RefNose(size: 'big', face: face).save(flush: true) + + when: + manager.session.clear() + RefFace.where { name == 'face' }.updateAll(nose: RefNose.get(nose.id)) + + then: 'ToOneEncoder writes nothing on the owner for this; neither should updateAll' + UnsupportedOperationException e = thrown() + e.message.contains('nose') + + and: 'no id field on a document that never carries one' + rawFaces().find(new Document('_id', new ObjectId(face.id))).first().get('nose') == null + } + + void "updateAll keeps nulls in a plain to-many id list, as the encoder does"() { + given: 'OneToManyEncoder drops nulls only when wrapping DBRefs; a plain id list keeps them' + RefTag a = new RefTag(label: 'keep-me').save(flush: true) + RefProject project = new RefProject(name: 'Null tags').save(flush: true) + + when: + manager.session.clear() + RefProject.where { name == 'Null tags' }.updateAll(tags: [RefTag.get(a.id), null]) + Document raw = rawProjects().find(new Document('_id', new ObjectId(project.id))).first() + + then: 'a plain id list keeps what the caller passed, as the encoder does' + raw.get('tags') == [new ObjectId(a.id), null] + } + + void "updateAll accepts an identifier in place of a to-one instance"() { + given: + RefProject from = new RefProject(name: 'Id from').save(flush: true) + RefProject to = new RefProject(name: 'Id to').save(flush: true) + RefTicket ticket = new RefTicket(title: 'Assign by id', project: from).save(flush: true) + + when: 'a hex String is an identifier, not an instance to reflect' + manager.session.clear() + RefTicket.where { title == 'Assign by id' }.updateAll(project: to.id) + + then: 'the same stored ObjectId an instance would have produced' + rawTickets().find(new Document('_id', new ObjectId(ticket.id))).first().get('project') == new ObjectId(to.id) + + when: 'and so is an ObjectId' + RefTicket.where { title == 'Assign by id' }.updateAll(project: new ObjectId(from.id)) + + then: + rawTickets().find(new Document('_id', new ObjectId(ticket.id))).first().get('project') == new ObjectId(from.id) + } + + void "updateAll accepts identifiers in place of to-many instances"() { + given: + RefTag a = new RefTag(label: 'id-a').save(flush: true) + RefTag b = new RefTag(label: 'id-b').save(flush: true) + RefProject project = new RefProject(name: 'Tags by id').save(flush: true) + + when: + manager.session.clear() + RefProject.where { name == 'Tags by id' }.updateAll(tags: [a.id, b.id]) + + then: + rawProjects().find(new Document('_id', new ObjectId(project.id))).first().get('tags') == + [new ObjectId(a.id), new ObjectId(b.id)] + } + + void "updateAll drops nulls from a reference to-many before wrapping DBRefs"() { + given: 'OneToManyEncoder filters nulls before wrapping; a DBRef(collection, null) is invalid' + RefTag a = new RefTag(label: 'ref-keep').save(flush: true) + RefLinkedProject project = new RefLinkedProject(name: 'Ref null tags').save(flush: true) + + when: + manager.session.clear() + RefLinkedProject.where { name == 'Ref null tags' }.updateAll(tags: [RefTag.get(a.id), null]) + List stored = rawLinkedProjects().find(new Document('_id', new ObjectId(project.id))).first().get('tags') + + then: 'the null is gone and the one reference is a DBRef carrying the stored ObjectId' + stored.size() == 1 + stored[0] instanceof DBRef + ((DBRef) stored[0]).id == new ObjectId(a.id) + + and: 'and it reads back' + manager.session.clear() + RefLinkedProject.get(project.id).tags*.label == ['ref-keep'] + } + + private MongoCollection rawLinkedProjects() { + manager.mongoClient.getDatabase('test').getCollection('refLinkedProject') + } + + void "updateAll on a many-to-many writes the target stored ids"() { + given: 'this engine shares the unidirectional branch for it, but it is its own association kind' + RefRight one = new RefRight(label: 'right-one').save(flush: true) + RefRight two = new RefRight(label: 'right-two').save(flush: true) + RefRole role = new RefRole(label: 'role') + role.addToRights(one) + role.save(flush: true) + + when: + manager.session.clear() + RefRole.where { label == 'role' }.updateAll(rights: [RefRight.get(two.id)]) + + then: 'an ObjectId array under the plain name, as OneToManyEncoder writes it' + rawRoles().find(new Document('_id', new ObjectId(role.id))).first().get('rights') == [new ObjectId(two.id)] + + and: 'and it reads back' + manager.session.clear() + RefRole.get(role.id).rights*.label == ['right-two'] + } + + private MongoCollection rawRoles() { + manager.mongoClient.getDatabase('test').getCollection('refRole') + } + + private MongoCollection rawFaces() { + manager.mongoClient.getDatabase('test').getCollection('refFace') + } + + private MongoCollection rawBiParents() { + manager.mongoClient.getDatabase('test').getCollection('refBiParent') + } + + private MongoCollection rawNotes() { + manager.mongoClient.getDatabase('test').getCollection('refNote') + } + + private MongoCollection rawPeople() { + manager.mongoClient.getDatabase('test').getCollection('refPerson') + } + + private MongoCollection rawTickets() { + manager.mongoClient.getDatabase('test').getCollection('refTicket') + } + + private MongoCollection rawProjects() { + manager.mongoClient.getDatabase('test').getCollection('refProject') + } +} + +@Entity +class RefProject { + String id + String name + Set tags = [] + static hasMany = [tags: RefTag] + static mapping = { + version false + } +} + +@Entity +class RefTicket { + String id + String title + RefProject project + static mapping = { + version false + } +} + +@Entity +class RefTag { + String id + String label + static mapping = { + version false + } +} + +@Entity +class RefPerson { + String id + String name + RefAddress address + static embedded = ['address'] + static mapping = { version false } +} + +@Entity +class RefAddress { + String city +} + +@Entity +class RefNote { + String id + String title + List labels = [] + static hasMany = [labels: String] + static mapping = { version false } +} + +@Entity +class RefBiParent { + String id + String name + Set children = [] + static hasMany = [children: RefBiChild] + static mapping = { version false } +} + +@Entity +class RefBiChild { + String id + String name + RefBiParent parent + static belongsTo = [parent: RefBiParent] + static mapping = { version false } +} + +@Entity +class RefFace { + String id + String name + RefNose nose + static hasOne = [nose: RefNose] + static mapping = { version false } +} + +@Entity +class RefNose { + String id + String size + RefFace face + static belongsTo = [face: RefFace] + static mapping = { version false } +} + +@Entity +class RefLinkedProject { + String id + String name + Set tags = [] + static hasMany = [tags: RefTag] + static mapping = { + version false + tags reference: true + } +} + +@Entity +class RefRole { + String id + String label + Set rights = [] + static hasMany = [rights: RefRight] + static mapping = { version false } +} + +@Entity +class RefRight { + String id + String label + Set roles = [] + static hasMany = [roles: RefRole] + static belongsTo = RefRole + static mapping = { version false } +} diff --git a/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdDefaultStoredAsConfigSpec.groovy b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdDefaultStoredAsConfigSpec.groovy index 9fb34367e88..4c95fabf058 100644 --- a/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdDefaultStoredAsConfigSpec.groovy +++ b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdDefaultStoredAsConfigSpec.groovy @@ -41,16 +41,30 @@ class StringIdDefaultStoredAsConfigSpec extends Specification { new MongoMappingContext(settings, domainClasses) } - void "without the global config, String id has no storedAs (default behavior unchanged)"() { + void "without the global config, String id defaults to ObjectId storedAs"() { given: MongoMappingContext ctx = contextFor([:], PlainStringIdDomain) when: PersistentEntity entity = ctx.getPersistentEntity(PlainStringIdDomain.name) - then: + then: 'objectid is the default as of 8.0.0 -- no config needed' entity != null - entity.mapping.identifier.storedAs == null + entity.mapping.identifier.storedAs == ObjectId + } + + void "the global default can be pinned back to string for legacy data"() { + given: + MongoMappingContext ctx = contextFor( + [(MongoSettings.SETTING_STRING_IDS_DEFAULT_STORED_AS): 'string'], + PlainStringIdDomain + ) + + when: + PersistentEntity entity = ctx.getPersistentEntity(PlainStringIdDomain.name) + + then: 'the pre-8.0.0 behavior, for applications with existing string _id data' + entity.mapping.identifier.storedAs == String } void "with the global default set to objectid, String id picks up ObjectId storedAs"() { @@ -95,7 +109,41 @@ class StringIdDefaultStoredAsConfigSpec extends Specification { entity.mapping.identifier.storedAs == null } - void "unrecognized storedAs value parses as null (safe fallback, no boot failure)"() { + void "the objectid default applies to the codec engine"() { + given: + MongoMappingContext ctx = contextFor( + [(MongoSettings.SETTING_ENGINE): 'codec'], + PlainStringIdDomain + ) + + expect: + ctx.getPersistentEntity(PlainStringIdDomain.name).mapping.identifier.storedAs == ObjectId + } + + void "the objectid default applies to the non-codec engine too"() { + given: 'both engines coerce _id and association references through the id mapping' + MongoMappingContext ctx = contextFor( + [(MongoSettings.SETTING_ENGINE): 'mapping'], + PlainStringIdDomain + ) + + expect: + ctx.getPersistentEntity(PlainStringIdDomain.name).mapping.identifier.storedAs == ObjectId + } + + void "an explicit value still applies to the non-codec engine"() { + given: 'scoping affects the default only -- an explicit request is still honoured' + MongoMappingContext ctx = contextFor( + [(MongoSettings.SETTING_ENGINE) : 'mapping', + (MongoSettings.SETTING_STRING_IDS_DEFAULT_STORED_AS): 'objectid'], + PlainStringIdDomain + ) + + expect: + ctx.getPersistentEntity(PlainStringIdDomain.name).mapping.identifier.storedAs == ObjectId + } + + void "unrecognized storedAs value falls back to the default (safe, no boot failure)"() { given: MongoMappingContext ctx = contextFor( [(MongoSettings.SETTING_STRING_IDS_DEFAULT_STORED_AS): 'banana'], @@ -105,8 +153,8 @@ class StringIdDefaultStoredAsConfigSpec extends Specification { when: PersistentEntity entity = ctx.getPersistentEntity(PlainStringIdDomain.name) - then: - entity.mapping.identifier.storedAs == null + then: 'a typo warns and yields the default rather than a third, silent behavior' + entity.mapping.identifier.storedAs == ObjectId } } diff --git a/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdWithObjectIdStorageSpec.groovy b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdWithObjectIdStorageSpec.groovy index 5cc21931f8c..ecd9ac68555 100644 --- a/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdWithObjectIdStorageSpec.groovy +++ b/grails-data-mongodb/core/src/test/groovy/org/grails/datastore/gorm/mongo/bugs/StringIdWithObjectIdStorageSpec.groovy @@ -370,6 +370,28 @@ class StringIdWithObjectIdStorageSpec extends GrailsDataTckSpec assignedRawCollection() { + manager.mongoClient + .getDatabase('test') + .getCollection('assignedNonHexVideo') + } + private com.mongodb.client.MongoCollection rawCollection() { manager.mongoClient .getDatabase('test') @@ -387,6 +409,15 @@ class StringIdWithObjectIdStorageSpec extends GrailsDataTckSpec> for details. +The storage type of `String id` fields is controlled globally by `grails.mongodb.stringIds.defaultStoredAs` (values `objectid`, the default as of 8.0.0, or `string`). See <> for details, including the upgrade note for applications with existing String `_id` data. + +==== Persistence Engine (deprecated) + +`grails.mongodb.engine` selects the persistence engine. `codec` is the default and the recommended value. `mapping` remains available for compatibility, but is deprecated. + +The `mapping` engine is deprecated and will be removed in a future release. It reaches MongoDB through a separate persister hierarchy that has to be kept in step with the codec one for every change to the storage layer, and it offers no capability the codec engine lacks. An application setting it should remove the setting: + +[source,yaml] +---- +grails: + mongodb: + engine: mapping # <1> +---- +<1> Deprecated. Remove this to use the default codec engine. + +Selecting it logs a warning at startup. ==== Multi-Document Transactions diff --git a/grails-data-mongodb/docs/src/docs/asciidoc/introduction/upgradeNotes.adoc b/grails-data-mongodb/docs/src/docs/asciidoc/introduction/upgradeNotes.adoc index 4c52dce0b42..c825db8f4bd 100644 --- a/grails-data-mongodb/docs/src/docs/asciidoc/introduction/upgradeNotes.adoc +++ b/grails-data-mongodb/docs/src/docs/asciidoc/introduction/upgradeNotes.adoc @@ -18,6 +18,41 @@ under the License. //// === Upgrade Notes +==== String id Storage Now Defaults to ObjectId + +A domain that declares `String id` without its own `storedAs` mapping now persists `_id` as a BSON `ObjectId`. Before 8.0.0 the default was BSON `String`. + +Application code is unaffected: `person.id` is still the 24-char hex `String`, and `get`, `findAllByIdInList`, updates and deletes still accept hex strings. What changes is the BSON type written to and matched in the database. + +*This is a data-compatibility change.* An application upgrading from 7.x whose collections already hold String `_id` values has two options: + +. Keep the previous behavior by pinning the old default: ++ +[source,yaml] +---- +grails: + mongodb: + stringIds: + defaultStoredAs: string +---- ++ +. Or migrate the stored `_id` values to `ObjectId`. Because `_id` is immutable, each document must be reinserted under the converted id rather than updated in place. + +Domains keyed by a natural string value (slug, email, UUID) should declare the opt-out explicitly, since such values are not valid ObjectId hex: + +[source,groovy] +---- +class UserProfile { + String id // e.g. "jsmith@example.com" + + static mapping = { + id generator: 'assigned', storedAs: String + } +} +---- + +See <> for the full description of `storedAs`. + ==== Dependency Upgrades GORM 7.1 supports Apache Groovy 3, Java 14, MongoDB Driver 4.3 and Spring 5.3.x. diff --git a/grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/dirtyChecking.adoc b/grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/dirtyChecking.adoc index 0b5d1fe32c2..c886e3cab96 100644 --- a/grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/dirtyChecking.adoc +++ b/grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/dirtyChecking.adoc @@ -36,8 +36,34 @@ This method is defined by the `org.grails.datastore.mapping.dirty.checking.Dirty In the case of collections and association types GORM for MongoDB will wrap each collection in a dirty checking aware collection type. -One of the implications of this is if you override the collection with a non-dirty checking aware type it can disable -dirty checking and prevent the property from being updated. +Assigning a new collection over a tracked one keeps dirty checking active: the generated setter wraps the +replacement in the same dirty checking aware type, so a pattern such as re-initialising an empty collection +(`if (!book.authors) book.authors = []`) followed by in-place mutation is tracked and persisted. This holds +for basic collections and maps, embedded collections, and one-to-many and many-to-many associations alike. The wrappers +also track iterator-based mutation — Groovy's `removeAll(Closure)` / `retainAll(Closure)`, `removeIf`, +`ListIterator` operations, `sort` / `replaceAll`, `addFirst` / `addLast` on a `List` property, +`removeFirst` / `removeLast` on a `List` or `SortedSet` property, and the `subList`, `reversed`, +`headSet`, `tailSet` and `subSet` views — and for `Map` properties the default +methods (`putIfAbsent`, `merge`, `compute*`, `replace*`) and removals through the `entrySet()` / `keySet()` / +`values()` views. + +Assigning a collection that belongs to *another* entity re-binds it to the entity being assigned to, so +`bookA.tags = bookB.tags` followed by `bookA.tags.add('classic')` marks `bookA` dirty rather than `bookB`. +The two properties go on sharing the same underlying collection afterwards, exactly as a plain Groovy +assignment would. This applies to the properties GORM wraps in a dirty checking aware collection — basic +collections and maps, and embedded collections. When *borrowed from another entity* a one-to-many or +many-to-many behaves differently: it is held in a `PersistentCollection`, which is stored as-is and left to +the store, as with the store-specific types below. Re-initialising such a property on its own entity is +still tracked, as described above. + +A few paths remain outside automatic dirty checking and still require an explicit `markDirty(propertyName)`: + +* a hand-written setter (one you define yourself rather than the generated one) stores the value it is given + without re-wrapping, so a collection assigned through it loses tracking until the next load; +* mutating an entry's value directly during map iteration (`Map.Entry.setValue`); +* replacing a collection wrapped by a store-specific type (for example the Neo4j collection wrappers) is + deliberately left to that store's persister, which re-wraps the raw replacement on save — that store's + behaviour is unchanged. If any of your updates are not updating the properties that you anticipate you can force an update using the link:../api/org/grails/datastore/mapping/dirty/checking/DirtyCheckable.html#markDirty(java.lang.String)[markDirty()] method: diff --git a/grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/idGeneration.adoc b/grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/idGeneration.adoc index 9465fb48b23..e890e169f1e 100644 --- a/grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/idGeneration.adoc +++ b/grails-data-mongodb/docs/src/docs/asciidoc/objectMapping/idGeneration.adoc @@ -126,9 +126,9 @@ p.save() ==== Decoupling the Declared Type from the Storage Type -When a domain declares `String id`, GORM by default writes `_id` as a BSON `String`. This is convenient in application code — HTTP controllers return clean JSON, URLs embed ids directly, and you never need `new ObjectId(...)` at call sites — but it gives up the native `ObjectId` benefits at the storage layer (12-byte index entries vs. 24-byte hex strings, embedded creation timestamp, etc.). +A domain that declares `String id` is convenient in application code — HTTP controllers return clean JSON, URLs embed ids directly, and you never need `new ObjectId(...)` at call sites. Writing `_id` as a BSON `String` to match would give up the native `ObjectId` benefits at the storage layer (12-byte index entries vs. 24-byte hex strings, embedded creation timestamp, etc.). -You can get both using the `storedAs` mapping option. The domain sees a `String id` (hex form), but `_id` on disk is a BSON `ObjectId`: +The `storedAs` mapping option decouples the two, so you get both: the domain sees a `String id` (hex form), while `_id` on disk is a BSON `ObjectId`. As of Grails 8.0.0 this is the default for any `String id` that declares no `storedAs` of its own — the mapping below is what that default applies for you, and is only needed to be explicit or to override a global `defaultStoredAs: string`: [source,groovy] ---- @@ -154,17 +154,21 @@ This is especially useful when migrating a legacy `ObjectId id` domain to `Strin ===== Global Default -You can opt in globally with an `application.yml` setting, which applies to every domain declaring `String id` that does not specify its own `storedAs`: +As of Grails 8.0.0 this is the *default*: every domain declaring `String id` that does not specify its own `storedAs` stores `_id` as a BSON `ObjectId`. No configuration is required to get the behavior described above. + +To opt the whole application back out — for example an existing application whose data already holds BSON String `_id` values — pin the previous default in `application.yml`: [source,yaml] ---- grails: mongodb: stringIds: - defaultStoredAs: objectid + defaultStoredAs: string ---- -Valid values are `string` (current default, BSON String) and `objectid` (BSON ObjectId). Unrecognized values fall back to the default with a warning logged at startup. +Valid values are `objectid` (BSON ObjectId, the default) and `string` (BSON String, the pre-8.0.0 default). Unrecognized values fall back to the default with a warning logged at startup. + +WARNING: This default changed in 8.0.0. An application upgrading from 7.x with existing `String id` data is reading and writing BSON String `_id` values; on 8.0.0 those documents are no longer matched by id lookups unless you either migrate the stored `_id` values to `ObjectId` or set `defaultStoredAs: string` above. Per-domain `storedAs` always wins over the global default. Domains using a natural string key (slug, email, UUID) should opt out explicitly: diff --git a/grails-data-mongodb/docs/src/docs/asciidoc/querying/queryingBasics.adoc b/grails-data-mongodb/docs/src/docs/asciidoc/querying/queryingBasics.adoc index b3a0cd2e95b..580dcadb27a 100644 --- a/grails-data-mongodb/docs/src/docs/asciidoc/querying/queryingBasics.adoc +++ b/grails-data-mongodb/docs/src/docs/asciidoc/querying/queryingBasics.adoc @@ -41,3 +41,48 @@ include::{grailsDataMongodbCoreDir}/src/test/groovy/grails/gorm/tests/FindNative ... include::{grailsDataMongodbCoreDir}/src/test/groovy/grails/gorm/tests/FindNativeSpec.groovy[tags=collectionFind,indent=0] ---- + +=== Bulk Updates + +`updateAll` sets the given properties on every document the criteria match, in a single `updateMany`, without loading the entities: + +[source,groovy] +---- +Ticket.where { status == 'open' }.updateAll(assignee: user) +---- + +An association value is written the way normal persistence writes it: the target's identifier, in the type that target's `_id` is stored as, and as a `DBRef` where the mapping declares `reference: true`. Either the associated instance or its identifier can be passed, so `updateAll(assignee: user)` and `updateAll(assignee: user.id)` write the same value. + +Whether an association can be bulk-updated depends on which document holds the reference: + +[cols="2,1,3"] +|=== +|Association |Supported |Reason + +|to-one reference +|Yes +|The reference is a field on this document. + +|unidirectional one-to-many, many-to-many +|Yes +|The identifiers are stored as an array on this document. + +|embedded, and collections of simple values such as `hasMany = [labels: String]` +|Yes +|The value is written like any other property. + +|`hasOne` +|No +|The foreign key is held by the associated document. + +|bidirectional one-to-many +|No +|The foreign key is held by each child. +|=== + +The unsupported kinds throw `UnsupportedOperationException` rather than writing a field normal persistence never reads. Update the side that holds the foreign key instead. For a bidirectional one-to-many, that means updating the children: + +[source,groovy] +---- +Child.where { id in childIds }.updateAll(parent: newParent) +---- diff --git a/grails-data-mongodb/embedded/build.gradle b/grails-data-mongodb/embedded/build.gradle index 59a10499d75..c595107b6e1 100644 --- a/grails-data-mongodb/embedded/build.gradle +++ b/grails-data-mongodb/embedded/build.gradle @@ -78,7 +78,7 @@ dependencies { testImplementation 'org.spockframework:spock-core' testImplementation 'org.mongodb:mongodb-driver-sync' // proves each backend answers a real driver - testRuntimeOnly 'org.slf4j:slf4j-nop' + testRuntimeOnly 'org.slf4j:slf4j-simple' // includes initializer diagnostics in the child JVM's log } apply { diff --git a/grails-data-mongodb/embedded/src/main/java/org/grails/datastore/gorm/mongodb/embedded/InMemoryMongoBackend.java b/grails-data-mongodb/embedded/src/main/java/org/grails/datastore/gorm/mongodb/embedded/InMemoryMongoBackend.java index 6fe46b29fb7..6c79126db94 100644 --- a/grails-data-mongodb/embedded/src/main/java/org/grails/datastore/gorm/mongodb/embedded/InMemoryMongoBackend.java +++ b/grails-data-mongodb/embedded/src/main/java/org/grails/datastore/gorm/mongodb/embedded/InMemoryMongoBackend.java @@ -128,8 +128,25 @@ public int getPort() { return this.port; } + /** + * Stopping twice is what an ordinary shutdown does: the lifecycle bean stops the server when + * the application context closes, and the JVM shutdown hook - which is there for a context + * that never closes - runs after it. A server that is already stopped is left alone. + * + *

    Shutting a {@code MongoServer} down a second time is not harmless. It closes the clients + * it still tracks and waits, without a timeout, for them to close, and a client that connected + * as the first shutdown began can be left tracked on an event loop that has since terminated, + * where closing it never completes. From the shutdown hook, that wait kept the JVM from + * exiting at all. + * + *

    Synchronized because the context's shutdown hook and the initializer's run at the same + * time on the way out. + */ @Override - public void stop() { + public synchronized void stop() { + if (!this.running) { + return; + } this.server.shutdownNow(); this.running = false; } @@ -140,7 +157,7 @@ public boolean isRunning() { } @Override - public void restart() { + public synchronized void restart() { MongoServer restarted = new MongoServer(this.backend); restarted.bind("localhost", this.port); this.server = restarted; diff --git a/grails-data-mongodb/embedded/src/main/java/org/grails/datastore/gorm/mongodb/embedded/RunningEmbeddedMongo.java b/grails-data-mongodb/embedded/src/main/java/org/grails/datastore/gorm/mongodb/embedded/RunningEmbeddedMongo.java index a5516952e04..e5a390ea713 100644 --- a/grails-data-mongodb/embedded/src/main/java/org/grails/datastore/gorm/mongodb/embedded/RunningEmbeddedMongo.java +++ b/grails-data-mongodb/embedded/src/main/java/org/grails/datastore/gorm/mongodb/embedded/RunningEmbeddedMongo.java @@ -38,6 +38,7 @@ public interface RunningEmbeddedMongo { /** * Stops the server. Called from a JVM shutdown hook, so it must not throw. + * May be called again after the server has stopped, in which case it must do nothing. */ void stop(); diff --git a/grails-data-mongodb/embedded/src/test/groovy/org/grails/datastore/gorm/mongodb/embedded/EmbeddedMongoInitializerSpec.groovy b/grails-data-mongodb/embedded/src/test/groovy/org/grails/datastore/gorm/mongodb/embedded/EmbeddedMongoInitializerSpec.groovy index c28a7b975bc..47f555cff87 100644 --- a/grails-data-mongodb/embedded/src/test/groovy/org/grails/datastore/gorm/mongodb/embedded/EmbeddedMongoInitializerSpec.groovy +++ b/grails-data-mongodb/embedded/src/test/groovy/org/grails/datastore/gorm/mongodb/embedded/EmbeddedMongoInitializerSpec.groovy @@ -35,11 +35,10 @@ import java.nio.file.Path /** * Exercises both backends against real servers, and stops every one of them. * - *

    Leaving them to the shutdown hook the initializer registers does not work here: a Gradle test - * worker cannot exit while a server it started holds a thread that is not a daemon, and the hook - * that would stop the server runs only on the way out. The worker waits for what only its own exit - * would release, Gradle waits for the worker, and a build whose tests have all passed never - * finishes. + *

    Leaving them to the shutdown hook the initializer registers is not enough here. That hook runs + * only as the test worker exits, after every test has reported, so a stop that fails there names no + * feature. And the worker exits through {@code System.exit}, which waits for every shutdown hook to + * finish: a stop that hangs there keeps a build whose tests have all passed from ever finishing. */ class EmbeddedMongoInitializerSpec extends Specification { diff --git a/grails-data-mongodb/embedded/src/test/groovy/org/grails/datastore/gorm/mongodb/embedded/EmbeddedMongoLifecycleSpec.groovy b/grails-data-mongodb/embedded/src/test/groovy/org/grails/datastore/gorm/mongodb/embedded/EmbeddedMongoLifecycleSpec.groovy index 99975625711..02adee21cdf 100644 --- a/grails-data-mongodb/embedded/src/test/groovy/org/grails/datastore/gorm/mongodb/embedded/EmbeddedMongoLifecycleSpec.groovy +++ b/grails-data-mongodb/embedded/src/test/groovy/org/grails/datastore/gorm/mongodb/embedded/EmbeddedMongoLifecycleSpec.groovy @@ -31,6 +31,8 @@ import spock.lang.TempDir import spock.util.concurrent.PollingConditions import java.nio.file.Path +import java.util.concurrent.CompletableFuture +import java.util.concurrent.TimeUnit /** * Exercises the stop and restart that a CRaC checkpoint and restore drive. @@ -150,6 +152,7 @@ class EmbeddedMongoLifecycleSpec extends Specification { then: 'the second stop finds the files the first one removed, and says nothing of it' noExceptionThrown() + !running.running conditions.eventually { assert !listening(27979) } } @@ -186,6 +189,7 @@ class EmbeddedMongoLifecycleSpec extends Specification { then: noExceptionThrown() + !running.running conditions.eventually { assert !listening(27978) } } @@ -275,19 +279,38 @@ class EmbeddedMongoLifecycleSpec extends Specification { second?.stop() } - void 'stopping a server that is already stopped is what a shutdown hook does'() { - given: 'an in-memory server the context has stopped' + void 'stopping a server that is already stopped is what a shutdown hook does on port #port'() { + given: 'an in-memory server the context stopped just after a client connected to it' RunningEmbeddedMongo running = new InMemoryMongoBackend() - .start(new EmbeddedMongoSettings(27974, null, null)) - new Socket('localhost', 27974).withCloseable { } + .start(new EmbeddedMongoSettings(port, null, null)) + new Socket('localhost', port).withCloseable { } running.stop() - when: 'the JVM shutdown hook stops it again on the way out' - running.stop() + when: 'the JVM shutdown hook stops it again on the way out, which has to return for the JVM to exit' + CompletableFuture.runAsync { running.stop() }.get(30, TimeUnit.SECONDS) - then: 'it says nothing, because a hook has nowhere to report to' + then: 'it returns, and says nothing, because a hook has nowhere to report to' noExceptionThrown() !running.running + + where: 'ten attempts give the race more chances to reproduce, on ports no other feature uses' + port << (27950..27959) + } + + void 'an application whose context stopped its in-memory server still exits'() { + given: + File log = temp.resolve('application.log').toFile() + + when: 'an application starts a server, lets a client connect, stops it as its context closes and returns' + Process application = startApplication(27969, log) + boolean exited = application.waitFor(30, TimeUnit.SECONDS) + + then: 'the JVM exits, rather than its shutdown hook waiting forever to stop a server that is already stopped' + assert exited : "still running 30 seconds after its main method returned:\n${log.text}" + assert application.exitValue() == 0 : log.text + + cleanup: + application?.destroyForcibly() } private static void write(String url, String title) { @@ -322,4 +345,30 @@ class EmbeddedMongoLifecycleSpec extends Specification { context.environment.propertySources.addFirst(new MapPropertySource('test', properties)) context } + + /** + * {@link ContextStoppedServerApplication} in a JVM of its own, because whether that JVM exits is + * the question. The command is written to an argument file, since a test class path is longer + * than some platforms allow a command line to be. + */ + private Process startApplication(int port, File log) { + File arguments = temp.resolve('application.args').toFile() + arguments.text = [ + '-cp', + System.getProperty('java.class.path'), + ContextStoppedServerApplication.name, + String.valueOf(port), + ].collect { String argument -> quoted(argument) }.join('\n') + Process application = new ProcessBuilder( + new File(System.getProperty('java.home'), 'bin/java').absolutePath, + "@${arguments.absolutePath}".toString() + ).redirectErrorStream(true).redirectOutput(log).start() + application.outputStream.close() + application + } + + /** An argument file separates arguments at whitespace, which a path may contain. */ + private static String quoted(String argument) { + '"' + argument.replace('\\', '\\\\').replace('"', '\\"') + '"' + } } diff --git a/grails-data-mongodb/embedded/src/test/groovy/org/grails/datastore/gorm/mongodb/embedded/EmbeddedMongoReplicaSetSpec.groovy b/grails-data-mongodb/embedded/src/test/groovy/org/grails/datastore/gorm/mongodb/embedded/EmbeddedMongoReplicaSetSpec.groovy index d5408b289d1..3dc839ec673 100644 --- a/grails-data-mongodb/embedded/src/test/groovy/org/grails/datastore/gorm/mongodb/embedded/EmbeddedMongoReplicaSetSpec.groovy +++ b/grails-data-mongodb/embedded/src/test/groovy/org/grails/datastore/gorm/mongodb/embedded/EmbeddedMongoReplicaSetSpec.groovy @@ -35,6 +35,23 @@ import spock.lang.Specification */ class EmbeddedMongoReplicaSetSpec extends Specification { + private final List contexts = [] + + /** + * Stopped through the bean the initializer registers rather than by closing the context: a + * context that was never refreshed is not active, and closing one that is not active does + * nothing at all - which left every mongod these features started running until the JVM + * shutdown hook stopped it on the way out. + */ + void cleanup() { + contexts.each { GenericApplicationContext context -> + if (context.beanFactory.containsBean(EmbeddedMongoLifecycle.BEAN_NAME)) { + context.beanFactory.getBean(EmbeddedMongoLifecycle.BEAN_NAME, EmbeddedMongoLifecycle).stop() + } + } + contexts.clear() + } + void 'a server asked for a replica set commits a transaction across two collections'() { given: GenericApplicationContext context = contextWith([ @@ -62,7 +79,6 @@ class EmbeddedMongoReplicaSetSpec extends Specification { cleanup: session?.close() client?.close() - context?.close() } void 'a transaction that is not committed leaves nothing behind'() { @@ -88,7 +104,6 @@ class EmbeddedMongoReplicaSetSpec extends Specification { cleanup: session?.close() client?.close() - context?.close() } void 'an application that asks GORM for transactions is given a replica set without naming one'() { @@ -106,7 +121,6 @@ class EmbeddedMongoReplicaSetSpec extends Specification { cleanup: client?.close() - context?.close() } void 'a server nobody asked to replicate is left standalone'() { @@ -123,12 +137,12 @@ class EmbeddedMongoReplicaSetSpec extends Specification { cleanup: client?.close() - context?.close() } private GenericApplicationContext contextWith(Map properties) { GenericApplicationContext context = new GenericApplicationContext() context.environment.propertySources.addFirst(new MapPropertySource('test', properties)) + contexts << context context } diff --git a/grails-data-mongodb/embedded/src/test/java/org/grails/datastore/gorm/mongodb/embedded/ContextStoppedServerApplication.java b/grails-data-mongodb/embedded/src/test/java/org/grails/datastore/gorm/mongodb/embedded/ContextStoppedServerApplication.java new file mode 100644 index 00000000000..97f7bb02185 --- /dev/null +++ b/grails-data-mongodb/embedded/src/test/java/org/grails/datastore/gorm/mongodb/embedded/ContextStoppedServerApplication.java @@ -0,0 +1,56 @@ +/* + * 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.mongodb.embedded; + +import java.io.IOException; +import java.net.Socket; +import java.util.Map; + +import org.springframework.context.support.GenericApplicationContext; +import org.springframework.core.env.MapPropertySource; + +/** + * An application that starts an in-memory server the way any application does, lets a client + * connect, and stops the server as its context closes. Returning from {@code main} is all that is + * left, and the JVM then exits through the shutdown hook the initializer registered, which stops the + * server a second time. + * + *

    {@code EmbeddedMongoLifecycleSpec} runs it in a JVM of its own, since whether that JVM exits is + * the question. The shutdown hang is a race: this Java application reproduced it reliably on the + * author's machine, while another machine reproduced it only in the in-process Spock feature. + * Both tests exercise the race, but neither is guaranteed to reproduce it without the fix. + */ +public final class ContextStoppedServerApplication { + + private ContextStoppedServerApplication() { + } + + public static void main(String[] args) throws IOException { + int port = Integer.parseInt(args[0]); + GenericApplicationContext context = new GenericApplicationContext(); + context.getEnvironment().getPropertySources().addFirst(new MapPropertySource("application", Map.of( + EmbeddedMongoInitializer.BACKEND, InMemoryMongoBackend.NAME, + "grails.mongodb.url", "mongodb://embedded:" + port + "/bookstore"))); + new EmbeddedMongoInitializer().initialize(context); + EmbeddedMongoLifecycle lifecycle = context.getBeanFactory() + .getBean(EmbeddedMongoLifecycle.BEAN_NAME, EmbeddedMongoLifecycle.class); + new Socket("localhost", port).close(); + lifecycle.stop(); + } +} diff --git a/grails-data-neo4j/core/src/test/groovy/grails/gorm/tests/HasManyReassignDirtyCheckingSpec.groovy b/grails-data-neo4j/core/src/test/groovy/grails/gorm/tests/HasManyReassignDirtyCheckingSpec.groovy new file mode 100644 index 00000000000..08eeac75716 --- /dev/null +++ b/grails-data-neo4j/core/src/test/groovy/grails/gorm/tests/HasManyReassignDirtyCheckingSpec.groovy @@ -0,0 +1,77 @@ +/* + * 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.tests + +import grails.gorm.annotation.Entity +import grails.neo4j.Neo4jEntity +import org.apache.grails.data.neo4j.core.Neo4jGormDatastoreSpec + +/** + * Reassigning a hasMany must leave the raw collection for Neo4jEntityPersister to wrap in its + * own relationship-aware type on save. DirtyCheckingSupport.rewrap only re-wraps values whose + * current wrapper is one of the plugin's exact generic classes — a store-specific subclass + * (Neo4jList et al.) is left alone, because a generic replacement makes + * createDirtyCheckableAwareCollection take its "already dirty-checkable" branch and in-place + * removals then stop deleting relationships (adds still MERGE, so only removals go missing). + */ +class HasManyReassignDirtyCheckingSpec extends Neo4jGormDatastoreSpec { + + void setupSpec() { + manager.registerDomainClasses(ReassignPlaylist, ReassignSong) + } + + void "in-place removal after a hasMany reassignment still deletes the relationship"() { + given: + ReassignSong a = new ReassignSong(title: 'a').save(flush: true) + ReassignSong b = new ReassignSong(title: 'b').save(flush: true) + ReassignSong c = new ReassignSong(title: 'c').save(flush: true) + ReassignPlaylist playlist = new ReassignPlaylist(name: 'mix').save(flush: true, validate: false) + manager.session.clear() + + when: 'the association is reassigned wholesale, saved, then mutated in place and saved again' + playlist = ReassignPlaylist.get(playlist.id) + playlist.trackChanges() + a = ReassignSong.get(a.id) + playlist.songs = [a, ReassignSong.get(b.id), ReassignSong.get(c.id)] + playlist.save(flush: true) + playlist.songs.remove(a) + playlist.save(flush: true) + manager.session.clear() + playlist = ReassignPlaylist.get(playlist.id) + + then: 'the removed relationship is gone after reload' + playlist.songs.size() == 2 + playlist.songs*.title as Set == ['b', 'c'] as Set + } +} + +@Entity +class ReassignPlaylist implements Neo4jEntity { + String name + List songs + static hasMany = [songs: ReassignSong] + static constraints = { + songs nullable: true + } +} + +@Entity +class ReassignSong implements Neo4jEntity { + String title +} diff --git a/grails-datamapping-core/src/main/groovy/grails/gorm/api/GormStaticOperations.groovy b/grails-datamapping-core/src/main/groovy/grails/gorm/api/GormStaticOperations.groovy index 1b31ee5fb6b..29793e063a9 100644 --- a/grails-datamapping-core/src/main/groovy/grails/gorm/api/GormStaticOperations.groovy +++ b/grails-datamapping-core/src/main/groovy/grails/gorm/api/GormStaticOperations.groovy @@ -235,12 +235,12 @@ interface GormStaticOperations { * Counts the number of persisted entities * @return The number of persisted entities */ - Integer count() + Long count() /** * Same as {@link #count()} but allows property-style syntax (Foo.count) */ - Integer getCount() + Long getCount() /** * Checks whether an entity exists diff --git a/grails-datamapping-core/src/main/groovy/org/grails/compiler/gorm/DirtyCheckingTransformer.groovy b/grails-datamapping-core/src/main/groovy/org/grails/compiler/gorm/DirtyCheckingTransformer.groovy index 5133f149ebd..272f887e5e1 100644 --- a/grails-datamapping-core/src/main/groovy/org/grails/compiler/gorm/DirtyCheckingTransformer.groovy +++ b/grails-datamapping-core/src/main/groovy/org/grails/compiler/gorm/DirtyCheckingTransformer.groovy @@ -31,6 +31,7 @@ import org.codehaus.groovy.ast.Parameter import org.codehaus.groovy.ast.PropertyNode import org.codehaus.groovy.ast.Variable import org.codehaus.groovy.ast.expr.ConstantExpression +import org.codehaus.groovy.ast.expr.Expression import org.codehaus.groovy.ast.expr.ListExpression import org.codehaus.groovy.ast.expr.MethodCallExpression import org.codehaus.groovy.ast.stmt.BlockStatement @@ -45,6 +46,7 @@ import org.springframework.validation.annotation.Validated import grails.gorm.dirty.checking.DirtyCheck import grails.gorm.dirty.checking.DirtyCheckedProperty import org.grails.datastore.mapping.dirty.checking.DirtyCheckable +import org.grails.datastore.mapping.dirty.checking.DirtyCheckingSupport import org.grails.datastore.mapping.model.config.GormProperties import org.grails.datastore.mapping.reflect.ClassUtils import org.grails.datastore.mapping.reflect.NameUtils @@ -58,6 +60,7 @@ import static org.codehaus.groovy.ast.tools.GeneralUtils.args import static org.codehaus.groovy.ast.tools.GeneralUtils.assignS import static org.codehaus.groovy.ast.tools.GeneralUtils.block import static org.codehaus.groovy.ast.tools.GeneralUtils.callX +import static org.codehaus.groovy.ast.tools.GeneralUtils.castX import static org.codehaus.groovy.ast.tools.GeneralUtils.constX import static org.codehaus.groovy.ast.tools.GeneralUtils.param import static org.codehaus.groovy.ast.tools.GeneralUtils.params @@ -91,6 +94,13 @@ class DirtyCheckingTransformer implements CompilationUnitAware { public static final ClassNode DIRTY_CHECKED_PROPERTY_CLASS_NODE = ClassHelper.make(DirtyCheckedProperty) public static final ClassNode DIRTY_CHECK_CLASS_NODE = ClassHelper.make(DirtyCheck) public static final AnnotationNode DIRTY_CHECKED_PROPERTY_ANNOTATION_NODE = new AnnotationNode(DIRTY_CHECKED_PROPERTY_CLASS_NODE) + private static final ClassNode DIRTY_CHECKING_SUPPORT_CLASS_NODE = ClassHelper.make(DirtyCheckingSupport) + // Interface-typed collection properties whose generated setter re-establishes change + // tracking via DirtyCheckingSupport.rewrap. Restricted to the exact interfaces the + // DirtyChecking* wrappers implement so the cast in the generated setter is always valid. + private static final Set REWRAPPABLE_TYPE_NAMES = [ + Collection.name, List.name, Set.name, SortedSet.name, Map.name + ] as Set static { if (ClassUtils.isPresent('jakarta.validation.Constraint')) { @@ -362,7 +372,21 @@ class DirtyCheckingTransformer implements CompilationUnitAware { final BlockStatement setterBody = new BlockStatement() MethodCallExpression markDirtyMethodCall = createMarkDirtyMethodCall(markDirtyMethodNode, propertyName, setterParameter) setterBody.addStatement(stmt(markDirtyMethodCall)) - setterBody.addStatement(assignS(propX(varX('this'), fieldName), varX(setterParameter))) + // Collection/Map-typed properties assign through DirtyCheckingSupport.rewrap so a + // value that replaces a tracked wrapper (installed by an interception-based store's + // decoder) stays tracked. Without this, `entity.items = []` over a tracked list + // stored a plain untracked collection and every later in-place mutation was + // invisible to change tracking. rewrap is a no-op when the old value was untracked, + // so stores with their own dirty checking (Hibernate) are unaffected. + Expression assignedValue + if (REWRAPPABLE_TYPE_NAMES.contains(returnType.name)) { + assignedValue = castX(returnType.plainNodeReference, callX( + DIRTY_CHECKING_SUPPORT_CLASS_NODE, 'rewrap', + args(varX('this'), constX(propertyName), propX(varX('this'), fieldName), varX(setterParameter)))) + } else { + assignedValue = varX(setterParameter) + } + setterBody.addStatement(assignS(propX(varX('this'), fieldName), assignedValue)) setter = classNode.addMethod(setterName, PUBLIC, ClassHelper.VOID_TYPE, params(setterParameter), ClassNode.EMPTY_ARRAY, setterBody) markAsGenerated(classNode, setter) diff --git a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormEntity.groovy b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormEntity.groovy index a9f6792221d..193eed92877 100644 --- a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormEntity.groovy +++ b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/GormEntity.groovy @@ -729,7 +729,7 @@ trait GormEntity implements GormValidateable, DirtyCheckable, GormEntityApi implements GormValidateable, DirtyCheckable, GormEntityApi extends AbstractGormApi implements GormAllOperations + Long result = execute({ Session session -> def query = session.createQuery(persistentClass) query.projections().count() def res = query.singleResult() logger.debug('Query singleResult returned {}', res) - res instanceof Number ? ((Number)res).intValue() : 0 - } as SessionCallback) + res instanceof Number ? ((Number)res).longValue() : 0L + } as SessionCallback) logger.debug('count() result is {}', result) return result } @Override - Integer getCount() { + Long getCount() { count() } diff --git a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/multitenancy/TenantDelegatingGormOperations.groovy b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/multitenancy/TenantDelegatingGormOperations.groovy index c5e0a99a01c..f864a19600a 100644 --- a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/multitenancy/TenantDelegatingGormOperations.groovy +++ b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/multitenancy/TenantDelegatingGormOperations.groovy @@ -386,14 +386,14 @@ class TenantDelegatingGormOperations implements GormAllOperations { } @Override - Integer count() { + Long count() { Tenants.withId(requireMultiTenantCapableDatastore(), tenantId) { allOperations.count() } } @Override - Integer getCount() { + Long getCount() { Tenants.withId(requireMultiTenantCapableDatastore(), tenantId) { allOperations.getCount() } diff --git a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/GormStaticApiSpec.groovy b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/GormStaticApiSpec.groovy index 62ec8c72ccc..263e8ca5806 100644 --- a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/GormStaticApiSpec.groovy +++ b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/GormStaticApiSpec.groovy @@ -28,6 +28,7 @@ import org.grails.datastore.mapping.core.connections.ConnectionSources import org.grails.datastore.mapping.core.connections.ConnectionSourcesProvider import org.grails.datastore.mapping.core.connections.MultipleConnectionSourceCapableDatastore import org.grails.datastore.mapping.model.MappingContext +import org.grails.datastore.mapping.query.Query import org.grails.datastore.mapping.multitenancy.MultiTenancySettings import org.grails.datastore.mapping.multitenancy.MultiTenantCapableDatastore import org.grails.datastore.mapping.simple.SimpleMapDatastore @@ -105,10 +106,33 @@ class GormStaticApiSpec extends Specification { def api = new GormStaticApi(GormStaticApiThing, datastore, []) when: - Integer n = api.count() + Long n = api.count() then: - n == 0 + n == 0L + } + + void "count() preserves a datastore count above Integer.MAX_VALUE"() { + given: + def ds = Stub(Datastore) + def session = Stub(Session) + def query = Mock(Query) + ds.getMappingContext() >> datastore.mappingContext + ds.connect() >> session + session.getDatastore() >> ds + session.createQuery(GormStaticApiThing) >> query + query.projections() >> Mock(Query.ProjectionList) + query.singleResult() >> 3_000_000_000L + def api = new GormStaticApi(GormStaticApiThing, ds, []) + + expect: 'the value survives instead of wrapping to a negative int' + api.count() == 3_000_000_000L + } + + void "the count members a domain class exposes are Long"() { + expect: + GormStaticApiThing.count() instanceof Long + GormStaticApiThing.count instanceof Long } void "getGormDynamicFinders returns the finders the api was constructed with"() { diff --git a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/dirty/checking/DirtyCheckCollectionReassignmentSpec.groovy b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/dirty/checking/DirtyCheckCollectionReassignmentSpec.groovy new file mode 100644 index 00000000000..73945b37a5a --- /dev/null +++ b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/dirty/checking/DirtyCheckCollectionReassignmentSpec.groovy @@ -0,0 +1,458 @@ +/* + * 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.dirty.checking + +import org.grails.datastore.mapping.collection.PersistentList +import org.grails.datastore.mapping.collection.PersistentSet +import org.grails.datastore.mapping.collection.PersistentSortedSet +import org.grails.datastore.mapping.core.Session +import org.grails.datastore.mapping.dirty.checking.DirtyCheckableCollection +import org.grails.datastore.mapping.dirty.checking.DirtyCheckable +import org.grails.datastore.mapping.dirty.checking.DirtyCheckingCollection +import org.grails.datastore.mapping.dirty.checking.DirtyCheckingList +import org.grails.datastore.mapping.dirty.checking.DirtyCheckingMap +import org.grails.datastore.mapping.dirty.checking.DirtyCheckingSortedSet +import org.grails.datastore.mapping.dirty.checking.DirtyCheckingSupport + +import spock.lang.Shared +import spock.lang.Specification + +/** + * Interception-based stores (MongoDB et al.) install DirtyChecking* wrappers on collection + * properties when an entity is decoded, and rely on them exclusively — there is no flush-time + * snapshot comparison. The generated dirty-checking setter, however, used to store whatever + * raw value it was handed, so reassigning a collection property replaced the tracked wrapper + * with a plain untracked collection and every later in-place mutation became invisible to + * {@code hasChanged()}; a subsequent save() persisted nothing. + * + *

    The observed real-world shape (schedule sharing): an empty tracked list is falsy in + * Groovy, so the common defensive re-init {@code if (!entity.shares) entity.shares = []} + * always replaced the tracked wrapper, and because {@code [] == []} the equality-suppressed + * markDirty never even flagged the assignment. The add() that followed was silently lost. + * + *

    The setter must therefore re-wrap: when the value being replaced was tracked, the + * replacement collection is wrapped too. A property that was never tracked (a transient + * instance, or a store like Hibernate that never installs these wrappers) is left untouched. + */ +class DirtyCheckCollectionReassignmentSpec extends Specification { + + @Shared + Class entityClass + + @Shared + Class keyedEntityClass + + def setupSpec() { + def gcl = new GroovyClassLoader() + keyedEntityClass = gcl.parseClass(''' +package org.grails.datastore.gorm.dirty.checking + +import grails.gorm.dirty.checking.DirtyCheck + +@DirtyCheck +class KeyedScheduleLike { + String businessKey + List shares = [] + + boolean equals(Object o) { + o instanceof KeyedScheduleLike && ((KeyedScheduleLike) o).businessKey == businessKey + } + + int hashCode() { businessKey == null ? 0 : businessKey.hashCode() } +} +''') + entityClass = gcl.parseClass(''' +package org.grails.datastore.gorm.dirty.checking + +import grails.gorm.dirty.checking.DirtyCheck + +@DirtyCheck +class ScheduleLike { + List shares = [] + Set tags = new HashSet() + SortedSet ranked = new TreeSet() + Map attributes = [:] +} +''') + } + + def 'reassigning an equal plain list over a tracked list keeps tracking (falsy empty re-init)'() { + given: 'an entity whose collection is tracked, as it is after a datastore decode' + def entity = entityClass.newInstance() + entity.shares = DirtyCheckingSupport.wrap([], (DirtyCheckable) entity, 'shares') + entity.trackChanges() + + when: 'the common defensive re-init runs (true for an EMPTY tracked list — Groovy falsy)' + if (!entity.shares) { + entity.shares = [] + } + + and: 'an element is added in place' + entity.shares.add('new-share') + + then: 'the replacement collection is still tracked and the mutation was recorded' + entity.shares instanceof DirtyCheckableCollection + ((DirtyCheckableCollection) entity.shares).isAssigned() // replacement, not decode — persisters must not diff per element + entity.hasChanged() + entity.hasChanged('shares') + entity.shares.contains('new-share') + } + + def 'reassigning a different plain list over a tracked list keeps tracking'() { + given: + def entity = entityClass.newInstance() + entity.shares = DirtyCheckingSupport.wrap(['a'], (DirtyCheckable) entity, 'shares') + entity.trackChanges() + + when: + entity.shares = ['b'] + + then: 'the assignment itself is flagged (values differ)' + entity.hasChanged('shares') + entity.shares instanceof DirtyCheckableCollection + + when: 'changes are reset and the list is mutated in place' + entity.trackChanges() + entity.shares.add('c') + + then: + entity.hasChanged() + entity.hasChanged('shares') + } + + def 'reassigning a plain set over a tracked set keeps tracking'() { + given: + def entity = entityClass.newInstance() + entity.tags = DirtyCheckingSupport.wrap(new HashSet(), (DirtyCheckable) entity, 'tags') + entity.trackChanges() + + when: + if (!entity.tags) { + entity.tags = new HashSet() + } + entity.tags.add('tag') + + then: + entity.tags instanceof DirtyCheckableCollection + entity.hasChanged('tags') + } + + def 'reassigning a plain map over a tracked map keeps tracking'() { + given: + def entity = entityClass.newInstance() + entity.attributes = new DirtyCheckingMap([:], (DirtyCheckable) entity, 'attributes') + entity.trackChanges() + + when: + if (!entity.attributes) { + entity.attributes = [:] + } + entity.attributes.put('k', 'v') + + then: + entity.attributes instanceof DirtyCheckableCollection + entity.hasChanged('attributes') + } + + def 'reassigning a plain SortedSet over a tracked SortedSet keeps tracking and the SortedSet API'() { + given: + def entity = entityClass.newInstance() + entity.ranked = DirtyCheckingSupport.wrap(new TreeSet(), (DirtyCheckable) entity, 'ranked') + entity.trackChanges() + + when: + if (!entity.ranked) { + entity.ranked = new TreeSet() + } + entity.ranked.add('r1') + + then: 'the replacement is the SortedSet wrapper, so the property keeps its declared API' + entity.ranked instanceof DirtyCheckingSortedSet + entity.ranked instanceof SortedSet + ((DirtyCheckableCollection) entity.ranked).isAssigned() + entity.hasChanged('ranked') + } + + def 'a store-specific wrapper subclass is never replaced by a generic rewrap'() { + given: 'a value tracked by a store-specific subclass, as the Neo4j store installs' + def entity = entityClass.newInstance() + entity.shares = new StoreSpecificList([], (DirtyCheckable) entity, 'shares') + entity.trackChanges() + + when: 'the property is reassigned wholesale' + entity.shares = ['a'] + + then: 'the raw value is stored so the store persister can install its own type on save' + !(entity.shares instanceof DirtyCheckableCollection) + entity.hasChanged('shares') + } + + def "assigning another entity's tracked list re-binds it to the entity being assigned to"() { + given: 'two entities, each holding a decode-installed wrapper of its own' + def a = entityClass.newInstance() + def b = entityClass.newInstance() + a.shares = DirtyCheckingSupport.wrap(['a1'], (DirtyCheckable) a, 'shares') + b.shares = DirtyCheckingSupport.wrap(['b1'], (DirtyCheckable) b, 'shares') + a.trackChanges() + b.trackChanges() + + when: "b's collection is assigned onto a" + a.shares = b.shares + + then: 'a holds a NEW wrapper bound to a, flagged as the wholesale replacement it is' + !a.shares.is(b.shares) + a.shares.getParent().is(a) + ((DirtyCheckableCollection) a.shares).isAssigned() + + when: 'changes are reset and the collection is mutated through a' + a.trackChanges() + b.trackChanges() + a.shares.add('added') + + then: 'a is marked dirty and b is left alone — before, it was the other way round' + a.hasChanged('shares') + !b.hasChanged('shares') + + and: 'the two properties still share one underlying collection, as a plain assignment does' + b.shares.contains('added') + } + + def "assigning another entity's tracked map re-binds it too"() { + given: 'the Map wrapper shares no supertype with the collection wrappers' + def a = entityClass.newInstance() + def b = entityClass.newInstance() + a.attributes = new DirtyCheckingMap([x: '1'], (DirtyCheckable) a, 'attributes') + b.attributes = new DirtyCheckingMap([y: '2'], (DirtyCheckable) b, 'attributes') + a.trackChanges() + b.trackChanges() + + when: + a.attributes = b.attributes + + then: + !a.attributes.is(b.attributes) + a.attributes.getParent().is(a) + ((DirtyCheckableCollection) a.attributes).isAssigned() + + when: + a.trackChanges() + b.trackChanges() + a.attributes.put('k', 'v') + + then: + a.hasChanged('attributes') + !b.hasChanged('attributes') + } + + def "a store-specific wrapper borrowed from another entity is stored untouched"() { + given: 'the value being replaced is tracked, so the re-binding branch is reached' + def a = entityClass.newInstance() + def b = entityClass.newInstance() + a.shares = DirtyCheckingSupport.wrap([], (DirtyCheckable) a, 'shares') + def storeSpecific = new StoreSpecificList(['x'], (DirtyCheckable) b, 'shares') + a.trackChanges() + + when: + a.shares = storeSpecific + + then: 'never swapped for a generic wrapper — the store re-wraps it on save' + a.shares.is(storeSpecific) + } + + def "the same-owner check compares identity, not equals"() { + given: 'two distinct entities that are equal by business key' + def a1 = keyedEntityClass.newInstance() + def a2 = keyedEntityClass.newInstance() + a1.businessKey = 'same' + a2.businessKey = 'same' + assert a1 == a2 + assert !a1.is(a2) + a1.shares = DirtyCheckingSupport.wrap(['x'], (DirtyCheckable) a1, 'shares') + a1.trackChanges() + + when: "a wrapper owned by the equal-but-distinct a2 is assigned to a1" + a1.shares = DirtyCheckingSupport.wrap(['y'], (DirtyCheckable) a2, 'shares') + + then: "it is re-bound to a1 rather than mistaken for a1's own" + a1.shares.getParent().is(a1) + ((DirtyCheckableCollection) a1.shares).isAssigned() + } + + def "a wrapper nested inside another is unwrapped all the way to the raw collection"() { + given: "an encoder can leave a wrapper wrapping a wrapper (BasicCollectionTypeEncoder does)" + def a = entityClass.newInstance() + def b = entityClass.newInstance() + def raw = ['x'] + def nested = new DirtyCheckingList( + new DirtyCheckingList(raw, (DirtyCheckable) b, 'shares'), (DirtyCheckable) b, 'shares') + a.shares = DirtyCheckingSupport.wrap([], (DirtyCheckable) a, 'shares') + a.trackChanges() + b.trackChanges() + + when: + a.shares = nested + + then: "re-bound onto the raw collection, not onto b's inner wrapper" + a.shares.getTarget().is(raw) + + when: + a.shares.add('added') + + then: "so a mutation marks only a — an inner wrapper would have marked b as well" + a.hasChanged('shares') + !b.hasChanged('shares') + } + + def "a collection borrowed onto a property that was never tracked is still re-bound"() { + given: "the shape of new Entity(shares: other.shares) — the field starts out null" + def a = entityClass.newInstance() + def b = entityClass.newInstance() + b.shares = DirtyCheckingSupport.wrap(['b1'], (DirtyCheckable) b, 'shares') + a.shares = null + a.trackChanges() + b.trackChanges() + + when: + a.shares = b.shares + + then: "a wrapper's existence proves a store already tracks it, just for the wrong owner" + a.shares.getParent().is(a) + + when: + a.shares.add('added') + + then: + a.hasChanged('shares') + !b.hasChanged('shares') + } + + def "a plain collection replacing a datastore #kind is re-wrapped"() { + given: "the class a stored to-many decodes into, rather than one of the generic wrappers" + def entity = entityClass.newInstance() + entity[property] = decoded(Mock(Session)) + entity.trackChanges() + + when: "the defensive re-init replaces it and the result is mutated in place" + entity[property] = replacement() + entity[property].add('a') + + then: + entity[property] instanceof DirtyCheckableCollection + ((DirtyCheckableCollection) entity[property]).isAssigned() + entity.hasChanged(property) + + where: + kind | property | decoded | replacement + 'PersistentList' | 'shares' | { Session s -> new PersistentList([], String, s) } | { -> [] } + 'PersistentSet' | 'tags' | { Session s -> new PersistentSet([], String, s) } | { -> new HashSet() } + 'PersistentSortedSet' | 'ranked' | { Session s -> new PersistentSortedSet([], String, s) } | { -> new TreeSet() } + } + + def "a store-specific PersistentCollection subclass is left to its store"() { + given: "the shape of the Neo4j and Rx to-many types, which subclass the datastore ones" + def entity = entityClass.newInstance() + entity.shares = new StoreSpecificPersistentList([], String, Mock(Session)) + entity.trackChanges() + + when: + entity.shares = ['a'] + + then: "the exact-class rule leaves it raw, so the store wraps it in its own type on save" + !(entity.shares instanceof DirtyCheckableCollection) + + and: "the assignment is still flagged, which the store persister relies on to re-wrap it" + entity.hasChanged('shares') + } + + def 'a property that was never tracked is left untouched by the setter'() { + given: 'a transient instance whose initializer collections were never wrapped' + def entity = entityClass.newInstance() + entity.trackChanges() + + when: + entity.shares = ['a'] + + then: 'the raw value is stored as-is (Hibernate and transient behaviour unchanged)' + !(entity.shares instanceof DirtyCheckableCollection) + entity.hasChanged('shares') + } + + def 'assigning an already-tracked wrapper over a tracked value passes it through unchanged'() { + given: + def entity = entityClass.newInstance() + entity.shares = DirtyCheckingSupport.wrap([], (DirtyCheckable) entity, 'shares') + entity.trackChanges() + + when: 'a decode-style wrapper is assigned, as a datastore decoder does' + def decoded = DirtyCheckingSupport.wrap(['x'], (DirtyCheckable) entity, 'shares') + entity.shares = decoded + + then: 'it is stored as-is and NOT re-flagged as an assignment' + entity.shares.is(decoded) + !((DirtyCheckableCollection) entity.shares).isAssigned() + } + + def 'rewrap wraps a plain non-List non-Set collection replacing a tracked value'() { + given: + def entity = entityClass.newInstance() + def tracked = DirtyCheckingSupport.wrap([], (DirtyCheckable) entity, 'shares') + + when: + def result = DirtyCheckingSupport.rewrap((DirtyCheckable) entity, 'shares', tracked, new ArrayDeque(['q'])) + + then: + result instanceof DirtyCheckingCollection + ((DirtyCheckableCollection) result).isAssigned() + } + + def 'rewrap leaves a non-collection value untouched'() { + given: + def entity = entityClass.newInstance() + def tracked = DirtyCheckingSupport.wrap([], (DirtyCheckable) entity, 'shares') + + expect: 'defensive tail — a value that is neither Collection nor Map is returned as-is' + DirtyCheckingSupport.rewrap((DirtyCheckable) entity, 'shares', tracked, 'not-a-collection') == 'not-a-collection' + } + + def 'assigning null over a tracked collection stores null'() { + given: + def entity = entityClass.newInstance() + entity.shares = DirtyCheckingSupport.wrap(['a'], (DirtyCheckable) entity, 'shares') + entity.trackChanges() + + when: + entity.shares = null + + then: + entity.shares == null + entity.hasChanged('shares') + } +} + +class StoreSpecificList extends DirtyCheckingList { + StoreSpecificList(List target, DirtyCheckable parent, String property) { + super(target, parent, property) + } +} + +class StoreSpecificPersistentList extends PersistentList { + StoreSpecificPersistentList(Collection keys, Class childType, Session session) { + super(keys, childType, session) + } +} diff --git a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckableCollection.java b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckableCollection.java index cd275c16177..95862ac2aae 100644 --- a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckableCollection.java +++ b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckableCollection.java @@ -53,4 +53,16 @@ public interface DirtyCheckableCollection extends Serializable { * @return True if the collection has changed size */ boolean hasChangedSize(); + + /** + * Whether this wrapper was created for a collection ASSIGNED over a previously tracked + * value (through a dirty-checking setter), as opposed to wrapping the value a datastore + * decoded. An assigned collection is a wholesale replacement: persisters must not diff it + * element-by-element against the stored state, whose layout it need not match. + * + * @return True if this wrapper was created by a property assignment + */ + default boolean isAssigned() { + return false; + } } diff --git a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingCollection.groovy b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingCollection.groovy index b47b00fb63f..ccd276d3817 100644 --- a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingCollection.groovy +++ b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingCollection.groovy @@ -19,6 +19,8 @@ package org.grails.datastore.mapping.dirty.checking +import java.util.function.Predicate + import groovy.transform.CompileStatic /** @@ -34,12 +36,23 @@ class DirtyCheckingCollection implements Collection, DirtyCheckableCollection { final DirtyCheckable parent final String property final int originalSize + final boolean assigned DirtyCheckingCollection(Collection target, DirtyCheckable parent, String property) { + this(target, parent, property, false) + } + + DirtyCheckingCollection(Collection target, DirtyCheckable parent, String property, boolean assigned) { this.target = target this.originalSize = target.size() this.parent = parent this.property = property + this.assigned = assigned + } + + @Override + boolean isAssigned() { + return assigned } @Override @@ -61,6 +74,24 @@ class DirtyCheckingCollection implements Collection, DirtyCheckableCollection { parent.hasChanged(property) || hasChangedElements() } + // Content equality, like AbstractPersistentCollection: the wrapper is transparent, so it + // equals whatever its target equals. Without that, the tracking views handed out by + // DirtyCheckingMap (keySet/entrySet/values) broke Groovy's Map == Map, whose extension + // method compares self.keySet().equals(other.keySet()). Identity comes first so the + // contract stays reflexive for a target that has no content equality of its own — + // values() wraps an AbstractCollection, which inherits Object.equals, and without the + // short-circuit the wrapper would not equal itself, breaking List.contains and HashSet + // membership. + @Override + boolean equals(Object other) { + other.is(this) || target.equals(other) + } + + @Override + int hashCode() { + target.hashCode() + } + protected boolean hasChangedElements() { target.any { (it instanceof DirtyCheckable) && ((DirtyCheckable) it).hasChanged() } } @@ -83,6 +114,18 @@ class DirtyCheckingCollection implements Collection, DirtyCheckableCollection { target.removeAll(c) } + @Override + boolean retainAll(Collection c) { + parent.markDirty(property) + target.retainAll(c) + } + + @Override + boolean removeIf(Predicate filter) { + parent.markDirty(property) + target.removeIf(filter) + } + @Override void clear() { parent.markDirty(property) @@ -95,5 +138,28 @@ class DirtyCheckingCollection implements Collection, DirtyCheckableCollection { target.remove(o) } + /** + * Returns an iterator whose {@code remove()} marks the parent dirty. Without this, + * every iterator-based removal — including Groovy's {@code removeAll(Closure)} / + * {@code retainAll(Closure)} DGM methods — silently bypassed change tracking. + */ + @Override + Iterator iterator() { + final Iterator underlying = target.iterator() + return new Iterator() { + @Override + boolean hasNext() { underlying.hasNext() } + + @Override + Object next() { underlying.next() } + + @Override + void remove() { + parent.markDirty(property) + underlying.remove() + } + } + } + } diff --git a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingList.groovy b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingList.groovy index d85afd3cce1..213557523d4 100644 --- a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingList.groovy +++ b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingList.groovy @@ -19,6 +19,8 @@ package org.grails.datastore.mapping.dirty.checking +import java.util.function.UnaryOperator + import groovy.transform.CompileStatic /** @@ -30,10 +32,16 @@ import groovy.transform.CompileStatic @CompileStatic class DirtyCheckingList extends DirtyCheckingCollection implements List { - @Delegate List target + // reversed() is excluded: on Groovy 6 the generated version replaced the tracking + // override below, so the reverse-ordered view came back untracked. + @Delegate(excludes = 'reversed') List target DirtyCheckingList(List target, DirtyCheckable parent, String property) { - super(target, parent, property) + this(target, parent, property, false) + } + + DirtyCheckingList(List target, DirtyCheckable parent, String property, boolean assigned) { + super(target, parent, property, assigned) this.target = target } @@ -60,4 +68,108 @@ class DirtyCheckingList extends DirtyCheckingCollection implements List { parent.markDirty(property) target.remove((int) index) } + + @Override + void sort(Comparator c) { + parent.markDirty(property) + target.sort(c) + } + + @Override + void replaceAll(UnaryOperator operator) { + parent.markDirty(property) + target.replaceAll(operator) + } + + // SequencedCollection (Java 21) mutators. Without these the @Delegate-generated versions + // call straight through to the target, bypassing change tracking entirely. + @Override + void addFirst(Object element) { + parent.markDirty(property) + target.addFirst(element) + } + + @Override + void addLast(Object element) { + parent.markDirty(property) + target.addLast(element) + } + + @Override + Object removeFirst() { + parent.markDirty(property) + target.removeFirst() + } + + @Override + Object removeLast() { + parent.markDirty(property) + target.removeLast() + } + + @Override + List reversed() { + // A live view that writes through to this list — return it tracking the same parent + // Flagged as assigned: a view stored on a property IS a wholesale replacement, and a + // persister must re-encode it rather than diff it element-by-element against what the + // property held before. The flag is inert for a view that is only traversed. + return new DirtyCheckingList(target.reversed(), parent, property, true) + } + + @Override + List subList(int fromIndex, int toIndex) { + // A live view that writes through to this list — return it tracking the same parent + return new DirtyCheckingList(target.subList(fromIndex, toIndex), parent, property, true) + } + + @Override + ListIterator listIterator() { + trackingListIterator(target.listIterator()) + } + + @Override + ListIterator listIterator(int index) { + trackingListIterator(target.listIterator(index)) + } + + /** A list iterator whose mutating operations mark the parent dirty (see {@link DirtyCheckingCollection#iterator()}). */ + private ListIterator trackingListIterator(final ListIterator underlying) { + return new ListIterator() { + @Override + boolean hasNext() { underlying.hasNext() } + + @Override + Object next() { underlying.next() } + + @Override + boolean hasPrevious() { underlying.hasPrevious() } + + @Override + Object previous() { underlying.previous() } + + @Override + int nextIndex() { underlying.nextIndex() } + + @Override + int previousIndex() { underlying.previousIndex() } + + @Override + void remove() { + parent.markDirty(property) + underlying.remove() + } + + @Override + void set(Object o) { + parent.markDirty(property) + underlying.set(o) + } + + @Override + void add(Object o) { + parent.markDirty(property) + underlying.add(o) + } + } + } } diff --git a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingMap.groovy b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingMap.groovy index ef2a21cf0a3..18e7c667df7 100644 --- a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingMap.groovy +++ b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingMap.groovy @@ -19,6 +19,9 @@ package org.grails.datastore.mapping.dirty.checking +import java.util.function.BiFunction +import java.util.function.Function + import groovy.transform.CompileStatic /** @@ -34,12 +37,23 @@ class DirtyCheckingMap implements Map, DirtyCheckableCollection { final DirtyCheckable parent final String property final int originalSize + final boolean assigned DirtyCheckingMap(Map target, DirtyCheckable parent, String property) { + this(target, parent, property, false) + } + + DirtyCheckingMap(Map target, DirtyCheckable parent, String property, boolean assigned) { this.target = target this.parent = parent this.property = property this.originalSize = target.size() + this.assigned = assigned + } + + @Override + boolean isAssigned() { + return assigned } @Override @@ -61,6 +75,20 @@ class DirtyCheckingMap implements Map, DirtyCheckableCollection { parent.hasChanged(property) } + // Content equality, like AbstractPersistentCollection — see DirtyCheckingCollection. + // Identity first: a target with no content equality of its own (values() wraps an + // AbstractCollection, which inherits Object.equals) must still equal itself, or + // List.contains and HashSet membership break on the wrapper. + @Override + boolean equals(Object other) { + other.is(this) || target.equals(other) + } + + @Override + int hashCode() { + target.hashCode() + } + @Override Object put(Object key, Object value) { parent.markDirty(property) @@ -84,4 +112,80 @@ class DirtyCheckingMap implements Map, DirtyCheckableCollection { parent.markDirty(property) target.clear() } + + @Override + Object putIfAbsent(Object key, Object value) { + parent.markDirty(property) + target.putIfAbsent(key, value) + } + + @Override + Object merge(Object key, Object value, BiFunction remappingFunction) { + parent.markDirty(property) + target.merge(key, value, remappingFunction) + } + + @Override + Object compute(Object key, BiFunction remappingFunction) { + parent.markDirty(property) + target.compute(key, remappingFunction) + } + + @Override + Object computeIfAbsent(Object key, Function mappingFunction) { + parent.markDirty(property) + target.computeIfAbsent(key, mappingFunction) + } + + @Override + Object computeIfPresent(Object key, BiFunction remappingFunction) { + parent.markDirty(property) + target.computeIfPresent(key, remappingFunction) + } + + @Override + Object replace(Object key, Object value) { + parent.markDirty(property) + target.replace(key, value) + } + + @Override + boolean replace(Object key, Object oldValue, Object newValue) { + parent.markDirty(property) + target.replace(key, oldValue, newValue) + } + + @Override + void replaceAll(BiFunction function) { + parent.markDirty(property) + target.replaceAll(function) + } + + @Override + boolean remove(Object key, Object value) { + parent.markDirty(property) + target.remove(key, value) + } + + /** + * The three views write through to the map, so they are returned wrapped in the tracking + * collection types — Groovy's {@code Map.removeAll(Closure)}/{@code retainAll(Closure)} + * iterate {@code entrySet()}, and {@code keySet().remove(...)} / {@code values().removeIf(...)} + * are ordinary removal paths. Mutating an entry's value directly during iteration + * ({@code Map.Entry.setValue}) remains untracked. + */ + @Override + Set entrySet() { + return new DirtyCheckingSet(target.entrySet(), parent, property, true) + } + + @Override + Set keySet() { + return new DirtyCheckingSet(target.keySet(), parent, property, true) + } + + @Override + Collection values() { + return new DirtyCheckingCollection(target.values(), parent, property, true) + } } diff --git a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSet.groovy b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSet.groovy index bfca6fa5a88..1d8b16bd250 100644 --- a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSet.groovy +++ b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSet.groovy @@ -33,7 +33,11 @@ class DirtyCheckingSet extends DirtyCheckingCollection implements Set { @Delegate Set target DirtyCheckingSet(Set target, DirtyCheckable parent, String property) { - super(target, parent, property) + this(target, parent, property, false) + } + + DirtyCheckingSet(Set target, DirtyCheckable parent, String property, boolean assigned) { + super(target, parent, property, assigned) this.target = target } } diff --git a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSortedSet.groovy b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSortedSet.groovy index d6022842a47..a1f8a482ec1 100644 --- a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSortedSet.groovy +++ b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSortedSet.groovy @@ -30,10 +30,59 @@ import groovy.transform.CompileStatic @CompileStatic class DirtyCheckingSortedSet extends DirtyCheckingCollection implements SortedSet { - @Delegate SortedSet target + // See the note on DirtyCheckingList: reversed() must not be generated over the override. + @Delegate(excludes = 'reversed') SortedSet target DirtyCheckingSortedSet(SortedSet target, DirtyCheckable parent, String property) { - super(target, parent, property) + this(target, parent, property, false) + } + + DirtyCheckingSortedSet(SortedSet target, DirtyCheckable parent, String property, boolean assigned) { + super(target, parent, property, assigned) this.target = target } + + // SequencedCollection (Java 21) removals; addFirst/addLast are not overridden because + // SortedSet rejects them outright, so they never reach a mutation to track. + @Override + Object removeFirst() { + parent.markDirty(property) + target.removeFirst() + } + + @Override + Object removeLast() { + parent.markDirty(property) + target.removeLast() + } + + @Override + SortedSet reversed() { + return trackingView(target.reversed()) + } + + // headSet/subSet/tailSet are @Delegate-generated live views onto the backing set, so a + // removal through one writes through while bypassing change tracking entirely. + @Override + SortedSet headSet(Object toElement) { + return trackingView(target.headSet(toElement)) + } + + @Override + SortedSet tailSet(Object fromElement) { + return trackingView(target.tailSet(fromElement)) + } + + @Override + SortedSet subSet(Object fromElement, Object toElement) { + return trackingView(target.subSet(fromElement, toElement)) + } + + /** A live view that writes through to this set, tracking the same parent. */ + private SortedSet trackingView(SortedSet view) { + // Flagged as assigned: a view stored on a property IS a wholesale replacement, and a + // persister must re-encode it rather than diff it element-by-element against what the + // property held before. The flag is inert for a view that is only traversed. + return new DirtyCheckingSortedSet(view, parent, property, true) + } } diff --git a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSupport.groovy b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSupport.groovy index 7fb541e908f..911133ad2cc 100644 --- a/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSupport.groovy +++ b/grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingSupport.groovy @@ -21,6 +21,9 @@ package org.grails.datastore.mapping.dirty.checking import groovy.transform.CompileStatic import org.grails.datastore.mapping.collection.PersistentCollection +import org.grails.datastore.mapping.collection.PersistentList +import org.grails.datastore.mapping.collection.PersistentSet +import org.grails.datastore.mapping.collection.PersistentSortedSet import org.grails.datastore.mapping.model.MappingContext import org.grails.datastore.mapping.model.PersistentEntity import org.grails.datastore.mapping.model.types.Association @@ -125,9 +128,192 @@ class DirtyCheckingSupport { if (coll instanceof List) { return new DirtyCheckingList(coll, parent, property) } + if (coll instanceof SortedSet) { + return new DirtyCheckingSortedSet(coll, parent, property) + } if (coll instanceof Set) { return new DirtyCheckingSet(coll, parent, property) } return new DirtyCheckingCollection(coll, parent, property) } + + /** + * Wraps a map in dirty checking capability. Returns an already-wrapped map unchanged — + * without that guard an encoder that write-backs on every save nests one wrapper per save. + * + * @param map The map + * @param parent The parent + * @param property The property + * @return The wrapped map + */ + static Map wrap(Map map, DirtyCheckable parent, String property) { + if (map instanceof DirtyCheckingMap) { + return map + } + return new DirtyCheckingMap(map, parent, property) + } + + /** + * Re-establishes change tracking when a tracked collection or map value is replaced + * through a generated dirty-checking setter. + * + *

    Interception-based stores (MongoDB et al.) install the DirtyChecking* wrappers when an + * entity is decoded and rely on them exclusively — there is no flush-time snapshot + * comparison. The generated setter used to store whatever raw value it was handed, so + * reassigning a collection property replaced the tracked wrapper with a plain, untracked + * collection and every later in-place mutation became invisible to change tracking. The + * common defensive re-init {@code if (!entity.items) entity.items = []} triggered this on + * every load (an empty tracked collection is falsy in Groovy), and because the new empty + * collection equals the old one the assignment itself was never flagged either. + * + *

    Tracking is only re-established, never introduced: the value being replaced must + * already be one of the exact classes a datastore decode installs — a generic wrapper, or + * the {@code PersistentList}/{@code PersistentSet}/{@code PersistentSortedSet} a to-many + * association decodes into. Anything else is returned untouched: a transient instance, a + * store like Hibernate that performs its own snapshot-based dirty checking, or a + * store-specific subclass such as the Neo4j collection types. Store-specific wrappers in + * particular must NOT be replaced by a generic one: + * the store's persister recognises its own types (and treats anything already + * dirty-checkable as such), so a generic replacement would permanently disable that store's + * relationship handling for the property. Leaving the raw value lets the persister wrap it + * in its own type on save, exactly as it did before this method existed. + * + * @param parent The dirty-checkable owner + * @param property The property being assigned + * @param oldValue The value being replaced + * @param newValue The value being assigned + * @return The value to store: {@code newValue}, wrapped if it replaces a tracked value + */ + static Object rewrap(DirtyCheckable parent, String property, Object oldValue, Object newValue) { + if (newValue == null) { + return newValue + } + if (newValue instanceof DirtyCheckableCollection) { + // Already tracking this same owner: the wrapper a datastore decode or an encode + // write-back installed. Pass it through untouched — re-wrapping would restart the + // originalSize baseline and flag an assignment that never happened. + if (tracksSameOwner(newValue, parent, property)) { + return newValue + } + // Anything that is not one of this package's own generic wrappers is left entirely + // alone, exactly as the oldValue gate does — see isGenericWrapper. + Object borrowed = genericWrapperTarget(newValue) + if (borrowed == null) { + return newValue + } + // Another entity's wrapper. Storing it verbatim binds this property to that entity: + // mutating it would mark THAT entity dirty and leave this one clean, and it would + // keep assigned=false, so a persister would diff a wholesale replacement element by + // element. Re-bind by wrapping its raw target below — never the wrapper itself, + // which would mark both entities dirty. The two properties go on sharing the + // underlying collection, as any plain assignment would. + newValue = borrowed + } + else if (!wasTracked(oldValue)) { + // A plain collection replacing a value this package was not already tracking. + // Tracking is introduced by the datastore at decode, never here — a transient + // instance, or a store like Hibernate that does its own snapshot-based dirty + // checking, must keep storing exactly what it was given. (A wrapper arriving from + // another entity is handled above and re-bound whatever it replaces: its existence + // means a store already tracks that collection, just for the wrong owner.) + return newValue + } + // Constructed with assigned=true: a replacement is a wholesale rewrite, so persisters + // must not diff it element-by-element against stored state (see DirtyCheckableCollection + // .isAssigned()). + if (newValue instanceof List) { + return new DirtyCheckingList((List) newValue, parent, property, true) + } + if (newValue instanceof SortedSet) { + return new DirtyCheckingSortedSet((SortedSet) newValue, parent, property, true) + } + if (newValue instanceof Set) { + return new DirtyCheckingSet((Set) newValue, parent, property, true) + } + if (newValue instanceof Collection) { + return new DirtyCheckingCollection((Collection) newValue, parent, property, true) + } + if (newValue instanceof Map) { + return new DirtyCheckingMap((Map) newValue, parent, property, true) + } + return newValue + } + + /** + * True when the wrapper already tracks changes for exactly this owner and property, rather + * than being a value borrowed from another entity. + * + *

    The owner is compared by identity on purpose: a domain class may implement + * {@code equals()} on a business key, and on an association comparing it can initialise a + * proxy and issue another query. + */ + private static boolean tracksSameOwner(Object value, DirtyCheckable parent, String property) { + if (value instanceof DirtyCheckingCollection) { + DirtyCheckingCollection wrapper = (DirtyCheckingCollection) value + return wrapper.parent.is(parent) && property == wrapper.property + } + if (value instanceof DirtyCheckingMap) { + DirtyCheckingMap wrapper = (DirtyCheckingMap) value + return wrapper.parent.is(parent) && property == wrapper.property + } + return false + } + + /** + * The raw collection or map behind one of this package's own generic wrappers, or + * {@code null} for anything else — a store-specific subclass such as the Neo4j collection + * types, or a {@code PersistentCollection}. Mirrors {@link #isGenericWrapper}. + */ + private static Object genericWrapperTarget(Object value) { + if (!isGenericWrapper(value)) { + return null + } + Object target = value + // Loop rather than unwrap once: an encoder can leave a wrapper nested inside another + // (BasicCollectionTypeEncoder builds a DirtyCheckingMap unconditionally), and re-binding + // onto an inner wrapper would leave every mutation marking the other entity too. + while (isGenericWrapper(target)) { + target = target instanceof DirtyCheckingMap + ? ((DirtyCheckingMap) target).target + : ((DirtyCheckingCollection) target).target + } + return target + } + + /** + * True when the property was already held in a collection the datastore tracks changes + * through: one of this package's own generic wrappers, or the {@code PersistentCollection} + * a to-many association decodes into. Replacing either with a plain collection would drop + * that tracking, so the replacement is wrapped. + * + *

    Deliberately does not read the value — a {@code PersistentCollection} may be an + * uninitialised lazy association, and touching it would issue a query. + */ + private static boolean wasTracked(Object value) { + return isGenericWrapper(value) || isPersistentCollection(value) + } + + /** + * True only for the datastore's own to-many collection classes, by exact class. A + * store-specific subclass (the Neo4j {@code Neo4jPersistentList} and friends) fails this + * check and stays with its store, exactly as {@link #isGenericWrapper} keeps the Neo4j + * wrapper subclasses out. + */ + private static boolean isPersistentCollection(Object value) { + Class valueClass = value?.getClass() + return valueClass == PersistentList || valueClass == PersistentSet || + valueClass == PersistentSortedSet + } + + /** + * True only for the plugin's own generic wrapper classes — the exact types installed by a + * datastore decode or a previous {@link #rewrap}. Subclasses (Neo4jList, Neo4jSet, …) and + * {@code PersistentCollection} implementations deliberately fail this check. + */ + private static boolean isGenericWrapper(Object value) { + Class valueClass = value?.getClass() + return valueClass == DirtyCheckingCollection || valueClass == DirtyCheckingList || + valueClass == DirtyCheckingSet || valueClass == DirtyCheckingSortedSet || + valueClass == DirtyCheckingMap + } } diff --git a/grails-datastore-core/src/test/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingCollectionSpec.groovy b/grails-datastore-core/src/test/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingCollectionSpec.groovy new file mode 100644 index 00000000000..134e29ab52f --- /dev/null +++ b/grails-datastore-core/src/test/groovy/org/grails/datastore/mapping/dirty/checking/DirtyCheckingCollectionSpec.groovy @@ -0,0 +1,501 @@ +/* + * 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.mapping.dirty.checking + +import spock.lang.Specification + +/** + * The DirtyChecking* wrappers are the only change-detection interception-based stores have + * (there is no Hibernate-style flush-time snapshot comparison), so every mutation path a + * wrapped collection exposes must mark the parent dirty. Historically only the directly + * overridden methods (add/remove/addAll/removeAll(Collection)/clear) did; everything that + * removes through an iterator — including Groovy's removeAll(Closure)/retainAll(Closure) + * DGM methods and Java's removeIf default method — silently bypassed tracking, so a + * subsequent save() persisted nothing. + */ +class DirtyCheckingCollectionSpec extends Specification { + + def 'iterator().remove() marks the parent dirty'() { + given: + def owner = new CollectionOwner() + def list = new DirtyCheckingList(['a', 'b', 'c'], owner, 'items') + owner.trackChanges() + + when: + Iterator i = list.iterator() + i.next() + i.remove() + + then: + list.size() == 2 + owner.hasChanged() + owner.hasChanged('items') + } + + def "Groovy removeAll(Closure) marks the parent dirty"() { + given: + def owner = new CollectionOwner() + def list = new DirtyCheckingList(['a', 'b', 'c'], owner, 'items') + owner.trackChanges() + + when: 'elements are removed via the DGM closure variant (iterator-based)' + list.removeAll { it == 'b' } + + then: + list.size() == 2 + owner.hasChanged() + owner.hasChanged('items') + } + + def "Groovy retainAll(Closure) marks the parent dirty"() { + given: + def owner = new CollectionOwner() + def list = new DirtyCheckingList(['a', 'b', 'c'], owner, 'items') + owner.trackChanges() + + when: + list.retainAll { it == 'a' } + + then: + list.size() == 1 + owner.hasChanged() + owner.hasChanged('items') + } + + def 'retainAll(Collection) marks the parent dirty'() { + given: + def owner = new CollectionOwner() + def list = new DirtyCheckingList(['a', 'b', 'c'], owner, 'items') + owner.trackChanges() + + when: + list.retainAll(['a']) + + then: + list.size() == 1 + owner.hasChanged() + owner.hasChanged('items') + } + + def 'removeIf(Predicate) marks the parent dirty'() { + given: + def owner = new CollectionOwner() + def list = new DirtyCheckingList(['a', 'b', 'c'], owner, 'items') + owner.trackChanges() + + when: + list.removeIf { String s -> s == 'c' } + + then: + list.size() == 2 + owner.hasChanged() + owner.hasChanged('items') + } + + def 'listIterator mutations mark the parent dirty'() { + given: + def owner = new CollectionOwner() + def list = new DirtyCheckingList(['a', 'b', 'c'], owner, 'items') + owner.trackChanges() + + when: + ListIterator li = list.listIterator() + li.next() + li.set('z') + + then: + list[0] == 'z' + owner.hasChanged() + owner.hasChanged('items') + + when: + owner.trackChanges() + li = list.listIterator(1) + li.next() + li.remove() + + then: + list.size() == 2 + owner.hasChanged('items') + + when: + owner.trackChanges() + li = list.listIterator() + li.add('new') + + then: + list.size() == 3 + owner.hasChanged('items') + } + + def 'sort and replaceAll mark the parent dirty'() { + given: + def owner = new CollectionOwner() + def list = new DirtyCheckingList(['c', 'a', 'b'], owner, 'items') + owner.trackChanges() + + when: + list.sort(Comparator. naturalOrder()) + + then: + list[0] == 'a' + owner.hasChanged('items') + + when: + owner.trackChanges() + list.replaceAll { String s -> s.toUpperCase() } + + then: + list[0] == 'A' + owner.hasChanged('items') + } + + def 'iterator removal on a wrapped Set marks the parent dirty'() { + given: + def owner = new CollectionOwner() + def set = new DirtyCheckingSet(['a', 'b'] as Set, owner, 'tags') + owner.trackChanges() + + when: + set.removeAll { it == 'a' } + + then: + set.size() == 1 + owner.hasChanged() + owner.hasChanged('tags') + } + + def 'directly overridden mutators still mark the parent dirty'() { + given: + def owner = new CollectionOwner() + def list = new DirtyCheckingList(['a'], owner, 'items') + owner.trackChanges() + + when: + list.add('b') + + then: + owner.hasChanged('items') + + when: + owner.trackChanges() + list.remove('a') + + then: + owner.hasChanged('items') + + when: + owner.trackChanges() + list.clear() + + then: + list.isEmpty() + owner.hasChanged('items') + } + + def 'listIterator navigation methods delegate without marking the parent dirty'() { + given: + def owner = new CollectionOwner() + def list = new DirtyCheckingList(['a', 'b'], owner, 'items') + owner.trackChanges() + + when: + ListIterator li = list.listIterator() + li.next() + + then: + li.hasPrevious() + li.nextIndex() == 1 + li.previousIndex() == 0 + li.previous() == 'a' + !owner.hasChanged() + } + + def 'a wrapped SortedSet tracks iterator removal and carries the assigned flag'() { + given: + def owner = new CollectionOwner() + def sorted = new DirtyCheckingSortedSet(new TreeSet(['a', 'b', 'c']), owner, 'sorted') + owner.trackChanges() + + expect: + !sorted.isAssigned() + new DirtyCheckingSortedSet(new TreeSet(), owner, 'sorted', true).isAssigned() + + when: + sorted.removeAll { it == 'b' } + + then: + sorted.size() == 2 + owner.hasChanged('sorted') + } + + def 'a wrapped Map carries the assigned flag'() { + given: + def owner = new CollectionOwner() + + expect: + !new DirtyCheckingMap([:], owner, 'attrs').isAssigned() + new DirtyCheckingMap([:], owner, 'attrs', true).isAssigned() + } + + def 'isAssigned defaults to false for implementations that do not override it'() { + expect: 'the interface default keeps pre-existing implementations (PersistentCollection) unflagged' + !new MinimalDirtyCheckableCollection().isAssigned() + } + + def 'subList mutations mark the parent dirty'() { + given: + def owner = new CollectionOwner() + def list = new DirtyCheckingList(['a', 'b', 'c'], owner, 'items') + owner.trackChanges() + + when: 'an element is removed through the live subList view' + list.subList(0, 2).remove('a') + + then: + list.size() == 2 + owner.hasChanged('items') + } + + def 'wrap returns the SortedSet wrapper for a SortedSet'() { + given: + def owner = new CollectionOwner() + + expect: + DirtyCheckingSupport.wrap(new TreeSet(['a']), owner, 'sorted') instanceof DirtyCheckingSortedSet + } + + def 'Map default methods mark the parent dirty'() { + given: + def owner = new CollectionOwner() + def map = new DirtyCheckingMap([a: 1], owner, 'attrs') + + expect: + marksDirty(owner) { map.putIfAbsent('b', 2) } + marksDirty(owner) { map.merge('a', 10) { x, y -> x + y } } + marksDirty(owner) { map.computeIfAbsent('c') { 3 } } + marksDirty(owner) { map.computeIfPresent('a') { k, v -> v + 1 } } + marksDirty(owner) { map.compute('a') { k, v -> 99 } } + marksDirty(owner) { map.replace('a', 100) } + marksDirty(owner) { map.replace('a', 100, 101) } + marksDirty(owner) { map.replaceAll { k, v -> v } } + marksDirty(owner) { map.remove('absent', 0) } + } + + def 'Map view removals mark the parent dirty'() { + given: + def owner = new CollectionOwner() + def map = new DirtyCheckingMap([a: 1, b: 2, c: 3, d: 4], owner, 'attrs') + owner.trackChanges() + + when: 'an entry is removed through the entrySet iterator (the path Groovy DGM removal methods use)' + Iterator entries = map.entrySet().iterator() + entries.next() + entries.remove() + + then: + map.size() == 3 + owner.hasChanged('attrs') + + when: + owner.trackChanges() + map.keySet().remove('b') + + then: + map.size() == 2 + owner.hasChanged('attrs') + + when: + owner.trackChanges() + map.values().removeIf { it == 3 } + + then: + map.size() == 1 + owner.hasChanged('attrs') + + when: + owner.trackChanges() + map.entrySet().removeAll { Map.Entry e -> e.key == 'd' } + + then: + map.isEmpty() + owner.hasChanged('attrs') + } + + private static boolean marksDirty(CollectionOwner owner, Closure mutation) { + owner.trackChanges() + mutation() + owner.hasChanged('attrs') + } + + def 'wrappers are content-equal to plain collections in both directions'() { + given: + def owner = new CollectionOwner() + def list = new DirtyCheckingList(['a', 'b'], owner, 'items') + def set = new DirtyCheckingSet(['a'] as Set, owner, 'tags') + def map = new DirtyCheckingMap([a: 1, b: 2], owner, 'attrs') + + expect: 'Groovy == and Java equals both hold, both ways (AbstractPersistentCollection precedent)' + list == ['a', 'b'] + ['a', 'b'] == list + list.hashCode() == ['a', 'b'].hashCode() + set == (['a'] as Set) + map == [a: 1, b: 2] + [a: 1, b: 2] == map + map.hashCode() == [a: 1, b: 2].hashCode() + + and: 'the map views are content-equal to the raw views (what Map equality iterates)' + map.keySet() == (['a', 'b'] as Set) + map.entrySet() == [a: 1, b: 2].entrySet() + map.values() as List == [1, 2] + } + + def 'SequencedCollection mutators mark the parent dirty'() { + given: 'without overrides these are @Delegate-generated straight through to the target' + def owner = new CollectionOwner() + def list = new DirtyCheckingList(['a', 'b', 'c'], owner, 'items') + + expect: + marksDirtyOn(owner, 'items') { list.addFirst('z') } + marksDirtyOn(owner, 'items') { list.addLast('y') } + marksDirtyOn(owner, 'items') { list.removeFirst() } + marksDirtyOn(owner, 'items') { list.removeLast() } + } + + def 'mutating the reversed view marks the parent dirty'() { + given: + def owner = new CollectionOwner() + def list = new DirtyCheckingList(['a', 'b'], owner, 'items') + owner.trackChanges() + + when: 'the live reverse-ordered view is mutated' + list.reversed().remove('a') + + then: + list.size() == 1 + owner.hasChanged('items') + } + + def 'SortedSet sequenced removals and its reversed view mark the parent dirty'() { + given: + def owner = new CollectionOwner() + def sorted = new DirtyCheckingSortedSet(new TreeSet(['a', 'b', 'c']), owner, 'sorted') + + expect: + marksDirtyOn(owner, 'sorted') { sorted.removeFirst() } + marksDirtyOn(owner, 'sorted') { sorted.removeLast() } + marksDirtyOn(owner, 'sorted') { sorted.reversed().remove('b') } + } + + def 'SortedSet range views mark the parent dirty'() { + given: 'headSet/subSet/tailSet are live views that write through to the backing set' + def owner = new CollectionOwner() + def sorted = new DirtyCheckingSortedSet(new TreeSet(['a', 'b', 'c', 'd']), owner, 'sorted') + + expect: + marksDirtyOn(owner, 'sorted') { sorted.headSet('b').remove('a') } + marksDirtyOn(owner, 'sorted') { sorted.tailSet('d').remove('d') } + marksDirtyOn(owner, 'sorted') { sorted.subSet('b', 'c').clear() } + + and: + sorted.size() == 1 + } + + def 'a view stored back onto a property counts as a wholesale replacement'() { + given: 'the flag decides whether a persister re-encodes or diffs element by element' + def owner = new CollectionOwner() + def list = new DirtyCheckingList(['a', 'b', 'c'], owner, 'items') + def sorted = new DirtyCheckingSortedSet(new TreeSet(['a', 'b']), owner, 'sorted') + def map = new DirtyCheckingMap([a: 1], owner, 'attrs') + + expect: 'every live view reports itself as assigned; the backing wrapper does not' + !((DirtyCheckableCollection) list).isAssigned() + ((DirtyCheckableCollection) list.reversed()).isAssigned() + ((DirtyCheckableCollection) list.subList(0, 2)).isAssigned() + ((DirtyCheckableCollection) sorted.reversed()).isAssigned() + ((DirtyCheckableCollection) sorted.headSet('b')).isAssigned() + ((DirtyCheckableCollection) map.keySet()).isAssigned() + ((DirtyCheckableCollection) map.values()).isAssigned() + } + + def 'a wrapper equals itself even when its target has only identity equality'() { + given: "values() wraps an AbstractCollection, which inherits Object.equals" + def owner = new CollectionOwner() + def values = new DirtyCheckingMap([a: 1], owner, 'attrs').values() + def overDeque = new DirtyCheckingCollection(new ArrayDeque(['x']), owner, 'items') + + expect: 'reflexive, so List.contains and HashSet membership work on the wrapper' + values == values + values.equals(values) + [values].contains(values) + overDeque.equals(overDeque) + [overDeque].contains(overDeque) + + and: 'content equality against the target is unaffected' + new DirtyCheckingList(['a'], owner, 'items') == ['a'] + } + + def 'wrap returns an already-wrapped map unchanged'() { + given: "an encoder that write-backs on every save would otherwise nest one wrapper per save" + def owner = new CollectionOwner() + def wrapped = DirtyCheckingSupport.wrap([a: 1], owner, 'attrs') + + expect: + wrapped instanceof DirtyCheckingMap + DirtyCheckingSupport.wrap(wrapped, owner, 'attrs').is(wrapped) + } + + private static boolean marksDirtyOn(CollectionOwner owner, String property, Closure mutation) { + owner.trackChanges() + mutation() + owner.hasChanged(property) + } + + def 'iteration without mutation does not mark the parent dirty'() { + given: + def owner = new CollectionOwner() + def list = new DirtyCheckingList(['a', 'b'], owner, 'items') + owner.trackChanges() + + when: 'the collection is only read' + def joined = list.collect { it }.join(',') + for (def ignored : list) { + // consume + } + + then: + joined == 'a,b' + !owner.hasChanged() + } +} + +class CollectionOwner implements DirtyCheckable { + List items + Set tags + SortedSet sorted + Map attrs +} + +class MinimalDirtyCheckableCollection implements DirtyCheckableCollection { + boolean hasChanged() { false } + int getOriginalSize() { 0 } + boolean hasGrown() { false } + boolean hasShrunk() { false } + boolean hasChangedSize() { false } +} diff --git a/grails-doc/src/en/guide/theWebLayer/gson/gsonHal.adoc b/grails-doc/src/en/guide/theWebLayer/gson/gsonHal.adoc index 020330e08c7..501b4238b40 100644 --- a/grails-doc/src/en/guide/theWebLayer/gson/gsonHal.adoc +++ b/grails-doc/src/en/guide/theWebLayer/gson/gsonHal.adoc @@ -254,14 +254,14 @@ For example: [source,groovy] model { Iterable bookList - Integer bookCount + Long bookCount Integer max // optional, defaults to 10 Integer offset // optional, defaults to 0 String sort // optional String order // optional } json { - hal.paginate(Book, bookCount, max, offset, sort, order) + hal.paginate(Book, bookCount, offset, max, sort, order) ... } @@ -305,7 +305,7 @@ If you have other links that you want to include along with the pagination links model { Author author Iterable bookList - Integer bookCount + Long bookCount } json { // the model, type to paginate, and the total count diff --git a/grails-doc/src/en/guide/upgrading/upgrading80x.adoc b/grails-doc/src/en/guide/upgrading/upgrading80x.adoc index 9eb3a478aab..d7ee5ad625c 100644 --- a/grails-doc/src/en/guide/upgrading/upgrading80x.adoc +++ b/grails-doc/src/en/guide/upgrading/upgrading80x.adoc @@ -3335,7 +3335,7 @@ imported whether or not they are present — a star import of an absent package not an error in Groovy, so the probe changed nothing it could observe. An application that relied on the import being *omitted* when the package was absent sees no difference in compiled output. -==== 54. Non-Public Bean Classes Are Marshalled, and Reported Once +==== 58. Non-Public Bean Classes Are Marshalled, and Reported Once Groovy 4 stamped `ACC_PUBLIC` on anonymous inner classes; Groovy 5, which Grails 8 builds on, does not. Reflection cannot invoke a method on a class that is not `public` from another package, even when the method @@ -3413,7 +3413,7 @@ logging: org.apache.grails.common.reflect.ReflectionUtils: ERROR ---- -==== 58. Interceptor URI Matching Uses the Dispatched Path +==== 59. Interceptor URI Matching Uses the Dispatched Path Interceptors that match on `uri`, with `match(uri: ...)` or `excludes(uri: ...)`, now match the same path that URL mappings route on: percent escapes are decoded, matrix parameters (`;name=value`) are removed and the context path is @@ -3461,7 +3461,7 @@ RFC 9110 section 4.2.3 define paths as case-sensitive, and during a `RequestDisp matched. Interceptors and the Spring Security matchers follow dispatch on both points because a matcher that disagrees with dispatch lets a request reach a controller without its interceptors or restrictions. -==== 59. Sort and Order Query Arguments Are Validated +==== 60. Sort and Order Query Arguments Are Validated Grails 8 checks the `sort` and `order` arguments of `list()`, dynamic finders, where queries, criteria queries and `listOrderBy*` before the query is built, on both Hibernate 5 and Hibernate 7. A client-supplied sort value used to be interpolated into the query, and the entry points did not agree on how a bad value was handled. They now fail the same way everywhere, with an `IllegalArgumentException` whose message deliberately does not echo the value. @@ -3488,7 +3488,7 @@ def index(Integer max) { } ---- -==== 60. XML Request Bodies That Declare a DOCTYPE Are Refused +==== 61. XML Request Bodies That Declare a DOCTYPE Are Refused XML that Grails parses on an application's behalf is now read with external entity expansion, external DTD retrieval and DTD grammar loading disabled. Earlier releases set these parser features under identifiers the parser did not recognise, @@ -3505,3 +3505,195 @@ is never retrieved. The plugin descriptor `grails-plugin.xml` is not among them: declaration, so it is read strictly at compile time and at runtime alike. See <> for the full description of the XML parsing defaults. + +==== 62. Generated Groovy Compiler Configuration Scripts + +On Gradle 9.7+, configure `groovyOptions.configurationScriptFile` before task execution. Assigning +`groovyOptions.configurationScript` from `compileGroovy.doFirst { ... }` fails because Gradle has +already finalized the compiler options. Grails prepares the combined compiler script before compilation. + +If another task generates your compiler script, prefer passing that task's output provider to +`configurationScriptFile`. The provider carries both the file location and its producing task, so +Grails can prepare the combined script after your script is generated. + +Declare the generated script as an output and connect it through its provider: + +[source,groovy] +.build.gradle +---- +import org.gradle.api.DefaultTask +import org.gradle.api.file.RegularFileProperty +import org.gradle.api.tasks.OutputFile +import org.gradle.api.tasks.TaskAction +import org.gradle.api.tasks.compile.GroovyCompile +import org.gradle.work.DisableCachingByDefault + +@DisableCachingByDefault(because = 'Writing a short compiler script is inexpensive') +abstract class GenerateCompilerScript extends DefaultTask { + @OutputFile + abstract RegularFileProperty getOutputFile() + + @TaskAction + void generate() { + def script = outputFile.get().asFile + script.parentFile.mkdirs() + script.text = "withConfig(configuration) { imports { star 'java.nio.file' } }" + } +} + +def generateUserConfigScript = tasks.register('generateUserConfigScript', GenerateCompilerScript) { + outputFile.set(layout.buildDirectory.file('user-config.groovy')) +} + +tasks.named('compileGroovy', GroovyCompile) { + groovyOptions.configurationScriptFile.set(generateUserConfigScript.flatMap { it.outputFile }) +} +---- + +No separate `dependsOn` is needed for this producer. If the script's content depends on task properties +or other files, declare those as task inputs so changes regenerate the script. + +Existing builds can also keep the plain file assignment with an explicit compile dependency: + +[source,groovy] +.build.gradle (alternative wiring for the producer above) +---- +tasks.named('compileGroovy', GroovyCompile) { + dependsOn generateUserConfigScript + groovyOptions.configurationScript = file('build/user-config.groovy') +} +---- + +Grails matches the configured file against the declared outputs of the compile task's direct +dependencies and orders script preparation after the matching producer. This preserves the Grails 7 +pattern when the producer declares the exact script file as an output, whether through an `@OutputFile` +property as `GenerateCompilerScript` does above or through `outputs.file(...)` on an ad-hoc task. A +dependency that declares only a containing output directory, writes the file without declaring it as an +output, or is reachable only through another task's `dependsOn` cannot be identified this way. Such a +build fails from a clean checkout before compilation starts, with a validation error on the +`generateCompileGroovyGrailsCompilerConfig` task that Grails registers: + +[source,text] +---- +A problem was found with the configuration of task ':generateCompileGroovyGrailsCompilerConfig' (type 'GrailsCompilerConfigScriptTask'). +Input file does not exist + ... property 'configurationScript' specifies file '.../build/user-config.groovy' which doesn't exist +---- + +If you see this error, expose the script as an output file and use the provider wiring instead. + +An existing, checked-in compiler script can still use a plain file assignment without a producer. +Tasks that only generate source or classpath entries do not need additional ordering against +compiler-script preparation: Grails does not read the compile or runtime classpath to prepare its script. + +==== 63. `count()` Returns `Long` + +`count()` and the `count` property return `Long` instead of `Integer`: + +[source,groovy] +---- +// Grails 7 +Integer total = Book.count() + +// Grails 8 +Long total = Book.count() +---- + +How wide the underlying value is depends on the datastore. Hibernate counts with SQL `COUNT(*)` and Neo4j +with Cypher `count(*)`, both 64-bit. MongoDB aggregates with `{$sum: 1}`, which returns Int32 and promotes +to Int64 once the total exceeds it, so the width depends on how many documents the collection holds. Only +the in-memory datastore is fixed at 32 bits, and it returns a list size. `GormStaticApi.count()` narrowed the datastore +result with `intValue()`, which truncates silently, so +a table with more than `Integer.MAX_VALUE` rows reported a wrong and possibly negative count with no +error. `Long` is the only type that holds every value a datastore can produce here, so it is what the API +now declares. + +The change affects `GormStaticOperations.count()` and `getCount()`, the `count()` and `count` members +generated on every domain class, and `RestfulController.countResources()`. Dynamic Groovy is unaffected: +`Book.count() == 5`, `int n = Book.count()` and arithmetic on the result all continue to work, because +Groovy converts between numeric types on assignment and compares them by value. + +Code that is statically compiled, or that overrides one of these methods, needs updating: + +[source,groovy] +---- +@CompileStatic +class ReportService { + // Grails 7 + // Integer bookCount() { Book.count() } + + // Grails 8 + Long bookCount() { Book.count() } +} + +class BookController extends RestfulController { + // Grails 7 + // protected Integer countResources() { ... } + + // Grails 8 + protected Long countResources() { ... } +} +---- + +Java callers assigning the result to an `Integer` or `int` need an explicit conversion, since `Long` does +not unbox to `int`: + +[source,java] +---- +long total = Book.count(); // auto-unboxes, no change needed +int small = Book.count().intValue(); // only when the count is known to be small +---- + +A GSP that declares the count in a typed model directive should declare it as `Number`, which accepts +whatever a controller supplies: + +[source,html] +---- +@{ model="List bookList; Number bookCount" } +---- + +JSON views declare their model the same way, and bind it the same way, so a `model` block naming the count +with a concrete numeric type needs the same treatment: + +[source,groovy] +.grails-app/views/book/index.gson +---- +model { + Iterable bookList + // Grails 7 + // Integer bookCount + Long bookCount // or Number, to accept either width +} +---- + +`HalViewHelper.paginate()` and `links()` now declare `total` as `Long`, and `paginate()` declares `offset` +as `Long` with it. Callers are unaffected, because Groovy widens an `Integer` argument to `Long` in both +dynamic and statically compiled code, but anyone implementing `HalViewHelper` has to update their +signatures. + +The GraphQL count field changes type with it. `graphql-java` maps `Long` to +`ExtendedScalars.GraphQLLong`, so a schema that exposed + +[source,graphql] +---- +type Query { + personCount: Int +} +---- + +now exposes + +[source,graphql] +---- +type Query { + personCount: Long +} +---- + +Queries that select the field are unchanged and the JSON value is unchanged, but generated clients and +schema snapshots need regenerating. The count fetcher previously narrowed the datastore result to fit +`Int`, so counts above `Integer.MAX_VALUE` were truncated there too. + +Paginated list responses already type `totalCount` as `Long` through the same type manager, in +`DefaultGraphQLPaginationResponseHandler`, so this aligns the count field with a scalar the schema +already used rather than introducing a new one. diff --git a/grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/core/GrailsGradlePlugin.groovy b/grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/core/GrailsGradlePlugin.groovy index 19fa4c167cb..50ee24fdab1 100644 --- a/grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/core/GrailsGradlePlugin.groovy +++ b/grails-gradle/plugins/src/main/groovy/org/grails/gradle/plugin/core/GrailsGradlePlugin.groovy @@ -380,9 +380,24 @@ class GrailsGradlePlugin implements Plugin { // on the property directly tries to convert the file itself into a task, and building a // collection from a property with no value fails while dependencies are resolved. t.dependsOn({ - RegularFileProperty configured = project.tasks.named(compileTaskName, GroovyCompile) - .get().groovyOptions.configurationScriptFile - configured.present ? [project.files(configured)] : [] + GroovyCompile compile = project.tasks.named(compileTaskName, GroovyCompile).get() + RegularFileProperty configured = compile.groovyOptions.configurationScriptFile + if (!configured.present) { + return [] + } + FileCollection scriptFiles = project.files(configured) + List dependencies = [scriptFiles] + if (scriptFiles.buildDependencies.getDependencies(t).isEmpty()) { + // A plain file has no producer metadata. Preserve the older file + dependsOn + // wiring by matching only direct compile dependencies that declare this exact + // output. Inheriting unrelated dependencies could introduce a cycle, and + // reading the compile or runtime classpath would restore the original bug. + File scriptFile = configured.asFile.get() + dependencies.addAll(compile.taskDependencies.getDependencies(compile).findAll { Task dependency -> + dependency != t && dependency.outputs.files.contains(scriptFile) + }) + } + dependencies } as Callable) } } diff --git a/grails-gradle/plugins/src/test/groovy/org/grails/gradle/plugin/core/GrailsGroovyCompilerConfigSpec.groovy b/grails-gradle/plugins/src/test/groovy/org/grails/gradle/plugin/core/GrailsGroovyCompilerConfigSpec.groovy index 14a11383f79..7cb870f86c8 100644 --- a/grails-gradle/plugins/src/test/groovy/org/grails/gradle/plugin/core/GrailsGroovyCompilerConfigSpec.groovy +++ b/grails-gradle/plugins/src/test/groovy/org/grails/gradle/plugin/core/GrailsGroovyCompilerConfigSpec.groovy @@ -157,6 +157,165 @@ class GrailsGroovyCompilerConfigSpec extends GradleSpecification { producer < generator } + def "a generated user script is consumed during compilation and follows changes with #wiring wiring"() { + given: 'a compiler script wired by provider or by plain file plus a compile dependency' + setupTestResourceProject('compiler-config-generated-user-script-execution') + + when: 'a clean build compiles and runs source that needs both sets of imports' + def first = executeTask('clean', ['verifyCompilation'] + scriptArgs) + + then: + first.task(':generateUserConfigScript').outcome == TaskOutcome.SUCCESS + first.task(':generateCompileGroovyGrailsCompilerConfig').outcome == TaskOutcome.SUCCESS + first.task(':compileGroovy').outcome == TaskOutcome.SUCCESS + first.output.contains('CONFIGURED_TYPE=java.nio.file.Path') + first.output.contains('GRAILS_IMPORT=1') + + when: 'nothing changes' + def unchanged = executeTask('verifyCompilation', scriptArgs) + + then: + unchanged.task(':generateUserConfigScript').outcome == TaskOutcome.UP_TO_DATE + unchanged.task(':generateCompileGroovyGrailsCompilerConfig').outcome == TaskOutcome.UP_TO_DATE + unchanged.task(':compileGroovy').outcome == TaskOutcome.UP_TO_DATE + + when: 'the producer changes the imported type' + def changed = executeTask('verifyCompilation', ['-PimportedType=java.net.URI'] + scriptArgs) + + then: 'the new script reaches the compiler rather than stale content being reused' + changed.task(':generateUserConfigScript').outcome == TaskOutcome.SUCCESS + changed.task(':generateCompileGroovyGrailsCompilerConfig').outcome == TaskOutcome.SUCCESS + changed.task(':compileGroovy').outcome == TaskOutcome.SUCCESS + changed.output.contains('CONFIGURED_TYPE=java.net.URI') + changed.output.contains('GRAILS_IMPORT=1') + + where: + wiring | scriptArgs + 'provider' | [] + 'ad-hoc provider' | ['-PadHocProducer'] + 'plain' | ['-PplainFile'] + 'plain, ad-hoc producer' | ['-PplainFile', '-PadHocProducer'] + } + + def "a plain script file keeps its #producer producer ordered before script preparation"() { + given: 'the legacy file assignment with a separate compileGroovy dependency' + setupTestResourceProject('compiler-config-generated-user-script-execution') + + when: 'compilation is requested normally from a clean project' + def natural = executeTask('verifyCompilation', ['-PplainFile'] + producerArgs) + + then: 'the producer runs before the combined script is read' + natural.task(':generateUserConfigScript').outcome == TaskOutcome.SUCCESS + natural.task(':compileGroovy').outcome == TaskOutcome.SUCCESS + natural.output.contains('CONFIGURED_TYPE=java.nio.file.Path') + natural.output.contains('GRAILS_IMPORT=1') + + when: 'script preparation is explicitly requested first after cleaning the outputs' + def forced = executeTask('clean', ['generateCompileGroovyGrailsCompilerConfig', + 'verifyCompilation', '-PplainFile'] + producerArgs) + + then: 'the producer dependency still takes precedence over the requested task order' + forced.task(':generateUserConfigScript').outcome == TaskOutcome.SUCCESS + forced.task(':compileGroovy').outcome == TaskOutcome.SUCCESS + forced.output.contains('CONFIGURED_TYPE=java.nio.file.Path') + forced.output.contains('GRAILS_IMPORT=1') + + where: 'the producer declares the script as an @OutputFile property or with outputs.file' + producer | producerArgs + 'typed' | [] + 'ad-hoc' | ['-PadHocProducer'] + } + + def "script preparation does not inherit unrelated compile dependencies with #wiring wiring"() { + given: 'an additional compile dependency that itself depends on script preparation' + setupTestResourceProject('compiler-config-generated-user-script-execution') + + when: + def result = executeTask('verifyCompilation', ['-PadditionalDependency'] + scriptArgs) + + then: 'there is no cycle and the additional task still runs before compilation' + result.task(':prepareCompilation').outcome == TaskOutcome.SUCCESS + result.task(':compileGroovy').outcome == TaskOutcome.SUCCESS + result.tasks*.path.indexOf(':prepareCompilation') < result.tasks*.path.indexOf(':compileGroovy') + result.output.contains('ADDITIONAL_COMPILE_DEPENDENCY=ran') + result.output.contains('CONFIGURED_TYPE=java.nio.file.Path') + result.output.contains('GRAILS_IMPORT=1') + + where: + wiring | scriptArgs + 'provider' | [] + 'plain' | ['-PplainFile'] + } + + def "a generated user script keeps its producer with the configuration cache and #wiring wiring"() { + given: + setupTestResourceProject('compiler-config-generated-user-script-execution') + + when: 'a cached graph is reused after cleaning all generated outputs' + def stored = executeTask('clean', ['verifyCompilation', '--configuration-cache'] + scriptArgs) + def reused = executeTask('clean', ['verifyCompilation', '--configuration-cache'] + scriptArgs) + + then: 'both builds compile using a newly produced script' + stored.output.contains('Configuration cache entry stored') + reused.output.contains('Configuration cache entry reused') + stored.task(':compileGroovy').outcome == TaskOutcome.SUCCESS + reused.task(':generateUserConfigScript').outcome == TaskOutcome.SUCCESS + reused.task(':compileGroovy').outcome == TaskOutcome.SUCCESS + reused.output.contains('CONFIGURED_TYPE=java.nio.file.Path') + reused.output.contains('GRAILS_IMPORT=1') + + where: + wiring | scriptArgs + 'provider' | [] + 'plain' | ['-PplainFile'] + } + + def "a build-cached user script is consumed during compilation with #wiring wiring"() { + given: 'an isolated build cache and a compiler that always executes after cleaning' + setupTestResourceProject('compiler-config-generated-user-script-execution') + def args = ['verifyCompilation', '--build-cache', '-PuncachedCompilation'] + scriptArgs + + when: 'the producer populates the build cache' + def first = executeTask('clean', args) + + then: + first.task(':generateUserConfigScript').outcome == TaskOutcome.SUCCESS + first.task(':compileGroovy').outcome == TaskOutcome.SUCCESS + first.output.contains('CONFIGURED_TYPE=java.nio.file.Path') + + when: 'outputs are deleted and the producer is restored from the build cache' + def restored = executeTask('clean', args) + + then: 'compilation actually executes using the restored script' + restored.task(':generateUserConfigScript').outcome == TaskOutcome.FROM_CACHE + restored.task(':generateCompileGroovyGrailsCompilerConfig').outcome == TaskOutcome.SUCCESS + restored.task(':compileGroovy').outcome == TaskOutcome.SUCCESS + restored.output.contains('CONFIGURED_TYPE=java.nio.file.Path') + restored.output.contains('GRAILS_IMPORT=1') + + when: 'the producer input changes' + def changed = executeTask('clean', args + ['-PimportedType=java.net.URI']) + + then: 'the old cache entry is not reused' + changed.task(':generateUserConfigScript').outcome == TaskOutcome.SUCCESS + changed.task(':compileGroovy').outcome == TaskOutcome.SUCCESS + changed.output.contains('CONFIGURED_TYPE=java.net.URI') + + when: 'the original input is requested again after cleaning' + def original = executeTask('clean', args) + + then: 'the original cached script replaces the changed script' + original.task(':generateUserConfigScript').outcome == TaskOutcome.FROM_CACHE + original.task(':compileGroovy').outcome == TaskOutcome.SUCCESS + original.output.contains('CONFIGURED_TYPE=java.nio.file.Path') + original.output.contains('GRAILS_IMPORT=1') + + where: + wiring | scriptArgs + 'provider' | [] + 'plain' | ['-PplainFile'] + } + def "one generator per source set, tracked so only a real change regenerates"() { given: 'a project whose star imports come from a build property' setupTestResourceProject('compiler-config-incremental') diff --git a/grails-gradle/plugins/src/test/resources/test-projects/compiler-config-generated-user-script-execution/build.gradle b/grails-gradle/plugins/src/test/resources/test-projects/compiler-config-generated-user-script-execution/build.gradle new file mode 100644 index 00000000000..ccc4323a3de --- /dev/null +++ b/grails-gradle/plugins/src/test/resources/test-projects/compiler-config-generated-user-script-execution/build.gradle @@ -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. + */ +import org.gradle.api.DefaultTask +import org.gradle.api.file.RegularFileProperty +import org.gradle.api.provider.Property +import org.gradle.api.tasks.CacheableTask +import org.gradle.api.tasks.Input +import org.gradle.api.tasks.JavaExec +import org.gradle.api.tasks.OutputFile +import org.gradle.api.tasks.TaskAction +import org.gradle.api.tasks.compile.GroovyCompile + +plugins { + id 'org.apache.grails.gradle.grails-app' +} + +grails { + bom = null + cliAutoProvision = false + starImports = ['java.util.concurrent.atomic'] +} + +dependencies { + implementation localGroovy() +} + +@CacheableTask +abstract class GenUserScript extends DefaultTask { + @Input abstract Property getImportedType() + @OutputFile abstract RegularFileProperty getOutputFile() + + @TaskAction + void generate() { + outputFile.get().asFile.parentFile.mkdirs() + outputFile.get().asFile.text = """ + withConfig(configuration) { + imports { + alias 'ImportedType', '${importedType.get()}' + } + } + """ + } +} + +def importedTypeName = providers.gradleProperty('importedType').orElse('java.nio.file.Path') +def userScript = layout.buildDirectory.file('user-config.groovy') + +def genUserScript +if (providers.gradleProperty('adHocProducer').isPresent()) { + // The shape most Grails 7 builds used: an untyped task that declares the script with outputs.file. + genUserScript = tasks.register('generateUserConfigScript') { + inputs.property('importedType', importedTypeName) + outputs.file(userScript) + doLast { + def script = userScript.get().asFile + script.parentFile.mkdirs() + script.text = """ + withConfig(configuration) { + imports { + alias 'ImportedType', '${importedTypeName.get()}' + } + } + """ + } + } +} else { + genUserScript = tasks.register('generateUserConfigScript', GenUserScript) { + importedType.set(importedTypeName) + outputFile.set(userScript) + } +} + +if (providers.gradleProperty('additionalDependency').isPresent()) { + def preparation = tasks.register('prepareCompilation') { + // Copying every compile dependency onto script preparation would create a cycle here. + dependsOn 'generateCompileGroovyGrailsCompilerConfig' + def unrelatedOutput = layout.buildDirectory.file('unrelated-config.groovy') + outputs.file(unrelatedOutput) + doLast { + unrelatedOutput.get().asFile.text = '// Not the configured compiler script' + println 'ADDITIONAL_COMPILE_DEPENDENCY=ran' + } + } + tasks.named('compileGroovy', GroovyCompile) { + dependsOn preparation + } +} + +tasks.named('compileGroovy', GroovyCompile) { + if (providers.gradleProperty('uncachedCompilation').isPresent()) { + outputs.cacheIf { false } + } + if (providers.gradleProperty('plainFile').isPresent()) { + // The plugin matches this file against the compile dependency's declared output. + dependsOn genUserScript + groovyOptions.configurationScript = file('build/user-config.groovy') + } else if (providers.gradleProperty('adHocProducer').isPresent()) { + groovyOptions.configurationScriptFile.fileProvider(genUserScript.map { it.outputs.files.singleFile }) + } else { + groovyOptions.configurationScriptFile.set(genUserScript.flatMap { it.outputFile }) + } +} + +tasks.register('verifyCompilation', JavaExec) { + classpath = sourceSets.main.runtimeClasspath + mainClass.set('example.ConfiguredType') +} diff --git a/grails-gradle/plugins/src/test/resources/test-projects/compiler-config-generated-user-script-execution/gradle.properties b/grails-gradle/plugins/src/test/resources/test-projects/compiler-config-generated-user-script-execution/gradle.properties new file mode 100644 index 00000000000..35c332fb874 --- /dev/null +++ b/grails-gradle/plugins/src/test/resources/test-projects/compiler-config-generated-user-script-execution/gradle.properties @@ -0,0 +1 @@ +grailsVersion=__PROJECT_VERSION__ diff --git a/grails-gradle/plugins/src/test/resources/test-projects/compiler-config-generated-user-script-execution/settings.gradle b/grails-gradle/plugins/src/test/resources/test-projects/compiler-config-generated-user-script-execution/settings.gradle new file mode 100644 index 00000000000..a9780889b2b --- /dev/null +++ b/grails-gradle/plugins/src/test/resources/test-projects/compiler-config-generated-user-script-execution/settings.gradle @@ -0,0 +1,25 @@ +/* + * 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. + */ +rootProject.name = 'compiler-config-generated-user-script-execution' + +buildCache { + local { + directory = new File(settingsDir, '.build-cache') + } +} diff --git a/grails-gradle/plugins/src/test/resources/test-projects/compiler-config-generated-user-script-execution/src/main/groovy/example/ConfiguredType.groovy b/grails-gradle/plugins/src/test/resources/test-projects/compiler-config-generated-user-script-execution/src/main/groovy/example/ConfiguredType.groovy new file mode 100644 index 00000000000..3b69e531945 --- /dev/null +++ b/grails-gradle/plugins/src/test/resources/test-projects/compiler-config-generated-user-script-execution/src/main/groovy/example/ConfiguredType.groovy @@ -0,0 +1,29 @@ +/* + * 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 example + +class ConfiguredType { + + static void main(String[] args) { + ImportedType value = null + AtomicInteger count = new AtomicInteger() + println "CONFIGURED_TYPE=${ImportedType.name}" + println "GRAILS_IMPORT=${count.incrementAndGet()}" + } +} diff --git a/grails-gradle/plugins/src/test/resources/test-projects/compiler-config-generated-user-script/build.gradle b/grails-gradle/plugins/src/test/resources/test-projects/compiler-config-generated-user-script/build.gradle index 769237d7b34..25c0a501039 100644 --- a/grails-gradle/plugins/src/test/resources/test-projects/compiler-config-generated-user-script/build.gradle +++ b/grails-gradle/plugins/src/test/resources/test-projects/compiler-config-generated-user-script/build.gradle @@ -1,3 +1,21 @@ +/* + * 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. + */ import org.gradle.api.DefaultTask import org.gradle.api.file.RegularFileProperty import org.gradle.api.tasks.OutputFile diff --git a/grails-redis/build.gradle b/grails-redis/build.gradle index 66635ea2ab9..4e83b6aec5e 100644 --- a/grails-redis/build.gradle +++ b/grails-redis/build.gradle @@ -83,5 +83,4 @@ apply { from rootProject.layout.projectDirectory.file('gradle/docs-config.gradle') from rootProject.layout.projectDirectory.file('gradle/grails-extension-gradle-config.gradle') from rootProject.layout.projectDirectory.file('gradle/redis-test-config.gradle') - from rootProject.layout.projectDirectory.file('gradle/test-config.gradle') } diff --git a/grails-rest-transforms/src/main/groovy/grails/rest/RestfulController.groovy b/grails-rest-transforms/src/main/groovy/grails/rest/RestfulController.groovy index fd8f1e4e969..a900a31b7cf 100644 --- a/grails-rest-transforms/src/main/groovy/grails/rest/RestfulController.groovy +++ b/grails-rest-transforms/src/main/groovy/grails/rest/RestfulController.groovy @@ -281,7 +281,7 @@ class RestfulController { * * @return List of resources or empty if it doesn't exist */ - protected Integer countResources() { + protected Long countResources() { resource.count() } diff --git a/grails-scaffolding/src/main/groovy/grails/plugin/scaffolding/RestfulServiceController.groovy b/grails-scaffolding/src/main/groovy/grails/plugin/scaffolding/RestfulServiceController.groovy index dbc77d34fb4..42fcd8d2532 100644 --- a/grails-scaffolding/src/main/groovy/grails/plugin/scaffolding/RestfulServiceController.groovy +++ b/grails-scaffolding/src/main/groovy/grails/plugin/scaffolding/RestfulServiceController.groovy @@ -81,8 +81,8 @@ class RestfulServiceController> extends RestfulControlle } @Override - protected Integer countResources() { - Math.toIntExact(getService().count(params)) + protected Long countResources() { + getService().count(params) } @Override diff --git a/grails-scaffolding/src/main/templates/scaffolding/index.gsp b/grails-scaffolding/src/main/templates/scaffolding/index.gsp index 2d4eb907ca2..a4360ed650c 100644 --- a/grails-scaffolding/src/main/templates/scaffolding/index.gsp +++ b/grails-scaffolding/src/main/templates/scaffolding/index.gsp @@ -1,4 +1,4 @@ -@{ model="List<${fullName}> ${propertyName}List; Integer ${propertyName}Count" } +@{ model="List<${fullName}> ${propertyName}List; Number ${propertyName}Count" } diff --git a/grails-test-examples/hibernate7/views-functional-tests/grails-app/views/product/index.gson b/grails-test-examples/hibernate7/views-functional-tests/grails-app/views/product/index.gson index 6314c8e1e6a..f21a1f19b57 100644 --- a/grails-test-examples/hibernate7/views-functional-tests/grails-app/views/product/index.gson +++ b/grails-test-examples/hibernate7/views-functional-tests/grails-app/views/product/index.gson @@ -21,7 +21,7 @@ import functional.tests.Product model { Iterable productList - Integer productCount + Long productCount Integer max Integer offset String sort diff --git a/grails-test-examples/views-functional-tests/grails-app/views/product/index.gson b/grails-test-examples/views-functional-tests/grails-app/views/product/index.gson index 6314c8e1e6a..f21a1f19b57 100644 --- a/grails-test-examples/views-functional-tests/grails-app/views/product/index.gson +++ b/grails-test-examples/views-functional-tests/grails-app/views/product/index.gson @@ -21,7 +21,7 @@ import functional.tests.Product model { Iterable productList - Integer productCount + Long productCount Integer max Integer offset String sort diff --git a/grails-views-gson/src/main/groovy/grails/plugin/json/view/api/HalViewHelper.groovy b/grails-views-gson/src/main/groovy/grails/plugin/json/view/api/HalViewHelper.groovy index a611fbd3cd7..54e029a1739 100644 --- a/grails-views-gson/src/main/groovy/grails/plugin/json/view/api/HalViewHelper.groovy +++ b/grails-views-gson/src/main/groovy/grails/plugin/json/view/api/HalViewHelper.groovy @@ -105,7 +105,7 @@ interface HalViewHelper { * * @param object The model to create links for */ - void links(Map model, Object paginationObject, Number total) + void links(Map model, Object paginationObject, Long total) /** * Pagination support which outputs hal links to the resulting pages @@ -113,7 +113,7 @@ interface HalViewHelper { * @param object The object to create links for * @param total The total number of objects to be paginated */ - void paginate(Object object, Integer total) + void paginate(Object object, Long total) /** * Pagination support which outputs hal links to the resulting pages @@ -122,7 +122,7 @@ interface HalViewHelper { * @param total The total number of objects to be paginated * @param offset The numerical offset where the page starts (defaults to 0) */ - void paginate(Object object, Integer total, Integer offset) + void paginate(Object object, Long total, Long offset) /** * Pagination support which outputs hal links to the resulting pages @@ -132,7 +132,7 @@ interface HalViewHelper { * @param offset The numerical offset where the page starts (defaults to 0) * @param max The maximum number of objects to be shown (defaults to 10) */ - void paginate(Object object, Integer total, Integer offset, Integer max) + void paginate(Object object, Long total, Long offset, Integer max) /** * Pagination support which outputs hal links to the resulting pages @@ -143,7 +143,7 @@ interface HalViewHelper { * @param max The maximum number of objects to be shown (defaults to 10) * @param sort The field to sort on (defaults to null) */ - void paginate(Object object, Integer total, Integer offset, Integer max, String sort) + void paginate(Object object, Long total, Long offset, Integer max, String sort) /** * Pagination support which outputs hal links to the resulting pages @@ -155,7 +155,7 @@ interface HalViewHelper { * @param sort The field to sort on (defaults to null) * @param order The order in which the results are to be sorted eg: DESC or ASC */ - void paginate(Object object, Integer total, Integer offset, Integer max, String sort, String order) + void paginate(Object object, Long total, Long offset, Integer max, String sort, String order) /** * Render embedded links for the associations of the given object (if any) diff --git a/grails-views-gson/src/main/groovy/grails/plugin/json/view/api/internal/DefaultHalViewHelper.groovy b/grails-views-gson/src/main/groovy/grails/plugin/json/view/api/internal/DefaultHalViewHelper.groovy index 5eddc813f3c..5bdd8465f16 100644 --- a/grails-views-gson/src/main/groovy/grails/plugin/json/view/api/internal/DefaultHalViewHelper.groovy +++ b/grails-views-gson/src/main/groovy/grails/plugin/json/view/api/internal/DefaultHalViewHelper.groovy @@ -190,7 +190,7 @@ class DefaultHalViewHelper extends DefaultJsonViewHelper implements HalViewHelpe } //TODO: Once GROOVY-9662 is fixed, remove explicit delegate call and typecast to StreamingJsonDelegate - void links(Map model, Object paginationObject, Number total, String contentType = this.contentType) { + void links(Map model, Object paginationObject, Long total, String contentType = this.contentType) { def jsonView = view jsonDelegate.call(LINKS_ATTRIBUTE) { def linkGenerator = jsonView.linkGenerator @@ -226,7 +226,7 @@ class DefaultHalViewHelper extends DefaultJsonViewHelper implements HalViewHelpe } if (paginationObject != null) { - List links = getPaginationLinks(paginationObject, total.intValue(), jsonView.params) as List + List links = getPaginationLinks(paginationObject, total, jsonView.params) as List for (link in links) { ((StreamingJsonBuilder.StreamingJsonDelegate) delegate).call(link.rel) { ((StreamingJsonBuilder.StreamingJsonDelegate) delegate).call(HREF_ATTRIBUTE, link.href) @@ -244,7 +244,7 @@ class DefaultHalViewHelper extends DefaultJsonViewHelper implements HalViewHelpe } void links(Map model, String contentType = this.contentType) { - links(model, null, 0) + links(model, null, 0L) } /** @@ -258,12 +258,12 @@ class DefaultHalViewHelper extends DefaultJsonViewHelper implements HalViewHelpe * @param order The order in which the results are to be sorted eg: DESC or ASC */ //TODO: Once GROOVY-9662 is fixed, remove explicit delegate call and typecast to StreamingJsonDelegate - void paginate(Object object, Integer total, Integer offset = null, Integer max = null, String sort = null, String order = null) { + void paginate(Object object, Long total, Long offset = null, Integer max = null, String sort = null, String order = null) { Map linkParams = buildPaginateParams(max, offset, sort, order) GrailsView jsonView = view Parameters httpParams = jsonView.params - offset = offset ?: httpParams.int(PAGINATION_OFFSET, 0) + offset = offset ?: httpParams.long(PAGINATION_OFFSET, 0L) max = max ?: httpParams.int(PAGINATION_MAX, 10) sort = sort ?: httpParams.get(PAGINATION_SORT) order = order ?: httpParams.get(PAGINATION_ORDER) diff --git a/grails-views-gson/src/main/groovy/grails/plugin/json/view/api/internal/DefaultJsonApiViewHelper.groovy b/grails-views-gson/src/main/groovy/grails/plugin/json/view/api/internal/DefaultJsonApiViewHelper.groovy index 7df53ff987d..7c64b3a66bf 100644 --- a/grails-views-gson/src/main/groovy/grails/plugin/json/view/api/internal/DefaultJsonApiViewHelper.groovy +++ b/grails-views-gson/src/main/groovy/grails/plugin/json/view/api/internal/DefaultJsonApiViewHelper.groovy @@ -475,7 +475,7 @@ class DefaultJsonApiViewHelper extends DefaultJsonViewHelper implements JsonApiV if (!paginationArgs.containsKey(PAGINATION_TOTAL) || !paginationArgs.containsKey(PAGINATION_RESROUCE)) { throw new IllegalArgumentException('JSON API pagination arguments must contain resource and total') } - Integer total = (Integer) paginationArgs.get(PAGINATION_TOTAL) + Long total = ((Number) paginationArgs.get(PAGINATION_TOTAL)).longValue() Object resource = paginationArgs.get(PAGINATION_RESROUCE) Parameters params = defaultPaginateParams(paginationArgs) List links = getPaginationLinks(resource, total, params) diff --git a/grails-views-gson/src/main/groovy/grails/plugin/json/view/api/internal/DefaultJsonViewHelper.groovy b/grails-views-gson/src/main/groovy/grails/plugin/json/view/api/internal/DefaultJsonViewHelper.groovy index 5407b8080e7..c496472d75d 100644 --- a/grails-views-gson/src/main/groovy/grails/plugin/json/view/api/internal/DefaultJsonViewHelper.groovy +++ b/grails-views-gson/src/main/groovy/grails/plugin/json/view/api/internal/DefaultJsonViewHelper.groovy @@ -160,15 +160,15 @@ class DefaultJsonViewHelper extends DefaultGrailsViewHelper { ViewUtils.getBooleanFromMap(ASSOCIATIONS, arguments, true) } - protected List getPaginationLinks(Object object, Integer total, Parameters params) { - int offset = params.int(PAGINATION_OFFSET, 0) + protected List getPaginationLinks(Object object, Long total, Parameters params) { + long offset = params.long(PAGINATION_OFFSET, 0L) int max = params.int(PAGINATION_MAX, 10) String sort = params.get(PAGINATION_SORT) String order = params.get(PAGINATION_ORDER) getPaginationLinks(object, total, max, offset, sort, order) } - protected List getPaginationLinks(Object object, Integer total, Integer max, Integer offset, String sort, String order) { + protected List getPaginationLinks(Object object, Long total, Integer max, Long offset, String sort, String order) { Map linkParams = buildPaginateParams(max, offset, sort, order) List links = [] @@ -176,17 +176,17 @@ class DefaultJsonViewHelper extends DefaultGrailsViewHelper { if (total > max) { Map firstParams = paramsWithOffset(linkParams, 0) links << new Link('first', link(resource: object, method: HttpMethod.GET, absolute: true, params: firstParams)) - Integer prevOffset = getPrevOffset(offset, max) + Long prevOffset = getPrevOffset(offset, max) if (prevOffset != null) { Map prevParams = paramsWithOffset(linkParams, prevOffset) links << new Link('prev', link(resource: object, method: HttpMethod.GET, absolute: true, params: prevParams)) } - Integer nextOffset = getNextOffset(total, offset, max) + Long nextOffset = getNextOffset(total, offset, max) if (nextOffset) { Map nextParams = paramsWithOffset(linkParams, nextOffset) links << new Link('next', link(resource: object, method: HttpMethod.GET, absolute: true, params: nextParams)) } - Integer lastOffset = getLastOffset(total, max) + Long lastOffset = getLastOffset(total, max) if (lastOffset) { Map lastParams = paramsWithOffset(linkParams, lastOffset) links << new Link('last', link(resource: object, method: HttpMethod.GET, absolute: true, params: lastParams)) @@ -196,7 +196,7 @@ class DefaultJsonViewHelper extends DefaultGrailsViewHelper { return links } - protected Map buildPaginateParams(Integer max, Integer offset, String sort, String order) { + protected Map buildPaginateParams(Integer max, Long offset, String sort, String order) { Map params = [:] params.put(PAGINATION_OFFSET, offset) params.put(PAGINATION_MAX, max) @@ -230,7 +230,7 @@ class DefaultJsonViewHelper extends DefaultGrailsViewHelper { * @param offset The new offset to use * @return The resulting parameters */ - protected Map paramsWithOffset(Map originalParameters, Integer offset) { + protected Map paramsWithOffset(Map originalParameters, Long offset) { Map params = [:] originalParameters.each { String k, Object v -> params.put(k, v) @@ -239,26 +239,26 @@ class DefaultJsonViewHelper extends DefaultGrailsViewHelper { return params } - protected Integer getPrevOffset(Integer offset, Integer max) { - if (offset <= 0) { + protected Long getPrevOffset(Long offset, Integer max) { + if (offset <= 0L) { return null } - return Math.max(offset - max, 0) + return Math.max(offset - max, 0L) } - protected Integer getNextOffset(Integer total, Integer offset, Integer max) { - if (offset < 0 || offset + max >= total) { + protected Long getNextOffset(Long total, Long offset, Integer max) { + if (offset < 0L || offset + max >= total) { return null } return offset + max } - protected Integer getLastOffset(Integer total, Integer max) { - if (total <= 0) { + protected Long getLastOffset(Long total, Integer max) { + if (total <= 0L) { return null } - Integer laststep = ((int) Math.round(Math.ceil((double) total / max))) - 1 - return Math.max((laststep * max), 0) + long laststep = ((long) Math.ceil((double) total / max)) - 1L + return Math.max(laststep * max, 0L) } groovy.json.JsonGenerator getGenerator() { diff --git a/grails-views-gson/src/test/groovy/grails/plugin/json/view/IterableRenderSpec.groovy b/grails-views-gson/src/test/groovy/grails/plugin/json/view/IterableRenderSpec.groovy index 72661d201dd..2536059a128 100644 --- a/grails-views-gson/src/test/groovy/grails/plugin/json/view/IterableRenderSpec.groovy +++ b/grails-views-gson/src/test/groovy/grails/plugin/json/view/IterableRenderSpec.groovy @@ -254,6 +254,30 @@ class IterableRenderSpec extends Specification implements JsonViewUnitTest { ''') } + void 'Test render a collection type with JSON API and a total above Integer.MAX_VALUE'() { + given: 'A collection' + mappingContext.addPersistentEntities(Player, Team) + Player player = new Player(name: 'Cantona') + player.id = 1 + def players = [player] + + when: 'the total exceeds Integer.MAX_VALUE, as a large table count does' + def renderResult = render(''' + import groovy.transform.* + import grails.plugin.json.view.iterable.Player + + @Field Collection players + + json jsonapi.render(players, [pagination: [resource: Player, total: 3_000_000_000L]]) + ''', [players: players], { + uri = '/foo' + }) + + then: 'the last link keeps the 64-bit offset instead of wrapping negative' + renderResult.jsonText.contains('offset=2999999990') + !renderResult.jsonText.contains('offset=-') + } + void 'Test render a collection type with JSON API and pagination override max'() { given: 'A collection' mappingContext.addPersistentEntities(Player, Team) diff --git a/grails-views-gson/src/test/groovy/grails/plugin/json/view/api/PaginationSpec.groovy b/grails-views-gson/src/test/groovy/grails/plugin/json/view/api/PaginationSpec.groovy index c4fd28b6388..80ce93bfafa 100644 --- a/grails-views-gson/src/test/groovy/grails/plugin/json/view/api/PaginationSpec.groovy +++ b/grails-views-gson/src/test/groovy/grails/plugin/json/view/api/PaginationSpec.groovy @@ -79,7 +79,7 @@ class PaginationSpec extends Specification { @Unroll("getLastOffset(total:#total, max:#max) == #expectedOffset") void "test last offests"() { when: - Integer lastOffset = jsonViewHelper.getLastOffset(total, max) + Long lastOffset = jsonViewHelper.getLastOffset(total, max) then: lastOffset == expectedOffset @@ -96,12 +96,14 @@ class PaginationSpec extends Specification { 99 | 5 | 95 100 | 5 | 95 101 | 5 | 100 + 3_000_000_000L | 10 | 2_999_999_990L + 3_000_000_001L | 10 | 3_000_000_000L } @Unroll("getPrevOffset(offset:#offset, max:#max) == #expectedOffset") void "test prev offests"() { when: - Integer prevOffset = jsonViewHelper.getPrevOffset(offset, max) + Long prevOffset = jsonViewHelper.getPrevOffset(offset, max) then: prevOffset == expectedOffset @@ -119,12 +121,13 @@ class PaginationSpec extends Specification { 99 | 5 | 94 100 | 5 | 95 101 | 5 | 96 + 3_000_000_000L | 10 | 2_999_999_990L } @Unroll("getNextOffset(total:#total, offset:#offset, max:#max) == #expectedOffset") void "test getNextOffset"() { when: - Integer nextOffset = jsonViewHelper.getNextOffset(total, offset, max) + Long nextOffset = jsonViewHelper.getNextOffset(total, offset, max) then: nextOffset == expectedOffset @@ -142,6 +145,8 @@ class PaginationSpec extends Specification { 100 | 6 | 5 | 11 100 | 90 | 5 | 95 100 | 94 | 5 | 99 + 3_000_000_000L | 2_999_999_980L | 10 | 2_999_999_990L + 3_000_000_000L | 2_999_999_990L | 10 | null 100 | 95 | 5 | null 100 | 99 | 5 | null