feat: add support for database views and serial types#81
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
Confidence Score: 3/5Not safe to merge without addressing the migration ordering issue — on a fresh schema containing both views and tables, the migrator can emit CREATE VIEW before the tables those views query, causing the migration to fail at runtime. The serial-type additions and DML guards on views are correct. The gap is in orderManagedTables: it builds its topological order from foreign-key edges only, so views always get inDegree=0 and sort alphabetically ahead of whatever tables they reference. In any fresh migration where a view name sorts before its underlying table name, the generated SQL plan is invalid. pkg/migrator/snapshot.go (orderManagedTables and BuildSnapshot view handling) and pkg/dialect/dialect.go (BaseDialect bigserial fallback inconsistency)
|
| Filename | Overview |
|---|---|
| pkg/dialect/dialect.go | Added SMALLSERIAL/SERIAL cases to BaseDialect.DataType; bigserial still returns BIGSERIAL, inconsistent with the new plain-integer fallbacks for the other two serial types. |
| pkg/migrator/snapshot.go | Two issues: orderManagedTables only tracks FK edges so views sort before dependent tables in fresh migrations; column/index snapshotting runs unconditionally for views. |
| pkg/migrator/diff.go | Adds view-aware diffing with drop-and-recreate; normalizeSQL comparison is correct. |
| pkg/rain/ddl.go | createViewSQL correctly uses useLiterals mode; serial-type logic updated correctly. |
| pkg/rain/query_compile.go | Adds useLiterals flag and writeLiteral helper; skipCTEs correctly saved/restored. |
| pkg/schema/schema.go | Adds TypeSmallSerial/TypeSerial, SmallSerial()/Serial() helpers, DefineView, IsView/ViewQuery fields; cloneTableDef correctly propagates view fields. |
Sequence Diagram
sequenceDiagram
participant User
participant schema
participant rain/ddl
participant migrator/snapshot
participant migrator/diff
User->>schema: DefineView(name, query, fn)
schema-->>User: "T (IsView=true, ViewQuery set)"
User->>migrator/snapshot: BuildSnapshot(dialect, tables)
migrator/snapshot->>migrator/snapshot: orderManagedTables (FK topo-sort only)
Note over migrator/snapshot: Views (no FKs) may sort before dependent tables
migrator/snapshot->>rain/ddl: CreateTableSQL(view)
rain/ddl-->>migrator/snapshot: CREATE VIEW name AS SELECT ...
migrator/snapshot-->>User: "Snapshot with IsView=true"
User->>migrator/diff: DiffSnapshots(prev, current)
alt View changed
migrator/diff-->>User: DROP VIEW + CREATE VIEW statements
else No change
migrator/diff-->>User: empty plan
end
User->>rain/ddl: Insert/Update/Delete on view
rain/ddl-->>User: error: cannot mutate view
Comments Outside Diff (2)
-
pkg/migrator/snapshot.go, line 221-251 (link)Views created before their dependent tables in fresh migrations
orderManagedTablesbuilds its topological order exclusively from foreign-key edges. Views carry no foreign keys, so they always start withinDegree = 0and are emitted first, sorted alphabetically against other zero-in-degree nodes. Whenever a view name sorts before the name of a table it queries (e.g.active_usersbeforeusers),planCreateAllemits theCREATE VIEWstatement before theCREATE TABLE, which most databases reject. Any schema that includes both views and tables is affected on a fresh migration (no previous snapshot).Prompt To Fix With AI
This is a comment left during a code review. Path: pkg/migrator/snapshot.go Line: 221-251 Comment: **Views created before their dependent tables in fresh migrations** `orderManagedTables` builds its topological order exclusively from foreign-key edges. Views carry no foreign keys, so they always start with `inDegree = 0` and are emitted first, sorted alphabetically against other zero-in-degree nodes. Whenever a view name sorts before the name of a table it queries (e.g. `active_users` before `users`), `planCreateAll` emits the `CREATE VIEW` statement before the `CREATE TABLE`, which most databases reject. Any schema that includes both views and tables is affected on a fresh migration (no previous snapshot). How can I resolve this? If you propose a fix, please make it concise.
-
pkg/migrator/snapshot.go, line 94-121 (link)Column/index snapshotting runs unconditionally for views
BuildSnapshotcallsddl.ColumnDefinitionSQL,ddl.AddConstraintSQL,ddl.AddForeignKeySQL, andddl.CreateIndexesSQLfor every managed table, including views. Column and index metadata have no meaning for views andcreateIndexesSQLwould happily emitCREATE INDEX … ON view_nameSQL if a developer accidentally adds index declarations inside aDefineViewcallback — statements that all major databases reject at runtime.Prompt To Fix With AI
This is a comment left during a code review. Path: pkg/migrator/snapshot.go Line: 94-121 Comment: **Column/index snapshotting runs unconditionally for views** `BuildSnapshot` calls `ddl.ColumnDefinitionSQL`, `ddl.AddConstraintSQL`, `ddl.AddForeignKeySQL`, and `ddl.CreateIndexesSQL` for every managed table, including views. Column and index metadata have no meaning for views and `createIndexesSQL` would happily emit `CREATE INDEX … ON view_name` SQL if a developer accidentally adds index declarations inside a `DefineView` callback — statements that all major databases reject at runtime. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
Fix the following 3 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 3
pkg/migrator/snapshot.go:221-251
**Views created before their dependent tables in fresh migrations**
`orderManagedTables` builds its topological order exclusively from foreign-key edges. Views carry no foreign keys, so they always start with `inDegree = 0` and are emitted first, sorted alphabetically against other zero-in-degree nodes. Whenever a view name sorts before the name of a table it queries (e.g. `active_users` before `users`), `planCreateAll` emits the `CREATE VIEW` statement before the `CREATE TABLE`, which most databases reject. Any schema that includes both views and tables is affected on a fresh migration (no previous snapshot).
### Issue 2 of 3
pkg/migrator/snapshot.go:94-121
**Column/index snapshotting runs unconditionally for views**
`BuildSnapshot` calls `ddl.ColumnDefinitionSQL`, `ddl.AddConstraintSQL`, `ddl.AddForeignKeySQL`, and `ddl.CreateIndexesSQL` for every managed table, including views. Column and index metadata have no meaning for views and `createIndexesSQL` would happily emit `CREATE INDEX … ON view_name` SQL if a developer accidentally adds index declarations inside a `DefineView` callback — statements that all major databases reject at runtime.
### Issue 3 of 3
pkg/dialect/dialect.go:64-69
`bigserial` still returns "BIGSERIAL" in `BaseDialect` while the two new serial types return plain integer variants. A custom dialect that embeds `BaseDialect` would get auto-increment semantics for `bigserial` columns but not for `smallserial` or `serial`, silently dropping auto-increment behavior on those columns.
```suggestion
case "smallserial":
return "SMALLINT"
case "serial":
return "INTEGER"
case "bigserial":
return "BIGINT"
```
Reviews (1): Last reviewed commit: "feat(schema): add support for database v..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| case "smallserial": | ||
| return "SMALLINT" | ||
| case "serial": | ||
| return "INTEGER" | ||
| case "bigserial": | ||
| return "BIGSERIAL" |
There was a problem hiding this comment.
bigserial still returns "BIGSERIAL" in BaseDialect while the two new serial types return plain integer variants. A custom dialect that embeds BaseDialect would get auto-increment semantics for bigserial columns but not for smallserial or serial, silently dropping auto-increment behavior on those columns.
| case "smallserial": | |
| return "SMALLINT" | |
| case "serial": | |
| return "INTEGER" | |
| case "bigserial": | |
| return "BIGSERIAL" | |
| case "smallserial": | |
| return "SMALLINT" | |
| case "serial": | |
| return "INTEGER" | |
| case "bigserial": | |
| return "BIGINT" |
Prompt To Fix With AI
This is a comment left during a code review.
Path: pkg/dialect/dialect.go
Line: 64-69
Comment:
`bigserial` still returns "BIGSERIAL" in `BaseDialect` while the two new serial types return plain integer variants. A custom dialect that embeds `BaseDialect` would get auto-increment semantics for `bigserial` columns but not for `smallserial` or `serial`, silently dropping auto-increment behavior on those columns.
```suggestion
case "smallserial":
return "SMALLINT"
case "serial":
return "INTEGER"
case "bigserial":
return "BIGINT"
```
How can I resolve this? If you propose a fix, please make it concise.
1 participant
This change introduces support for database views and additional serial types (SMALLSERIAL, SERIAL) to bring Rain ORM closer to feature parity with Drizzle ORM.
Key changes:
TypeSmallSerialandTypeSerialto schema types andTableModelhelpers.DefineViewfor code-first view definitions.PR created automatically by Jules for task 15314982305874579590 started by @cungminh2710