matrei commented on issue #14365: URL: https://github.com/apache/grails-core/issues/14365#issuecomment-6608658583
+1 for the narrower option, but please target 7.0.x rather than 7.2.x. Fixes are still landing on 7.0.x for 7.0.18, and the code involved is the same there, so it can go in on 7.0.x and merge up. It only enforces constraints an app explicitly declared on the tenant id, and Hibernate already uses those for the schema, so I'm fine with it in a patch. A few things for the PR: 1. `canPropertyBeConstrained` always returns `true`, so there's nothing to change there. Please also leave `isConstrainableProperty` as it is: it's what keeps the default `nullable: false` off the tenant id, so changing it would make a tenant id that declares only, say, `maxSize` non-nullable. 2. Sending the tenant id through the existing non-derived branch also runs `applyDefaultConstraints`. That applies the global `'*'` constraints from `grails.gorm.default.constraints`, and creates a constrained property for a tenant id that declares nothing. Both go beyond "declared only". Could you add tests that pin this down: global constraints don't reach the tenant id, a tenant id with only `maxSize` stays nullable, and an undeclared tenant id gets no constrained property? 3. Hibernate 7 isn't on any 7.x branch, so the Hibernate 7 spec can't be part of the 7.0.x PR. Please add it in a follow-up PR against 8.0.x once the fix has been merged up. For the docs and release notes line, these are the cases that will notice the change: unit tests that don't configure discriminator mode (the listener that fills in the tenant id isn't registered then), saves inside `Tenants.withoutId`, and `DATABASE`/`SCHEMA` mode with a tenant id property. -- 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]
