hk2166 commented on issue #14365: URL: https://github.com/apache/grails-core/issues/14365#issuecomment-6541048323
I'd like to work on this. It still happens on 7.2.x and 8.0.x. I reproduced it on 7.2.x with a `SimpleMapDatastore` in discriminator mode: a `nullable: false` and a `maxSize` declared on the tenant id are both ignored, so `validate()` passes for a `Customer` without a `firmId`, or with a tenant id longer than the limit. **Cause:** `MappingFactory.createTenantId` maps every tenant id as a derived property, and has done so since multi-tenancy was added. `DefaultConstraintEvaluator` drops the constraints of derived properties: it logs "Derived properties may not be constrained", and `canPropertyBeConstrained` returns false for them. The Hibernate binders already treat the tenant id as an exception (`mappedForm.isDerived() && !(grailsProp instanceof TenantId)` in `GrailsDomainBinder`, and the same check in Hibernate 7's `SimpleValueBinder`). The constraint evaluator doesn't. **Proposed fix:** the evaluator applies the same exception for `TenantId`, in both the evaluation loop and `canPropertyBeConstrained`, so the constraints declared on the tenant id are checked. During validation, `MultiTenantEventListener` assigns the current tenant id on the `ValidationEvent`, so a normal save sees the id and passes. The fix only changes what happens when no tenant id is assigned (`Tenants.withoutId`, or no multi-tenancy mode as in the example) or when the assigned id breaks a declared constraint. One question before I start: should the tenant id also get the default constraints, like any other property (`nullable: false` unless declared otherwise)? That would make an app that saves without a current tenant and never declared constraints on the tenant id fail validation where it saves today. My suggestion is to apply only the constraints declared on the tenant id on 7.2.x, since those are the ones the warning says are dropped, and leave the defaults as a possible change for a later minor. **Tests:** - `DefaultConstraintEvaluatorSpec`: the evaluator keeps the declared constraints of a tenant id. - A multi-tenancy spec in `grails-datamapping-core-test`: `validate()` and `save()` with no tenant id, with a tenant id that breaks a declared constraint, and with one the resolver assigns. - A Hibernate 5 spec through the real event listener. I'd target 7.2.x, since 7.0.x and 7.1.x reach end of support today, and it would merge forward. @jdaugherty @borinquenkid, does that sound right? -- 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]
