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. 1 Skipped Deployment
|
|
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? |
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.