Inappropriate nested cascade validation of associations
- Dominant language
- Groovy
- Stars
- 2.9k
- Forks
- 975
- Avg merge
- 1d 22h
- Merged PRs (30d)
- 92
Description
This is complicated to reproduce as it requires several layers of associations, but it can be done at least if you have a single domain A with a one to many association to B which has another one to many on itself B, then another one to many from A to C which has a one to many to D which has one to many to E which then fails validation.
When A.validate() is called it results in the generated `fixedField` in `org.springframework.validation.AbstractBindingResult` is
`B[0].childB[0].A.C[0].D[0].E[4].value`.
The `validatedObjects` Set at this point contains A, B[0] and childB[0] has just been added.
This has occured I believe due to lines 245 of `grails.gorm.validation.PersistentEntityValidator` where it calls
```
if (associatedPersistentProperty instanceof Association) {
if(association.doesCascade(CascadeType.PERSIST, CascadeType.MERGE)) {
if(association.isBidirectional() && associatedPersistentProperty == association.inverseSide) {
// If this property is the inverse side of the currently processed association then
// we don't want to process it because that would cause a potential infinite loop
continue
}
cascadeToAssociativeProperty(
associatedObject,
errors,
associatedReflector,
(Association)associatedPersistentProperty,
validatedObjects)
}
}
```
I believe its actually supposed to check the cascade and bidirectionality of the associatedPersistentProperty rather than the original association which means the code should read
```
if (associatedPersistentProperty instanceof Association) {
if(((Association)associatedPersistentProperty).doesCascade(CascadeType.PERSIST, CascadeType.MERGE)) {
if(((Association)associatedPersistentProperty).isBidirectional() && associatedPersistentProperty == association.inverseSide) {
// If this property is the inverse side of the currently processed association then
// we don't want to process it because that would cause a potential infinite loop
continue
}
cascadeToAssociativeProperty(
associatedObject,
errors,
associatedReflector,
(Association)associatedPersistentProperty,
validatedObjects)
}
}
```
Alternatively, if the intent is to check the `association` rather than the `associatedPersistentProperty` the solution would be to check the `validatedObjects` set at line 112 and 113 to see if the associatedObject has already been validated.
```
if(associatedObject != null && proxyHandler?.isInitialized(associatedObject) && !validatedObjects.contains(associatedObject)) {
if(association.isOwningSide() || association.doesCascade(CascadeType.PERSIST, CascadeType.MERGE)) {
```
Thanks for reporting an issue for GORM, please review the task list below before submitting the
issue.
> WARNING: Your issue report will be closed if the issue report is incomplete and does not include an example. Make sure the below tasks not completed!
> NOTE: If you are unsure about something and the issue is more of a question a better place to ask questions is on Stack Overflow (http://stackoverflow.com/tags/grails) or Slack (http://slack-signup.grails.org). DO NOT use the issue tracker to ask questions.
### Task List
- [x] Steps to reproduce provided
- [x] Stacktrace (if present) provided
- [ ] Example that reproduces the problem uploaded to Github
- [x] Full description of the issue provided (see below)
### Steps to Reproduce
See Above
### Expected Behaviour
Should not revalidated the associated property
### Actual Behaviour
Validated into the nested associated property.
### Environment Information
- **Operating System**: Mac OS Sierra
- **GORM Version:** 6.1.8
- **Grails Version (if using Grails):** 3.3.1
- **JDK Version:** 8u141-oracle
### Example Application
- TODO: link to github repository with example that reproduces the issue
Contributor guide
Research direction
Start in grails.gorm.validation.PersistentEntityValidator, especially the cascade logic around lines 245 and 112-113. Reproduce the nested A/B/C/D/E association case described in the issue, then determine whether cascade and inverse-side checks use the associated property correctly. Done means A.validate() no longer revalidates the inappropriate nested association.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- groovy
- Domain
- backend
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 42/100