ENG-1907 - Update content uniqueness and upsert surfaces#1198
ENG-1907 - Update content uniqueness and upsert surfaces#1198mdroidian wants to merge 3 commits into
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
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.