ENG-1907 adjustments#1236
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Updates to Preview Branch (eng-1907-update-content-uniqueness-and-upsert-surfaces-with-original) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| id: number | ||
| last_modified: string | ||
| metadata: Json | ||
| original: boolean |
There was a problem hiding this comment.
🟡 Content 'original' flag typed as always-present, hiding possible empty value
The original field of Content is declared as a plain boolean (original: boolean at packages/database/src/dbTypes.ts:267, and original?: boolean at :284 and :301) even though the underlying database column is nullable and the feature intentionally stores an empty value for non-original rows, so code reading this field can receive an unexpected empty value the type says is impossible.
Impact: Callers relying on this type may mishandle rows where the flag is empty, leading to incorrect logic when distinguishing original from non-original content.
Nullability mismatch between base table type and view types
The schema defines the column as original BOOLEAN DEFAULT true with no NOT NULL (packages/database/supabase/schemas/content.sql:93), and the migration comment states "Use null, never false for the non-original rows." Because the column is nullable, the generated types should be boolean | null. The views that select this same column are correctly typed boolean | null (packages/database/src/dbTypes.ts:1070, :1183, :1637), confirming the base-table Content type at :267 (and the Insert/Update at :284/:301) is inconsistent and understates nullability.
Prompt for agents
The Content table's `original` column is nullable (defined as `original BOOLEAN DEFAULT true` with no NOT NULL in packages/database/supabase/schemas/content.sql), and the design intentionally stores NULL for non-original rows. However the generated dbTypes declare it as non-nullable: Row `original: boolean` (packages/database/src/dbTypes.ts:267), Insert `original?: boolean` (:284), and Update `original?: boolean` (:301). These should be `boolean | null` / `original?: boolean | null` to match the actual column nullability, consistent with how the my_contents and my_contents_with_embedding views already type it as `boolean | null`. Regenerate the types from the migrated schema (or hand-fix these three lines) so consumers correctly account for the null case.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
I added another comment; this field should be true or false. False is translated to null in the upsert function. Not defining it will be interpreted as true, following the default, and breaking the usual pattern that not defining keeps the original value. I think this makes the upsert api more usable, hiding the complexity of null.
8181f93 to
28c5067
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Add an original column to Content. Add a full index including original to Content; To allow multiple distinct format translations, set original to NULL instead of FALSE. Adjust the ForeignKey from FileReference to Content to use that index, instead of adding content_type to ForeignKey. That last change is worth discussing.
28c5067 to
0287faa
Compare
1 participant
Here are my proposed changes:
Add an original column to Content.
Add a full index including original to Content;
To allow multiple distinct format translations, set original to NULL instead of FALSE.
Note that this will not be a concern most of the time; we go through the upsert functions, and they expect us to use false.
(Another option, cleaner than using NULL or FALSE in original, would be my earlier idea of a nullable "translation_of" reference column, which also allows a unique index. Then the FileReference would have an always-null column there.)
Adjust the ForeignKey from FileReference to Content to use that index, instead of adding content_type to ForeignKey. That last change is worth discussing.
Rationale:
I think the file references would be the same between formats, and I don't think we should re-compute them.
But there are counter-arguments: translation could in theory require to translate assets. I am betting against that.
https://www.loom.com/share/2981055089e1450ba015badc417b26c6
Note : I stacked a new PR on yours, because I cannot tag you for review otherwise.