feature/knowledge-curation #91
@@ -601,12 +601,41 @@ impl KnowledgeDao for SqliteKnowledgeDao {
|
|||||||
trace_db_call(cx, "delete", "delete_entity", |_span| {
|
trace_db_call(cx, "delete", "delete_entity", |_span| {
|
||||||
use schema::entities::dsl::*;
|
use schema::entities::dsl::*;
|
||||||
let mut conn = self.connection.lock().expect("KnowledgeDao lock");
|
let mut conn = self.connection.lock().expect("KnowledgeDao lock");
|
||||||
diesel::delete(entities.filter(id.eq(entity_id)))
|
|
||||||
.execute(conn.deref_mut())
|
// entity_facts has a CHECK constraint requiring
|
||||||
.map(|_| ())
|
// `object_entity_id IS NOT NULL OR object_value IS NOT NULL`.
|
||||||
.map_err(|e| anyhow::anyhow!("Delete error: {}", e))
|
// The FK on object_entity_id is ON DELETE SET NULL — but
|
||||||
|
// facts that pointed at the deleted entity *only* via the
|
||||||
|
// entity reference (the common case for relational facts
|
||||||
|
// like "Alice is_friend_of Bob") have no object_value, so
|
||||||
|
// SET NULL would leave them with both NULLs and the CHECK
|
||||||
|
// aborts the whole DELETE. Pre-delete those facts in a
|
||||||
|
// transaction so the CASCADE / SET NULL chain on what
|
||||||
|
// remains can fire cleanly.
|
||||||
|
//
|
||||||
|
// Long-term fix is to change the FK to ON DELETE CASCADE
|
||||||
|
// via a table-rebuild migration, but the DAO-side workaround
|
||||||
|
// is sufficient and less invasive.
|
||||||
|
conn.transaction::<(), diesel::result::Error, _>(|conn| {
|
||||||
|
use schema::entity_facts::dsl as ef;
|
||||||
|
diesel::delete(
|
||||||
|
ef::entity_facts
|
||||||
|
.filter(ef::object_entity_id.eq(entity_id))
|
||||||
|
.filter(ef::object_value.is_null()),
|
||||||
|
)
|
||||||
|
.execute(conn)?;
|
||||||
|
|
||||||
|
diesel::delete(entities.filter(id.eq(entity_id))).execute(conn)?;
|
||||||
|
Ok(())
|
||||||
|
})
|
||||||
|
.map_err(|e| anyhow::anyhow!("Delete error: {}", e))
|
||||||
|
})
|
||||||
|
.map_err(|e| {
|
||||||
|
// Surface the actual diesel error string before collapsing
|
||||||
|
// to the opaque DbErrorKind::QueryError.
|
||||||
|
log::warn!("delete_entity({}) failed: {}", entity_id, e);
|
||||||
|
DbError::new(DbErrorKind::QueryError)
|
||||||
})
|
})
|
||||||
.map_err(|_| DbError::new(DbErrorKind::QueryError))
|
|
||||||
}
|
}
|
||||||
|
|
||||||
fn merge_entities(
|
fn merge_entities(
|
||||||
@@ -1334,6 +1363,77 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn delete_entity_clears_relational_facts_that_would_violate_check() {
|
||||||
|
// entity_facts has a CHECK that at least one of object_entity_id /
|
||||||
|
// object_value is non-null. The FK on object_entity_id is
|
||||||
|
// ON DELETE SET NULL, which would leave purely-relational facts
|
||||||
|
// (subject + predicate + object_entity_id, no object_value)
|
||||||
|
// with both nulls and abort the delete. The DAO pre-deletes
|
||||||
|
// those rows in a transaction so the parent delete can succeed.
|
||||||
|
let cx = opentelemetry::Context::new();
|
||||||
|
let conn = connection_with_fks_on();
|
||||||
|
let alice = create_user(&conn, "alice");
|
||||||
|
create_persona_row(&conn, alice, "default");
|
||||||
|
|
||||||
|
let mut dao = SqliteKnowledgeDao::from_connection(conn.clone());
|
||||||
|
let bob = make_entity(&mut dao, "Bob");
|
||||||
|
let carol = make_entity(&mut dao, "Carol");
|
||||||
|
|
||||||
|
// A relational fact where Carol is the object — exactly the
|
||||||
|
// shape the CHECK + SET NULL combination would otherwise break.
|
||||||
|
let (rel_fact, _) = dao
|
||||||
|
.upsert_fact(
|
||||||
|
&cx,
|
||||||
|
InsertEntityFact {
|
||||||
|
subject_entity_id: bob.id,
|
||||||
|
predicate: "is_friend_of".to_string(),
|
||||||
|
object_entity_id: Some(carol.id),
|
||||||
|
object_value: None,
|
||||||
|
source_photo: None,
|
||||||
|
source_insight_id: None,
|
||||||
|
confidence: 0.6,
|
||||||
|
status: "active".to_string(),
|
||||||
|
created_at: 0,
|
||||||
|
persona_id: "default".to_string(),
|
||||||
|
user_id: alice,
|
||||||
|
},
|
||||||
|
)
|
||||||
|
.unwrap();
|
||||||
|
|
||||||
|
// A typed fact where Bob is the subject — should survive.
|
||||||
|
add_fact(&mut dao, bob.id, "has_age", "30", alice, "default");
|
||||||
|
|
||||||
|
// Delete Carol — should succeed (relational fact pre-deleted).
|
||||||
|
dao.delete_entity(&cx, carol.id).unwrap();
|
||||||
|
|
||||||
|
assert!(
|
||||||
|
dao.get_entity_by_id(&cx, carol.id).unwrap().is_none(),
|
||||||
|
"Carol should be deleted"
|
||||||
|
);
|
||||||
|
// The relational fact about Carol should be gone (pre-deleted by
|
||||||
|
// the DAO's transaction, not SET NULL'd).
|
||||||
|
let bob_facts = dao
|
||||||
|
.get_facts_for_entity(
|
||||||
|
&cx,
|
||||||
|
bob.id,
|
||||||
|
&PersonaFilter::Single {
|
||||||
|
user_id: alice,
|
||||||
|
persona_id: "default".to_string(),
|
||||||
|
},
|
||||||
|
)
|
||||||
|
.unwrap();
|
||||||
|
assert!(
|
||||||
|
!bob_facts.iter().any(|f| f.id == rel_fact.id),
|
||||||
|
"relational fact pointing at Carol should be removed"
|
||||||
|
);
|
||||||
|
// The typed fact survives.
|
||||||
|
assert!(
|
||||||
|
bob_facts.iter().any(|f| f.predicate == "has_age"),
|
||||||
|
"typed fact about Bob should survive Carol's deletion"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn upsert_entity_collapses_near_duplicate_by_embedding() {
|
fn upsert_entity_collapses_near_duplicate_by_embedding() {
|
||||||
// The agent's pre-flight check uses FTS5 prefix tokens, which
|
// The agent's pre-flight check uses FTS5 prefix tokens, which
|
||||||
|
|||||||
Reference in New Issue
Block a user