jdaugherty commented on code in PR #16511:
URL: https://github.com/apache/grails-core/pull/16511#discussion_r4178034464
##########
grails-datamapping-core-test/src/test/groovy/grails/gorm/services/multitenancy/partitioned/PartitionMultiTenancySpec.groovy:
##########
@@ -72,6 +72,32 @@ class PartitionMultiTenancySpec extends Specification {
System.setProperty(SystemPropertyTenantResolver.PROPERTY_NAME, '')
}
+ void 'an instance whose tenant id names another tenant is saved under the
current tenant'() {
+ given: 'a current tenant'
+ System.setProperty(SystemPropertyTenantResolver.PROPERTY_NAME, '910')
+
+ when: 'a book with the tenant id of another tenant is saved'
+ Book book = Book.withTransaction { new Book(title: 'Inserted',
tenantId: 911).save(flush: true) }
+
+ then: 'it gets the current tenant, and only the current tenant sees it'
+ book.tenantId == 910
+ Book.withTransaction { Book.countByTitle('Inserted') } == 1
+ Book.withTenant('911') { Book.withTransaction {
Book.countByTitle('Inserted') } } == 0
+
+ when: 'its tenant id is changed to another tenant and it is saved
again'
+ Book updated = Book.withTransaction {
+ Book loaded = Book.get(book.id)
+ loaded.tenantId = 911
+ loaded.save(flush: true)
+ }
+
+ then: 'it keeps the current tenant'
+ updated.tenantId == 910
Review Comment:
The insert half checks what each tenant sees, but the update half only
checks the property on the returned instance. Since the point is that the
update does not move the book, could this block check that through queries as
well?
```groovy
then: 'it keeps the current tenant'
updated.tenantId == 910
Book.withTransaction { Book.countByTitle('Inserted') } == 1
Book.withTenant('911') { Book.withTransaction {
Book.countByTitle('Inserted') } } == 0
```
Both pass locally on this branch.
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/multitenancy/MultiTenantEventListenerSpec.groovy:
##########
@@ -249,20 +249,42 @@ class MultiTenantEventListenerSpec extends Specification {
eventType << [ValidationEvent, PreInsertEvent, PreUpdateEvent]
}
- void "onApplicationEvent PreInsertEvent prefers an already-set entity
property over the resolved tenant id"() {
+ @Unroll
+ void "onApplicationEvent #eventType.simpleName replaces a tenant id
already set on the entity with the current tenant id"() {
Review Comment:
Not on this line: the class Javadoc at line 46 still lists "inserts now
prefer an already-set entity property over the resolved tenant id" among the
behaviours this spec covers, which this change reverses. Could you update it to
say that an already-set tenant id is kept only when the current id is
`ConnectionSource.DEFAULT`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
