feat(relations): introduce typed relation definitions and select relation loading#9
Merged
cungminh2710 merged 2 commits intoMar 29, 2026
Conversation
Contributor
Greptile SummaryThis PR introduces first-class relation metadata to the schema layer (
Confidence Score: 4/5Safe to merge after fixing the empty-result unknown-relation silent failure; all other findings are style/robustness suggestions. One P1 defect: WithRelations with an invalid name silently returns nil when the base query yields 0 rows, directly contradicting the stated contract. The schema layer, model parsing, and query builder changes are solid. The two P2 findings are robustness/style and do not block merge, but the P1 should be addressed. pkg/rain/relation_loading.go — unknown-relation validation bypassed for empty result sets; also two unguarded map lookups that could panic if called outside their validated context. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+62
to
+83
| func (q *SelectQuery) loadRelationsIntoSlice(ctx context.Context, parents reflect.Value) error { | ||
| tableSource, ok := q.table.(tableDefSource) | ||
| if !ok { | ||
| return fmt.Errorf("rain: relation loading requires a concrete table source") | ||
| } | ||
|
|
||
| if parents.Len() == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| for _, relationName := range q.relationNames { | ||
| relation, exists := tableSource.table.RelationByName(relationName) | ||
| if !exists { | ||
| return fmt.Errorf("rain: unknown relation %q on table %q", relationName, tableSource.table.Name) | ||
| } | ||
| if err := q.loadRelation(ctx, parents, relation); err != nil { | ||
| return err | ||
| } | ||
| } | ||
|
|
||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
loadRelationsIntoSlice returns nil early when parents.Len() == 0 (line 68–70), which means the relation name validation loop on lines 72–80 is never reached. If the base query returns 0 rows, an invalid relation name like WithRelations("does_not_exist") is silently accepted instead of returning the expected "unknown relation" error.
The test in query_internal_test.go happens to pass only because alice and bob are always present in the database at the time the unknown-relation assertion runs. A query that returns zero rows (e.g. filtering by a non-existent ID) would skip the error entirely.
The fix is to validate relation names before the early-return guard:
func (q *SelectQuery) loadRelationsIntoSlice(ctx context.Context, parents reflect.Value) error {
tableSource, ok := q.table.(tableDefSource)
if !ok {
return fmt.Errorf("rain: relation loading requires a concrete table source")
}
// Validate all relation names first, regardless of result-set size.
for _, relationName := range q.relationNames {
if _, exists := tableSource.table.RelationByName(relationName); !exists {
return fmt.Errorf("rain: unknown relation %q on table %q", relationName, tableSource.table.Name)
}
}
if parents.Len() == 0 {
return nil
}
for _, relationName := range q.relationNames {
relation, _ := tableSource.table.RelationByName(relationName)
if err := q.loadRelation(ctx, parents, relation); err != nil {
return err
}
}
return nil
}Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/rain/relation_loading.go
Line: 62-83
Comment:
**Unknown relation silently ignored for empty result sets**
`loadRelationsIntoSlice` returns `nil` early when `parents.Len() == 0` (line 68–70), which means the relation name validation loop on lines 72–80 is never reached. If the base query returns 0 rows, an invalid relation name like `WithRelations("does_not_exist")` is silently accepted instead of returning the expected `"unknown relation"` error.
The test in `query_internal_test.go` happens to pass only because `alice` and `bob` are always present in the database at the time the unknown-relation assertion runs. A query that returns zero rows (e.g. filtering by a non-existent ID) would skip the error entirely.
The fix is to validate relation names before the early-return guard:
```go
func (q *SelectQuery) loadRelationsIntoSlice(ctx context.Context, parents reflect.Value) error {
tableSource, ok := q.table.(tableDefSource)
if !ok {
return fmt.Errorf("rain: relation loading requires a concrete table source")
}
// Validate all relation names first, regardless of result-set size.
for _, relationName := range q.relationNames {
if _, exists := tableSource.table.RelationByName(relationName); !exists {
return fmt.Errorf("rain: unknown relation %q on table %q", relationName, tableSource.table.Name)
}
}
if parents.Len() == 0 {
return nil
}
for _, relationName := range q.relationNames {
relation, _ := tableSource.table.RelationByName(relationName)
if err := q.loadRelation(ctx, parents, relation); err != nil {
return err
}
}
return nil
}
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+114
to
+135
| for _, sourceKey := range sourceKeys { | ||
| query := &SelectQuery{runner: q.runner, dialect: q.dialect, table: tableDefSource{table: relation.TargetTable}} | ||
| relatedRows := reflect.New(reflect.SliceOf(relatedElemType)) | ||
| if err := query.Where(schema.ComparisonExpr{Left: schema.Ref(relation.TargetColumn), Operator: "=", Right: schema.ValueExpr{Value: sourceKey}}). | ||
| Scan(ctx, relatedRows.Interface()); err != nil { | ||
| if err == sql.ErrNoRows { | ||
| continue | ||
| } | ||
| return err | ||
| } | ||
| for rowIdx := 0; rowIdx < relatedRows.Elem().Len(); rowIdx++ { | ||
| related := relatedRows.Elem().Index(rowIdx) | ||
| targetValue, ok, err := relationColumnValue(related, relation.TargetColumn.Name) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !ok { | ||
| continue | ||
| } | ||
| relatedByTargetKey[toTypedKey(targetValue)] = append(relatedByTargetKey[toTypedKey(targetValue)], related) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
The inner loop fires one SELECT per entry in sourceKeys, so loading a relation across N distinct parent key values issues N round-trips to the database. For a page of 50 posts, that's 50 queries just to load the author.
The ADR acknowledges this as a known v1 trade-off ("batched IN loading" is listed as a future optimization), so this is informational rather than blocking. However, it may be worth adding a brief code comment so future maintainers see the intentional constraint and know exactly where to target the optimization:
// TODO: replace per-key queries with a single IN-clause query
// (see docs/adr/2026-03-28-typed-relations-design.md).
for _, sourceKey := range sourceKeys {Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/rain/relation_loading.go
Line: 114-135
Comment:
**N+1 query per distinct source-key value**
The inner loop fires one `SELECT` per entry in `sourceKeys`, so loading a relation across N distinct parent key values issues N round-trips to the database. For a page of 50 posts, that's 50 queries just to load the author.
The ADR acknowledges this as a known v1 trade-off ("batched `IN` loading" is listed as a future optimization), so this is informational rather than blocking. However, it may be worth adding a brief code comment so future maintainers see the intentional constraint and know exactly where to target the optimization:
```go
// TODO: replace per-key queries with a single IN-clause query
// (see docs/adr/2026-03-28-typed-relations-design.md).
for _, sourceKey := range sourceKeys {
```
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+180
to
+198
| func (q *SelectQuery) relationElementType(parent reflect.Value, relation schema.RelationDef) (reflect.Type, error) { | ||
| meta, _, err := lookupModelMeta(parent.Addr().Interface()) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| fieldInfo := meta.byRelation[relation.Name] | ||
| field := parent.FieldByIndex(fieldInfo.index) | ||
| switch relation.Type { | ||
| case schema.RelationTypeBelongsTo: | ||
| if field.Kind() == reflect.Pointer { | ||
| return field.Type().Elem(), nil | ||
| } | ||
| return field.Type(), nil | ||
| case schema.RelationTypeHasMany: | ||
| return field.Type().Elem(), nil | ||
| default: | ||
| return nil, fmt.Errorf("rain: unsupported relation type %q", relation.Type) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
ok guard when indexing meta.byRelation in relationElementType and setRelationValue
relationElementType (line 185) and setRelationValue (line 224) both do:
fieldInfo := meta.byRelation[relation.Name]
field := parent.FieldByIndex(fieldInfo.index)without checking the ok return value. If relation.Name is absent from the map, fieldInfo.index is nil, and FieldByIndex(nil) panics.
In the current call graph this can't happen because validateRelationField is always called first for every parent. However, since relationElementType is a method on *SelectQuery, it could be called independently by future code paths. Adding a guard (matching the pattern already used in validateRelationField) would make both functions safe in isolation:
fieldInfo, ok := meta.byRelation[relation.Name]
if !ok {
return nil, fmt.Errorf("rain: relation %q not found in model metadata", relation.Name)
}Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/rain/relation_loading.go
Line: 180-198
Comment:
**Missing `ok` guard when indexing `meta.byRelation` in `relationElementType` and `setRelationValue`**
`relationElementType` (line 185) and `setRelationValue` (line 224) both do:
```go
fieldInfo := meta.byRelation[relation.Name]
field := parent.FieldByIndex(fieldInfo.index)
```
without checking the `ok` return value. If `relation.Name` is absent from the map, `fieldInfo.index` is `nil`, and `FieldByIndex(nil)` panics.
In the current call graph this can't happen because `validateRelationField` is always called first for every parent. However, since `relationElementType` is a method on `*SelectQuery`, it could be called independently by future code paths. Adding a guard (matching the pattern already used in `validateRelationField`) would make both functions safe in isolation:
```go
fieldInfo, ok := meta.byRelation[relation.Name]
if !ok {
return nil, fmt.Errorf("rain: relation %q not found in model metadata", relation.Name)
}
```
How can I resolve this? If you propose a fix, please make it concise.
cungminh2710
deleted the
codex/linear-mention-qc-227-introduce-typed-relation-definitions
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
belongs_toandhas_manyrelations and the query layer can load related rows without manual join wiring.Description
RelationTypeandRelationDef, and storing relations onTableDefwithRelationByNamelookup.TableModel.BelongsTo(...)andTableModel.HasMany(...)with validation and duplicate checks, and preserve relations when aliasing table handles viacloneTableDef.rain:"relation:<name>"tags and cache relation-targeted fields alongsidedbcolumn tags.SelectQuery.WithRelations(...)and wireSelectQuery.Scanto perform relation-aware scanning when relation names are requested.pkg/rain/relation_loading.go, which: loads related rows after base scan, enforces mapping rules (struct/ptr forbelongs_to, slice forhas_many), and returns clear errors for unknown or misconfigured relations.belongs_toandhas_many, unknown-relation failure case, and an ADR atdocs/adr/2026-03-28-typed-relations-design.mddescribing the v1 design and mapping rules.Testing
make fmt(applied code formatting) andmake lint(no lint issues reported). Both succeeded.go test ./pkg/schema ./pkg/rainand fullmake test; unit and integration tests passed for the modified packages (okfor both packages; overall test run succeeded).Codex Task