Add nested delete support for associations in GORM #7597
Conversation
- Introduced `parseNestedDelete` and `deleteNestedAssociations` functions to handle nested deletions based on specified relationships. - Enhanced `DeleteBeforeAssociations` to support nested deletes when associations are included in the select statement. - Updated `deleteAssociation` to manage deletion logic for various relationship types, including HasOne, HasMany, Many2Many, and BelongsTo. - Added comprehensive tests for nested delete scenarios, covering various relationship types and error handling. This update improves the flexibility and robustness of the delete operations in GORM, allowing for more complex data structures to be managed effectively.
|
Add nested association delete support Adds first-class support for cascading/nested deletes when Key Changes• New helper functions Affected Areas• This summary was automatically generated by @propel-code-bot |
![propel-code-bot[bot]](https://avatars.githubusercontent.com/in/1179004?s=60&v=4){kind=small}
callbacks/delete.go
Outdated
| } | ||
| } | ||
|
|
||
| if err := loadDB.First(db.Statement.Dest).Error; err != nil { |
There was a problem hiding this comment.
[CriticalError]
Missing error handling: The database query on line 342 silently ignores errors, which can lead to incomplete deletion operations. The error should be handled properly:
| if err := loadDB.First(db.Statement.Dest).Error; err != nil { | |
| if err := loadDB.First(db.Statement.Dest).Error; err != nil { | |
| db.AddError(err) | |
| return | |
| } |
⚡ Committable suggestion
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Context for Agents
[**CriticalError**]
Missing error handling: The database query on line 342 silently ignores errors, which can lead to incomplete deletion operations. The error should be handled properly:
```suggestion
if err := loadDB.First(db.Statement.Dest).Error; err != nil {
db.AddError(err)
return
}
```
⚡ **Committable suggestion**
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
File: callbacks/delete.go
Line: 342
propel-code-bot
bot
changed the title
| associatedRecords := reflect.New(reflect.SliceOf(rel.FieldSchema.ModelType)) | ||
| query := selectQuery.Table(rel.FieldSchema.Table). | ||
| Joins("INNER JOIN "+joinTable+" ON "+rel.FieldSchema.Table+"."+rel.FieldSchema.PrimaryFieldDBNames[0]+" = "+joinTable+"."+rel.References[len(rel.References)-1].ForeignKey.DBName). | ||
| Where(strings.Join(joinConditions, " AND "), queryArgs...) |
There was a problem hiding this comment.
[CriticalError]
Array bounds access without validation: Line 507 accesses rel.References[len(rel.References)-1] without checking if rel.References is empty. If rel.References has length 0, this will panic with an index out of bounds error.
Suggested Change
| associatedRecords := reflect.New(reflect.SliceOf(rel.FieldSchema.ModelType)) | |
| query := selectQuery.Table(rel.FieldSchema.Table). | |
| Joins("INNER JOIN "+joinTable+" ON "+rel.FieldSchema.Table+"."+rel.FieldSchema.PrimaryFieldDBNames[0]+" = "+joinTable+"."+rel.References[len(rel.References)-1].ForeignKey.DBName). | |
| Where(strings.Join(joinConditions, " AND "), queryArgs...) | |
| if len(rel.References) == 0 { | |
| return reflect.Value{}, fmt.Errorf("no references found for relationship") | |
| } | |
| associatedRecords := reflect.New(reflect.SliceOf(rel.FieldSchema.ModelType)) | |
| query := selectQuery.Table(rel.FieldSchema.Table). | |
| Joins("INNER JOIN "+joinTable+" ON "+rel.FieldSchema.Table+"."+rel.FieldSchema.PrimaryFieldDBNames[0]+" = "+joinTable+"."+rel.References[len(rel.References)-1].ForeignKey.DBName). | |
| Where(strings.Join(joinConditions, " AND "), queryArgs...) |
⚡ Committable suggestion
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Context for Agents
[**CriticalError**]
Array bounds access without validation: Line 507 accesses `rel.References[len(rel.References)-1]` without checking if `rel.References` is empty. If `rel.References` has length 0, this will panic with an index out of bounds error.
<details>
<summary>Suggested Change</summary>
```suggestion
if len(rel.References) == 0 {
return reflect.Value{}, fmt.Errorf("no references found for relationship")
}
associatedRecords := reflect.New(reflect.SliceOf(rel.FieldSchema.ModelType))
query := selectQuery.Table(rel.FieldSchema.Table).
Joins("INNER JOIN "+joinTable+" ON "+rel.FieldSchema.Table+"."+rel.FieldSchema.PrimaryFieldDBNames[0]+" = "+joinTable+"."+rel.References[len(rel.References)-1].ForeignKey.DBName).
Where(strings.Join(joinConditions, " AND "), queryArgs...)
```
⚡ **Committable suggestion**
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
</details>
File: callbacks/delete.go
Line: 508| associatedRecords := reflect.New(reflect.SliceOf(rel.FieldSchema.ModelType)) | ||
| query := selectQuery.Table(rel.FieldSchema.Table). | ||
| Joins("INNER JOIN "+joinTable+" ON "+rel.FieldSchema.Table+"."+rel.FieldSchema.PrimaryFieldDBNames[0]+" = "+joinTable+"."+rel.References[len(rel.References)-1].ForeignKey.DBName). | ||
| Where(strings.Join(joinConditions, " AND "), queryArgs...) |
There was a problem hiding this comment.
[CriticalError]
Array bounds access without validation: Line 507 also accesses rel.FieldSchema.PrimaryFieldDBNames[0] without checking if the slice is empty. If PrimaryFieldDBNames is empty, this will panic with an index out of bounds error.
Suggested Change
| associatedRecords := reflect.New(reflect.SliceOf(rel.FieldSchema.ModelType)) | |
| query := selectQuery.Table(rel.FieldSchema.Table). | |
| Joins("INNER JOIN "+joinTable+" ON "+rel.FieldSchema.Table+"."+rel.FieldSchema.PrimaryFieldDBNames[0]+" = "+joinTable+"."+rel.References[len(rel.References)-1].ForeignKey.DBName). | |
| Where(strings.Join(joinConditions, " AND "), queryArgs...) | |
| if len(rel.References) == 0 || len(rel.FieldSchema.PrimaryFieldDBNames) == 0 { | |
| return reflect.Value{}, fmt.Errorf("missing references or primary field names for relationship") | |
| } | |
| associatedRecords := reflect.New(reflect.SliceOf(rel.FieldSchema.ModelType)) | |
| query := selectQuery.Table(rel.FieldSchema.Table). | |
| Joins("INNER JOIN "+joinTable+" ON "+rel.FieldSchema.Table+"."+rel.FieldSchema.PrimaryFieldDBNames[0]+" = "+joinTable+"."+rel.References[len(rel.References)-1].ForeignKey.DBName). | |
| Where(strings.Join(joinConditions, " AND "), queryArgs...) |
⚡ Committable suggestion
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
Context for Agents
[**CriticalError**]
Array bounds access without validation: Line 507 also accesses `rel.FieldSchema.PrimaryFieldDBNames[0]` without checking if the slice is empty. If `PrimaryFieldDBNames` is empty, this will panic with an index out of bounds error.
<details>
<summary>Suggested Change</summary>
```suggestion
if len(rel.References) == 0 || len(rel.FieldSchema.PrimaryFieldDBNames) == 0 {
return reflect.Value{}, fmt.Errorf("missing references or primary field names for relationship")
}
associatedRecords := reflect.New(reflect.SliceOf(rel.FieldSchema.ModelType))
query := selectQuery.Table(rel.FieldSchema.Table).
Joins("INNER JOIN "+joinTable+" ON "+rel.FieldSchema.Table+"."+rel.FieldSchema.PrimaryFieldDBNames[0]+" = "+joinTable+"."+rel.References[len(rel.References)-1].ForeignKey.DBName).
Where(strings.Join(joinConditions, " AND "), queryArgs...)
```
⚡ **Committable suggestion**
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
</details>
File: callbacks/delete.go
Line: 508
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for nested delete operations in GORM, allowing cascading deletion of associated records when using Select() with relationship names during delete operations.
Key changes:
- Added logic to parse and handle nested relationship deletion via
Select()clause (e.g.,Select("Posts.Comments")) - Refactored the delete association logic to support HasOne, HasMany, BelongsTo, and ManyToMany relationships
- Added comprehensive test coverage for nested deletion scenarios including deep nesting, polymorphic relations, and self-referential structures
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| callbacks/delete.go | Implements nested delete functionality with new helper functions for parsing nested selects and deleting associations based on relationship types |
| tests/delete_test.go | Adds 8 new test functions covering various nested delete scenarios including HasMany, BelongsTo, ManyToMany, embedded structs, deep nesting, multiple relations, polymorphic, and self-referential relationships |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
callbacks/delete.go
Outdated
| if records.Elem().Len() == 0 { | ||
| return nil |
There was a problem hiding this comment.
Potential panic when calling Len() on a non-slice/array reflect.Value. For HasOne relationships, records is a pointer to a single struct (line 432), not a slice, so Elem().Len() will panic. Check if the value is actually a slice before calling Len(), or handle HasOne separately.
| if records.Elem().Len() == 0 { | |
| return nil | |
| if rel.Type == schema.HasOne { | |
| // For HasOne, check if the struct is zero (not found) | |
| if records.Elem().IsZero() { | |
| return nil | |
| } | |
| } else { | |
| // For HasMany, check if the slice is empty | |
| if records.Elem().Len() == 0 { | |
| return nil | |
| } |
callbacks/delete.go
Outdated
| return err | ||
| } | ||
|
|
||
| if associatedRecords.Elem().Len() > 0 { |
There was a problem hiding this comment.
Potential panic if findMany2ManyAssociatedRecords returns an empty reflect.Value{} (line 497). Before calling Elem().Len(), check if associatedRecords.IsValid() to avoid panicking on an invalid reflect.Value.
| if associatedRecords.Elem().Len() > 0 { | |
| if associatedRecords.IsValid() && associatedRecords.Elem().Len() > 0 { |
| DB.Create(&user) | ||
|
|
||
| var deletedUser NestedDeleteDeepNestingUser | ||
| result := DB.Select("Posts.Comments").Delete(&deletedUser, user.ID) |
There was a problem hiding this comment.
It looks like this is the only test for nested associations, there must many cases are not covered.
There was a problem hiding this comment.
I added more tests and solved issues pointed out higher. Please let me know if it's enough, or if you have other idea about what tests should be there :) Thanks
2 participants
What did this pull request do?
close #6687
User Case Description