Skip to content

ENG-2039 Create a generic upsert route#1237

Open
maparent wants to merge 3 commits into
mainfrom
eng-2039-create-a-generic-upsert-route
Open

ENG-2039 Create a generic upsert route#1237
maparent wants to merge 3 commits into
mainfrom
eng-2039-create-a-generic-upsert-route

Conversation

@maparent

@maparent maparent commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

@linear-code

linear-code Bot commented Jul 18, 2026

Copy link
Copy Markdown

ENG-2039

@vercel

vercel Bot commented Jul 18, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
discourse-graph Ready Ready Preview, Comment Jul 20, 2026 2:39pm

Request Review

@supabase

supabase Bot commented Jul 18, 2026

Copy link
Copy Markdown

This pull request has been ignored for the connected project zytfjzqyijgagqxrzbmz because there are no changes detected in packages/database/supabase directory. You can change this behaviour in Project Integrations Settings ↗︎.


Preview Branches by Supabase.
Learn more about Supabase Branching ↗︎.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 potential issues.

View 1 additional finding in Devin Review.

Open in Devin Review

Comment thread apps/website/app/api/internal/space/[id]/route.ts
Comment thread packages/database/src/lib/crossAppConverters.ts
@graphite-app

graphite-app Bot commented Jul 19, 2026

Copy link
Copy Markdown
Contributor

PR size/scope check

This PR is over our review-size guideline.

  • Recommended: ~200 lines changed
  • Acceptable limit: up to 400 lines when well-scoped/self-contained
  • Preferred file count: fewer than 5 files

Please split this into smaller PRs unless there is a clear reason the changes need to land together.

If keeping it as one PR, please add a brief justification covering:

  • What single problem this PR solves
  • Why the files/changes are coupled

Comment thread packages/database/features/addGeneric.feature Outdated
@maparent
maparent force-pushed the eng-2039-create-a-generic-upsert-route branch from 201f07c to 42ce851 Compare July 19, 2026 02:02
@maparent
maparent force-pushed the eng-2039-create-a-generic-upsert-route branch from 42ce851 to b368c4c Compare July 19, 2026 13:11
@maparent
maparent force-pushed the eng-2039-create-a-generic-upsert-route branch 2 times, most recently from e557164 to bbe2ba9 Compare July 19, 2026 13:16
@maparent
maparent force-pushed the eng-2039-create-a-generic-upsert-route branch from bbe2ba9 to fe2fa8f Compare July 19, 2026 13:22
Comment thread packages/database/src/lib/crossAppConverters.ts Outdated

@mdroidian mdroidian left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving this as a comment mid review, because the action-specific function comment should be discussed.

if (userId === undefined)
return createApiResponse(
request,
asPostgrestFailure("Please login", "invalid", 401),

@mdroidian mdroidian Jul 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this be passed to the user? If so, as a user, when I receive this message, what should I do with it? Stated differently, what's the use case for when a user would hit this failure route and what action could they take to rectify it?

@@ -0,0 +1,167 @@
import { NextResponse, NextRequest } from "next/server";

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's name this route something more explicit to what it's doing. Right now it only conveys something about the space.
How about /api/internal/space/[id]/graph/upsert or just /graph if we plan on adding the GET here too.

Something to convey this is Concept, Content, and Document but not User, Space, etc

...missingNodeSchemaIds,
...missingRelationTypeSchemaIds,
];
if (missingSchemaIds.length > 0) {

@mdroidian mdroidian Jul 19, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maparent My understanding from our discussion on the 17th (discussion ref, specifically this ref) was that the action-specific functions would assemble the data required for the action before calling this generic route. This block instead fetches schemas that weren’t included in the request and converts them back to CrossApp types.

For example, if publishNodes is sharing a Claim and Evidence connected by Supports, the action-specific function should send the nodes, their node schemas, the relation, the Supports relation type schema, and the Claim–Supports–Evidence triple schema. Given that callers are expected to go through these action-specific functions, when would this route receive a relation triple schema without its referenced node and relation type schemas? If that shouldn’t happen, could we validate and reject the incomplete payload instead of fetching the dependencies here? That would also remove the need for the DB-to-CrossApp converters in this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants