Skip to content

ENG-2166 upsertConcepts overwrites too aggressively #1334

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
maparent wants to merge 2 commits into
base: main
Choose a base branch
from
+145 −10
Draft

ENG-2166 upsertConcepts overwrites too aggressively #1334

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9d21e72
no-overwrite-on-upsert
maparent Aug 22, 2026
7c01939
more cases, cucumber
maparent Aug 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 120 additions & 0 deletions packages/database/features/addConcepts.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -184,3 +184,123 @@ Feature: Concept upsert
| _id | @is_relation | @arity | @_reference_content | @literal_content |
| ns1 | false | 0 | {} | {} |
| rts1 | true | 2 | {"destination": "ns1", "source": "ns1"} | {"roles":["source","destination"]} |

# A client that knows nothing of a field must not erase what another client stored:
# in upsert_concepts, a value absent from the input leaves the stored one alone.
Scenario: Fields absent from an upsert keep their stored value
Given user user1 upserts these concepts to space s1:
"""
[
{
"name": "Evidence",
"author_local_id": "user1",
"source_local_id": "ns1",
"created": "2000/01/01",
"last_modified": "2001/01/02",
"is_schema": true
},
{
"name": "A Source",
"author_local_id": "user1",
"source_local_id": "n2",
"schema_represented_by_local_id": "ns1",
"created": "2000/01/03",
"last_modified": "2001/01/04"
},
{
"name": "An Evidence",
"author_local_id": "user1",
"source_local_id": "n1",
"schema_represented_by_local_id": "ns1",
"description": "a description",
"epistemic_status": "certain",
"created": "2000/01/03",
"last_modified": "2001/01/04",
"literal_content": {
"roles": ["source"]
},
"local_reference_content": {
"sourceDocument": "n2"
}
}
]
"""
# A second client rewrites the same node, sending only what it knows about
When user user1 upserts these concepts to space s1:
"""
[
{
"name": "An Evidence, renamed",
"source_local_id": "n1",
"created": "2000/01/03",
"last_modified": "2002/02/02"
}
]
"""
Then a user logged in space s1 and calling getConcepts with these parameters: '{"scope":{"type":"nodes","nodeIds":["n1"]},"fields":{"concepts":["id","source_local_id","name","description","epistemic_status","author_id","schema_id","literal_content","reference_content"]}}'
And query results should look like this
| _id | name | description | epistemic_status | _author_id | _schema_id | @literal_content | @_reference_content |
| n1 | An Evidence, renamed | a description | certain | user1 | ns1 | {"roles": ["source"]} | {"sourceDocument": "n2"} |

# The stored values are still clearable, by saying so explicitly
When user user1 upserts these concepts to space s1:
"""
[
{
"name": "An Evidence, renamed",
"source_local_id": "n1",
"description": "",
"epistemic_status": "unknown",
"created": "2000/01/03",
"last_modified": "2003/03/03",
"literal_content": {},
"local_reference_content": {}
}
]
"""
Then a user logged in space s1 and calling getConcepts with these parameters: '{"scope":{"type":"nodes","nodeIds":["n1"]},"fields":{"concepts":["id","source_local_id","description","epistemic_status","literal_content","reference_content"]}}'
And query results should look like this
| _id | description | epistemic_status | @literal_content | @_reference_content |
| n1 | | unknown | {} | {} |

# The link to a schema is the exception: absent keeps it, and it is erased by
# declaring the concept to be a schema itself.
Scenario: Declaring a concept to be a schema erases its link to a schema
Given user user1 upserts these concepts to space s1:
"""
[
{
"name": "Evidence",
"author_local_id": "user1",
"source_local_id": "ns1",
"created": "2000/01/01",
"last_modified": "2001/01/02",
"is_schema": true
},
{
"name": "An Evidence",
"author_local_id": "user1",
"source_local_id": "n1",
"schema_represented_by_local_id": "ns1",
"created": "2000/01/03",
"last_modified": "2001/01/04"
}
]
"""
When user user1 upserts these concepts to space s1:
"""
[
{
"name": "An Evidence",
"source_local_id": "n1",
"created": "2000/01/03",
"last_modified": "2002/02/02",
"is_schema": true
}
]
"""
Then a user logged in space s1 and calling getConcepts with these parameters: '{"scope":{"schemas":true},"fields":{"concepts":["id","source_local_id","is_schema","schema_id","author_id"]}}'
And query results should look like this
| _id | @is_schema | _schema_id | _author_id |
| ns1 | true | | user1 |
| n1 | true | | user1 |
35 changes: 25 additions & 10 deletions packages/database/supabase/schemas/concept.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,9 @@ BEGIN
data.schema_represented_by_local_id, concept.space_id) INTO concept.schema_id;
END IF;
concept.source_local_id = COALESCE(concept.source_local_id, data.represented_by_local_id); -- legacy input field
concept.reference_content := coalesce(data.reference_content, '{}'::jsonb);
-- NULL means "the input said nothing about references"; upsert_concepts preserves
-- the stored value in that case. An explicit '{}' still clears the references.
concept.reference_content := data.reference_content;
IF data.local_reference_content IS NOT NULL THEN
FOR key, value IN SELECT * FROM jsonb_each(data.local_reference_content) LOOP
IF jsonb_typeof(value) = 'array' THEN
Expand All @@ -444,7 +446,7 @@ BEGIN
RAISE EXCEPTION 'Invalid value in local_reference_content % %', value, jsonb_typeof(value);
END IF;
END LOOP;
concept.reference_content := concept.reference_content || reference_content;
concept.reference_content := coalesce(concept.reference_content, '{}'::jsonb) || reference_content;
END IF;
RETURN concept;
END;
Expand Down Expand Up @@ -472,7 +474,10 @@ BEGIN
FOR concept_row IN SELECT * FROM jsonb_array_elements(data)
LOOP
-- first set defaults
local_concept := jsonb_populate_record(NULL::public.concept_local_input, '{"epistemic_status": "unknown", "literal_content":{},"reference_content":{},"is_schema":false}');
-- epistemic_status, literal_content and reference_content are deliberately left out of
-- the defaults: absent means "leave the stored value alone" on update, and only the
-- INSERT branch falls back to a default (see the ON CONFLICT clause below).
local_concept := jsonb_populate_record(NULL::public.concept_local_input, '{"is_schema":false}');

@devin-ai-integration devin-ai-integration Bot Aug 23, 2026

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.

🔴 Schema change ships without a migration

The edits to upsert_concepts live only in the declarative schema file. The documented workflow applies changes to the database through generated files in supabase/migrations/, and none was added. The new behavior never reaches the database and the new feature scenarios, which run against a migrated live database, fail.

Prompt for agents 
The declarative schema file packages/database/supabase/schemas/concept.sql was changed (upsert_concepts and _local_concept_to_db_concept) but no corresponding migration was committed under packages/database/supabase/migrations/. Per packages/database/README.md, schema edits must be turned into a migration via `pnpm run dbdiff:save <name>` and committed with `git add`, otherwise the local/CI/branch databases (which apply migrations, not the schema files) will not pick up the change and the new addConcepts.feature scenarios will fail. Generate and commit the migration file for this schema change.
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

-- then input values
local_concept := jsonb_populate_record(local_concept, concept_row);
local_concept.space_id := v_space_id;
Expand All @@ -489,19 +494,29 @@ BEGIN
INSERT INTO public."Concept" (
epistemic_status, name, description, author_id, created, last_modified, space_id, schema_id, literal_content, is_schema, source_local_id, reference_content
) VALUES (
db_concept.epistemic_status, db_concept.name, db_concept.description, db_concept.author_id, db_concept.created, db_concept.last_modified, db_concept.space_id, db_concept.schema_id, db_concept.literal_content, db_concept.is_schema, db_concept.source_local_id, db_concept.reference_content
coalesce(db_concept.epistemic_status, 'unknown'::public."EpistemicStatus"), db_concept.name, db_concept.description, db_concept.author_id, db_concept.created, db_concept.last_modified, db_concept.space_id, db_concept.schema_id, coalesce(db_concept.literal_content, '{}'::jsonb), db_concept.is_schema, db_concept.source_local_id, coalesce(db_concept.reference_content, '{}'::jsonb)
)
ON CONFLICT (space_id, source_local_id) DO UPDATE SET
epistemic_status = db_concept.epistemic_status,
-- absent (NULL) values leave the stored value alone, so that a client that knows
-- nothing of these fields does not erase what another client set.
epistemic_status = coalesce(db_concept.epistemic_status, "Concept".epistemic_status),
name = db_concept.name,
description = db_concept.description,
author_id = db_concept.author_id,
-- an empty string is how a client asks for the description to be cleared
description = CASE
WHEN db_concept.description = '' THEN NULL
ELSE coalesce(db_concept.description, "Concept".description) END,
-- a concept always has an author, so NULL here can only mean "not provided"
author_id = coalesce(db_concept.author_id, "Concept".author_id),
created = db_concept.created,
last_modified = db_concept.last_modified,
schema_id = db_concept.schema_id,
literal_content = db_concept.literal_content,
-- the link to a schema is only erased by declaring the concept to be a schema itself
schema_id = CASE
WHEN db_concept.is_schema THEN db_concept.schema_id
ELSE coalesce(db_concept.schema_id, "Concept".schema_id) END,
-- absent (NULL) content leaves the stored value alone; an explicit '{}' clears it
literal_content = coalesce(db_concept.literal_content, "Concept".literal_content),
is_schema = db_concept.is_schema,
reference_content = db_concept.reference_content
reference_content = coalesce(db_concept.reference_content, "Concept".reference_content)
-- If the syntax allowed two conflict clauses, I would add
-- ON CONFLICT (space_id, name) DO NOTHING
-- but since not, I have to handle it as an exception.
Expand Down
Loading