ENG-446 concept upsert functions #215
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 📝 WalkthroughWalkthroughThis update introduces a new composite type for representing concept data with local references, and adds two PostgreSQL functions: one to convert local concept inputs to database records by resolving local identifiers, and another to upsert multiple concepts from JSON input, handling conflicts and returning affected IDs. Type definitions are updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant DB
Client->>DB: Call upsert_concepts(space_id, concepts_json)
loop For each concept in concepts_json
DB->>DB: Convert JSON to concept_local_input
DB->>DB: local_concept_to_db_concept(input)
DB->>DB: Resolve local IDs to DB IDs
DB->>DB: Prepare Concept record
DB->>DB: INSERT INTO Concept (on conflict update)
alt Unique violation (other constraint)
DB->>DB: Log warning, return -1
end
end
DB-->>Client: Return array of inserted/updated concept IDs
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
packages/database/supabase/migrations/20250611180757_concept_upsert_functions.sql (2)
1-21: Samearityomission as in schema fileThe composite type declared here duplicates the one in
concept.sqland carries the same missingarityfield. Keep both definitions identical to prevent migration drift.
94-97: Duplicate default JSON with unknownaritykeyMirrors the comment in the schema file – remove or add the column for consistency.
🧹 Nitpick comments (2)
packages/database/supabase/schemas/concept.sql (2)
94-97: Default-record JSON contains unknown keysSame issue from the caller side: the literal used to seed defaults includes
"arity", which currently has no effect. If you elect not to addarityto the composite, drop the key here to avoid dead code paths.- local_concept := jsonb_populate_record(NULL::public.concept_local_input, '{"epistemic_status": "unknown", "arity":0, "literal_content":{},"reference_content":{},"is_schema":false}'); + local_concept := jsonb_populate_record( + NULL::public.concept_local_input, + '{"epistemic_status":"unknown","literal_content":{},"reference_content":{},"is_schema":false}' + );
85-90:v_platformis selected but never usedThe
SELECT platform INTO STRICT v_platformguard succeeds (or raises) but the variable is not referenced afterwards.
Unless you plan to validate platform-specific constraints later, remove the variable and theSELECTto reduce noise and pointless work.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/database/supabase/migrations/20250611180757_concept_upsert_functions.sql(1 hunks)packages/database/supabase/schemas/concept.sql(1 hunks)packages/database/types.gen.ts(3 hunks)
🔇 Additional comments (3)
packages/database/supabase/schemas/concept.sql (2)
152-173: Overwrites existingreference_contentunconditionallyIf the incoming JSON already contained a valid
reference_content, providinglocal_reference_contentwill replace it entirely rather than merge.
Confirm this is intentional; otherwise considerjsonb_concat()or keyed merges to avoid data loss.
101-121: ```shell
#!/bin/bash
set -eShow the definition of concept_local_input
echo "---- concept_local_input definition ----"
rg -n "CREATE TYPE public\.concept_local_input" -A10 packages/database/supabase/schemas/concept.sqlShow usage of 'arity' in the file
echo -e "\n---- occurrences of 'arity' ----"
rg -n "arity" packages/database/supabase/schemas/concept.sqlShow the upsert_concepts function context
echo -e "\n---- upsert_concepts function ----"
rg -n "upsert_concepts" -A10 packages/database/supabase/schemas/concept.sql</details> <details> <summary>packages/database/types.gen.ts (1)</summary> `672-691`: **Generated type will need regeneration if `arity` is added** Once you update the composite definition, remember to regenerate `types.gen.ts`; otherwise TypeScript will be out of sync with the database schema. </details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
|
@CodeRabbit: arity will be handled separately. But removing it from the constants right now. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 41435 @maparent Got it! I can see you're referring to the hardcoded The current line in the migration: local_concept := jsonb_populate_record(NULL::public.concept_local_input, '{"epistemic_status": "unknown", "arity":0, "literal_content":{},"reference_content":{},"is_schema":false}');Removing the arity from those defaults will allow for more flexible arity handling based on your specific business logic. This approach gives you better control over how arity is determined for each concept rather than always defaulting to 0. |
|
@CodeRabbit: Actually will remove arity from constants in the other pr. |
|
Tip For best results, initiate chat on the files or code changes. @maparent Understood! That makes perfect sense to handle the arity constant removal in a separate PR. It's good practice to keep changes focused - this PR handles the new concept upsert functionality, and the arity constant cleanup can be addressed independently in its own PR. Thanks for the clarification! |
maparent
force-pushed
the
ENG-446-bulk-db-concept-upsert
branch
from
4a8b7ff to
074ab07
Compare
maparent
force-pushed
the
ENG-446-bulk-db-concept-upsert
branch
from
074ab07 to
d0afedb
Compare
maparent
force-pushed
the
ENG-446-bulk-db-concept-upsert
branch
from
d0afedb to
92d0931
Compare
maparent
force-pushed
the
ENG-446-bulk-db-concept-upsert
branch
from
92d0931 to
a8ec1bf
Compare
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Overall looks good, but we could use more documentation for the caller on things like "What does this do", "What are the required inputs", "What are some conflicts that might arise and what to do about them", "What return value(s) should I look for and what should I do about them", etc
maparent
force-pushed
the
ENG-446-bulk-db-concept-upsert
branch
from
a8ec1bf to
42633c9
Compare
Summary by CodeRabbit
New Features
Documentation