ENG-1907 - Update content uniqueness and upsert surfaces#1198
Conversation
…riants - Introduced a new scenario in the feature file to test coexistence of different content types. - Updated step definitions to include checks for content rows based on variant and content type. - Modified database types to include `content_type` in relevant tables and relationships. - Adjusted SQL schemas to enforce content type constraints and ensure proper indexing. - Backfilled existing rows with default content types to maintain data integrity.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Updates to Preview Branch (eng-1907-update-content-uniqueness-and-upsert-surfaces) ↗︎
Tasks are run on every commit but only new migration files are pushed.
View logs for this Workflow Run ↗︎. |
|
@codex review |
| filehash character varying NOT NULL, -- or binary? | ||
| "created" timestamp without time zone NOT NULL, | ||
| last_modified timestamp without time zone NOT NULL, | ||
| content_type character varying NOT NULL DEFAULT 'text/obsidian+markdown', |
There was a problem hiding this comment.
Defaulting FileReference.content_type to text/obsidian+markdown preserves current behavior because Obsidian is the only file-reference writer today and does not pass this value yet. This is not a hard modeling choice: longer term, callers should probably pass the parent Content.content_type explicitly, e.g. Roam would use text/roam+markdown. Happy to make the DB stricter now if we prefer requiring that on insert.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e21302e23
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
This comment was marked as resolved.
This comment was marked as resolved.
|
One concern with this PR: PR #1154 added That means any omitted My preference is to make |
|
TBH I'm not sure that's a good idea as is, and I remember deciding against it, and I thought it had been discussed. If we do this, we will have multiple formats of a content, and we won't know which one is the original/source of truth. First, what is your use case of having multiple formats? I can see an optimization to avoid repeating translation work. In that case, what I would suggest if you think the time saving is worth the storage cost (which is plausible but I think we should analyze a bit), is to add another converted_from column, saying that a given content row was converted from another content row. Then we would know which one is the source of truth, and we could still have a unique key for the original triple (without format) when the converted_from column is empty. Or am I missing a use case? |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
| CREATE UNIQUE INDEX IF NOT EXISTS content_space_local_id_variant_content_type_originals_idx ON public."Content" USING btree ( | ||
| space_id, source_local_id, variant, original | ||
| ); |
There was a problem hiding this comment.
This unique index conflicts with the PR's goal. The index on (space_id, source_local_id, variant, original) prevents multiple Content rows with the same (space_id, source_local_id, variant) when original=true. However, the PR explicitly allows multiple representations (e.g., markdown and ATJSON) to coexist with the same variant='full' as shown in the test scenario (lines 137-184 of addContent.feature). Since Content.original defaults to true (line 93 of content.sql), this index will cause a unique constraint violation when inserting the second content row in the test.
-- This index should be removed or original should be nullable
-- and only set to true for one canonical representation
CREATE UNIQUE INDEX IF NOT EXISTS content_space_local_id_variant_content_type_originals_idx
ON public."Content" USING btree (space_id, source_local_id, variant, original);The test expects 2 Content rows with variant='full' and different content_types, but this will fail because both will have original=true by default, violating this unique constraint.
| CREATE UNIQUE INDEX IF NOT EXISTS content_space_local_id_variant_content_type_originals_idx ON public."Content" USING btree ( | |
| space_id, source_local_id, variant, original | |
| ); | |
| CREATE UNIQUE INDEX IF NOT EXISTS content_space_local_id_variant_content_type_originals_idx ON public."Content" USING btree ( | |
| space_id, source_local_id, variant, content_type | |
| ); | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
d10befd to
5b1ca3e
Compare
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.
| ON CONFLICT (space_id, source_local_id, variant, content_type) DO UPDATE SET | ||
| document_id = COALESCE(db_content.document_id, EXCLUDED.document_id), | ||
| author_id = COALESCE(db_content.author_id, EXCLUDED.author_id), | ||
| creator_id = COALESCE(db_content.creator_id, EXCLUDED.creator_id), | ||
| created = COALESCE(db_content.created, EXCLUDED.created), | ||
| text = COALESCE(db_content.text, EXCLUDED.text), | ||
| metadata = COALESCE(db_content.metadata, EXCLUDED.metadata), | ||
| scale = COALESCE(db_content.scale, EXCLUDED.scale), | ||
| last_modified = COALESCE(db_content.last_modified, EXCLUDED.last_modified), | ||
| part_of_id = COALESCE(db_content.part_of_id, EXCLUDED.part_of_id), | ||
| original = db_content.original |
There was a problem hiding this comment.
Critical bug in upsert logic: The ON CONFLICT clause uses (space_id, source_local_id, variant, content_type) but unconditionally updates original field on conflict. This allows overwriting an original=true content row with original=NULL (when original: false is passed), which breaks the foreign key constraint from FileReference that requires original=true.
Example failure:
-- Insert original content
INSERT INTO Content (..., original) VALUES (..., true);
-- FileReference now points to this row
-- Upsert same content with original=false
-- This updates original to NULL, breaking FileReference!
INSERT INTO Content (..., original) VALUES (..., NULL)
ON CONFLICT (..., content_type) DO UPDATE SET original = NULL;Fix: Either exclude original from the UPDATE clause, or add validation to prevent changing original from true to NULL:
ON CONFLICT (space_id, source_local_id, variant, content_type) DO UPDATE SET
...,
-- Don't allow overwriting original=true with NULL
original = CASE
WHEN EXCLUDED.original IS NULL THEN Content.original
ELSE EXCLUDED.original
END| ON CONFLICT (space_id, source_local_id, variant, content_type) DO UPDATE SET | |
| document_id = COALESCE(db_content.document_id, EXCLUDED.document_id), | |
| author_id = COALESCE(db_content.author_id, EXCLUDED.author_id), | |
| creator_id = COALESCE(db_content.creator_id, EXCLUDED.creator_id), | |
| created = COALESCE(db_content.created, EXCLUDED.created), | |
| text = COALESCE(db_content.text, EXCLUDED.text), | |
| metadata = COALESCE(db_content.metadata, EXCLUDED.metadata), | |
| scale = COALESCE(db_content.scale, EXCLUDED.scale), | |
| last_modified = COALESCE(db_content.last_modified, EXCLUDED.last_modified), | |
| part_of_id = COALESCE(db_content.part_of_id, EXCLUDED.part_of_id), | |
| original = db_content.original | |
| ON CONFLICT (space_id, source_local_id, variant, content_type) DO UPDATE SET | |
| document_id = COALESCE(db_content.document_id, EXCLUDED.document_id), | |
| author_id = COALESCE(db_content.author_id, EXCLUDED.author_id), | |
| creator_id = COALESCE(db_content.creator_id, EXCLUDED.creator_id), | |
| created = COALESCE(db_content.created, EXCLUDED.created), | |
| text = COALESCE(db_content.text, EXCLUDED.text), | |
| metadata = COALESCE(db_content.metadata, EXCLUDED.metadata), | |
| scale = COALESCE(db_content.scale, EXCLUDED.scale), | |
| last_modified = COALESCE(db_content.last_modified, EXCLUDED.last_modified), | |
| part_of_id = COALESCE(db_content.part_of_id, EXCLUDED.part_of_id), | |
| original = CASE | |
| WHEN EXCLUDED.original IS NULL THEN db_content.original | |
| ELSE EXCLUDED.original | |
| END | |
Spotted by Graphite
Is this helpful? React 👍 or 👎 to let us know.
2 participants
Updated content identity so multiple representations of the same source item can coexist.
Contentis now unique on(space_id, source_local_id, variant, content_type), andupsert_contentuses that same key so, for example, afullmarkdown row and afullATJSON row do not overwrite each other.FileReferencenow includescontent_typeso asset references point to the exact parentContentrepresentation. The current default istext/obsidian+markdownfor compatibility with the existing Obsidian asset sync path; future callers should pass the parent content type explicitly.Also updated the embedding view/types and added a feature scenario covering two
fullcontent rows with different content types.Also fixes ENG-1908 because changing
Contentuniqueness to includecontent_typemakes the oldFileReferenceforeign key ambiguous. UpdatingFileReferenceto includecontent_typeis part of making ENG-1907’s new content identity valid.