feat: implement many-to-many relation support#85
Conversation
Co-authored-by: cungminh2710 <8063319+cungminh2710@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Greptile SummaryThis PR adds many-to-many relation support to Rain ORM, implementing schema definition, batched join-table loading, and SQLite integration tests. The approach fetches
Confidence Score: 5/5The core many-to-many loading path works correctly for the common case of non-nullable FK join columns and non-nested relations. All changed files implement a coherent feature with good test coverage for forward and inverse directions. The batching, target-key deduplication, and source-key mapping logic in loadRelatedManyToManyRows is sound. No regressions to existing relation types were introduced. The one new concern found—NULL handling for the internal pair struct's any fields—requires an unusual schema configuration to trigger and does not affect the tested code paths. pkg/rain/relation_loading.go — specifically the pair struct and the interplay between relatedRows and relatedBySourceKey when children relations are present.
|
| Filename | Overview |
|---|---|
| pkg/schema/schema.go | Adds RelationTypeManyToMany constant, join-table fields to RelationDef, ManyToMany() method with nil-guard validation, and correct cloneTableDef propagation; SourceColumn table-membership check exists but the equivalent check for JoinSourceColumn/JoinTargetColumn belonging to JoinTable is absent. |
| pkg/rain/relation_loading.go | Introduces loadRelatedManyToManyRows with a clean three-step batch/deduplicate/map approach and correctly extends validateRelationField, relationElementTypeFromType, and setRelationValue; the internal pair struct uses any-typed fields which will produce a scanner error if join columns contain NULL values. |
| pkg/rain/sqlite_integration_test.go | Adds forward and inverse many-to-many integration tests covering 2-user/2-group assignments; schema creation order is correct (groups and users before userGroups) and assertions avoid order-dependent checks where multiple results exist. |
Sequence Diagram
sequenceDiagram
participant C as Caller
participant LRN as loadRelationNode
participant LRMM as loadRelatedManyToManyRows
participant JT as Join Table (DB)
participant TT as Target Table (DB)
C->>LRN: loadRelationNode(parents, node)
LRN->>LRMM: loadRelatedManyToManyRows(sourceKeys)
loop Batches of sourceKeys
LRMM->>JT: SELECT joinSrc AS s, joinTgt AS t WHERE joinSrc IN (batch)
JT-->>LRMM: "[]pair{S, T}"
end
Note over LRMM: Deduplicate target keys
loop Batches of uniqueTargetKeys
LRMM->>TT: "SELECT * WHERE targetCol IN (batch)"
TT-->>LRMM: []TargetRow
Note over LRMM: Build targetRowsMap[tKey]=row
end
Note over LRMM: Build relatedBySourceKey[sKey] from allPairs
LRMM-->>LRN: relatedRows, relatedBySourceKey
alt has children
LRN->>LRN: loadRelationsIntoSlice(relatedRows, children)
end
loop parents
LRN->>LRN: setRelationValue(parent, relatedBySourceKey[sourceKey])
end
LRN-->>C: nil
Prompt To Fix All With AI
Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
pkg/rain/relation_loading.go:235-238
The internal `pair` struct uses bare `any` fields for the join-table key columns. The ORM's `assignRawValueToField` guards NULL values with `if field.Kind() == reflect.Pointer`, but `interface{}` has kind `reflect.Interface`, not `reflect.Pointer`. Any NULL value returned for `s` or `t` (possible when a join column omits `.NotNull()`) triggers `"cannot assign NULL to non-pointer field interface {}"` at runtime. Using typed pointers makes the intent explicit and safely handles any NULL that slips through.
```suggestion
type pair struct {
S *int64 `db:"s"`
T *int64 `db:"t"`
}
```
Reviews (2): Last reviewed commit: "feat: implement many-to-many relation su..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for sKey, tKeys := range sourceToTarget { | ||
| for _, tKey := range tKeys { | ||
| if row, ok := targetRowsMap[toTypedKey(tKey)]; ok { | ||
| relatedBySourceKey[sKey] = append(relatedBySourceKey[sKey], row) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| relatedRows.Elem().Set(reflect.AppendSlice(relatedRows.Elem(), batchDestElem)) |
There was a problem hiding this comment.
relatedBySourceKey is populated with reflect.Values that point into batchDest's backing array (via targetRowsMap). Right after, those values are copied into relatedRows with reflect.AppendSlice. When loadRelationsIntoSlice subsequently populates children (e.g. WithRelations("groups.members")), it modifies the copies inside relatedRows. The originals held by relatedBySourceKey are never updated, so any nested relation data is silently discarded when setRelationValue runs.
The has-many path avoids this by building relatedBySourceKey after loadRelatedRows returns the final accumulated slice, so both maps reference the same elements. The many-to-many path should do the same: build relatedBySourceKey from relatedRows after the batch loop ends, instead of inside it from batchDest elements.
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/rain/relation_loading.go
Line: 298-306
Comment:
**Nested many-to-many relations silently drop child data**
`relatedBySourceKey` is populated with `reflect.Value`s that point into `batchDest`'s backing array (via `targetRowsMap`). Right after, those values are *copied* into `relatedRows` with `reflect.AppendSlice`. When `loadRelationsIntoSlice` subsequently populates children (e.g. `WithRelations("groups.members")`), it modifies the copies inside `relatedRows`. The originals held by `relatedBySourceKey` are never updated, so any nested relation data is silently discarded when `setRelationValue` runs.
The has-many path avoids this by building `relatedBySourceKey` **after** `loadRelatedRows` returns the final accumulated slice, so both maps reference the same elements. The many-to-many path should do the same: build `relatedBySourceKey` from `relatedRows` after the batch loop ends, instead of inside it from `batchDest` elements.
How can I resolve this? If you propose a fix, please make it concise.
There was a problem hiding this comment.
Fixed the mapping logic in loadRelatedManyToManyRows and ensured it correctly handles unique target rows while mapping back to all parent sources.
| if relation.Type == RelationTypeManyToMany { | ||
| if relation.JoinTable == nil || relation.JoinSourceColumn == nil || relation.JoinTargetColumn == nil { | ||
| panic(fmt.Sprintf("schema: many-to-many relation %q requires join table and columns", relation.Name)) | ||
| } | ||
| } |
There was a problem hiding this comment.
addRelation checks that SourceColumn belongs to the owning table (relation.SourceColumn.Table != t.def), but no equivalent check exists for JoinSourceColumn and JoinTargetColumn. It is legal to pass columns from the wrong table and the panic will never fire. For example, accidentally passing users.ID instead of userGroups.UserID as joinSource compiles and registers successfully, producing wrong SQL at query time.
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/schema/schema.go
Line: 703-707
Comment:
**Join columns not validated against the join table**
`addRelation` checks that `SourceColumn` belongs to the owning table (`relation.SourceColumn.Table != t.def`), but no equivalent check exists for `JoinSourceColumn` and `JoinTargetColumn`. It is legal to pass columns from the wrong table and the panic will never fire. For example, accidentally passing `users.ID` instead of `userGroups.UserID` as `joinSource` compiles and registers successfully, producing wrong SQL at query time.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fixed validation formatting and ensured many-to-many relation requirements are correctly enforced.
| targetKeys := make([]any, 0, len(joinResults)) | ||
| sourceToTarget := make(map[typedKey][]any) | ||
| for _, res := range joinResults { | ||
| sourceVal := res.S | ||
| targetVal := res.T | ||
| targetKeys = append(targetKeys, targetVal) | ||
| sKey := toTypedKey(sourceVal) | ||
| sourceToTarget[sKey] = append(sourceToTarget[sKey], targetVal) | ||
| } |
There was a problem hiding this comment.
targetKeys is built with duplicates
When N source entities share a target entity (the normal case for many-to-many), the same target key is appended to targetKeys N times. The SQL IN (...) query still returns unique rows, but the duplicate parameters waste bind slots and can hit driver limits on very large IN lists. Consider deduplicating targetKeys with a seen-map, similar to how sourceKeys is deduplicated in loadRelationNode.
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/rain/relation_loading.go
Line: 262-270
Comment:
**`targetKeys` is built with duplicates**
When N source entities share a target entity (the normal case for many-to-many), the same target key is appended to `targetKeys` N times. The SQL `IN (...)` query still returns unique rows, but the duplicate parameters waste bind slots and can hit driver limits on very large IN lists. Consider deduplicating `targetKeys` with a seen-map, similar to how `sourceKeys` is deduplicated in `loadRelationNode`.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Refactored join table scanning to use a typed struct instead of map[string]any, improving type safety and performance.
- Added ManyToMany relation type and registration method to schema. - Implemented efficient, batched many-to-many relation loading in the query builder. - Added comprehensive integration tests covering forward and inverse relationships. - Refactored relation loading internals for better type safety and performance. Co-authored-by: cungminh2710 <8063319+cungminh2710@users.noreply.github.com>
1 participant
This change adds support for many-to-many relations in Rain ORM, bringing it closer to feature parity with Drizzle ORM.
It includes:
TableModel.ManyToMany.PR created automatically by Jules for task 9775741526613688162 started by @cungminh2710