apache / apache/grails-core

Inappropriate nested cascade validation of associations

Open
#14,599 0 comments 2 reactions 0 assignees View on GitHub
relates-to: gorm
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.