fix: critical data integrity — timeline dedup, discussion atomicity, index collision
Three correctness bugs found via peer code review: 1. TimelineEvent PartialEq/Ord omitted entity_type — issue #42 and MR #42 with the same timestamp and event_type were treated as equal. In a BTreeSet or dedup, one would silently be dropped. Added entity_type to both PartialEq and Ord comparisons. 2. discussions.rs: store_payload() was called outside the transaction (on bare conn) while upsert_discussion/notes were inside. A crash between them left orphaned payload rows. Moved store_payload inside the unchecked_transaction block, matching mr_discussions.rs pattern. 3. Migration 017 created idx_issue_assignees_username(username, issue_id) but migration 005 already created the same index name with just (username). SQLite's IF NOT EXISTS silently skipped the composite version on every existing database. New migration 018 drops and recreates the index with correct composite columns. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
10
migrations/018_fix_assignees_composite_index.sql
Normal file
10
migrations/018_fix_assignees_composite_index.sql
Normal file
@@ -0,0 +1,10 @@
|
|||||||
|
-- Migration 018: Fix composite index on issue_assignees
|
||||||
|
-- Migration 005 created idx_issue_assignees_username(username) as single-column.
|
||||||
|
-- Migration 017 attempted to recreate as (username, issue_id) but IF NOT EXISTS
|
||||||
|
-- silently skipped it. Drop and recreate with the correct composite columns.
|
||||||
|
|
||||||
|
DROP INDEX IF EXISTS idx_issue_assignees_username;
|
||||||
|
CREATE INDEX idx_issue_assignees_username ON issue_assignees(username, issue_id);
|
||||||
|
|
||||||
|
INSERT INTO schema_version (version, applied_at, description)
|
||||||
|
VALUES (18, strftime('%s', 'now') * 1000, 'Fix composite index on issue_assignees');
|
||||||
@@ -53,6 +53,10 @@ const MIGRATIONS: &[(&str, &str)] = &[
|
|||||||
include_str!("../../migrations/016_mr_file_changes.sql"),
|
include_str!("../../migrations/016_mr_file_changes.sql"),
|
||||||
),
|
),
|
||||||
("017", include_str!("../../migrations/017_who_indexes.sql")),
|
("017", include_str!("../../migrations/017_who_indexes.sql")),
|
||||||
|
(
|
||||||
|
"018",
|
||||||
|
include_str!("../../migrations/018_fix_assignees_composite_index.sql"),
|
||||||
|
),
|
||||||
];
|
];
|
||||||
|
|
||||||
pub fn create_connection(db_path: &Path) -> Result<Connection> {
|
pub fn create_connection(db_path: &Path) -> Result<Connection> {
|
||||||
|
|||||||
@@ -25,6 +25,7 @@ pub struct TimelineEvent {
|
|||||||
impl PartialEq for TimelineEvent {
|
impl PartialEq for TimelineEvent {
|
||||||
fn eq(&self, other: &Self) -> bool {
|
fn eq(&self, other: &Self) -> bool {
|
||||||
self.timestamp == other.timestamp
|
self.timestamp == other.timestamp
|
||||||
|
&& self.entity_type == other.entity_type
|
||||||
&& self.entity_id == other.entity_id
|
&& self.entity_id == other.entity_id
|
||||||
&& self.event_type == other.event_type
|
&& self.event_type == other.event_type
|
||||||
}
|
}
|
||||||
@@ -42,6 +43,7 @@ impl Ord for TimelineEvent {
|
|||||||
fn cmp(&self, other: &Self) -> Ordering {
|
fn cmp(&self, other: &Self) -> Ordering {
|
||||||
self.timestamp
|
self.timestamp
|
||||||
.cmp(&other.timestamp)
|
.cmp(&other.timestamp)
|
||||||
|
.then_with(|| self.entity_type.cmp(&other.entity_type))
|
||||||
.then_with(|| self.entity_id.cmp(&other.entity_id))
|
.then_with(|| self.entity_id.cmp(&other.entity_id))
|
||||||
.then_with(|| self.event_type.cmp(&other.event_type))
|
.then_with(|| self.event_type.cmp(&other.event_type))
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -96,16 +96,6 @@ async fn ingest_discussions_for_issue(
|
|||||||
result.discussions_fetched += 1;
|
result.discussions_fetched += 1;
|
||||||
|
|
||||||
let payload_bytes = serde_json::to_vec(&gitlab_discussion)?;
|
let payload_bytes = serde_json::to_vec(&gitlab_discussion)?;
|
||||||
let payload_id = store_payload(
|
|
||||||
conn,
|
|
||||||
StorePayloadOptions {
|
|
||||||
project_id: Some(local_project_id),
|
|
||||||
resource_type: "discussion",
|
|
||||||
gitlab_id: &gitlab_discussion.id,
|
|
||||||
json_bytes: &payload_bytes,
|
|
||||||
compress: config.storage.compress_raw_payloads,
|
|
||||||
},
|
|
||||||
)?;
|
|
||||||
|
|
||||||
let normalized = transform_discussion(
|
let normalized = transform_discussion(
|
||||||
&gitlab_discussion,
|
&gitlab_discussion,
|
||||||
@@ -115,6 +105,17 @@ async fn ingest_discussions_for_issue(
|
|||||||
|
|
||||||
let tx = conn.unchecked_transaction()?;
|
let tx = conn.unchecked_transaction()?;
|
||||||
|
|
||||||
|
let payload_id = store_payload(
|
||||||
|
&tx,
|
||||||
|
StorePayloadOptions {
|
||||||
|
project_id: Some(local_project_id),
|
||||||
|
resource_type: "discussion",
|
||||||
|
gitlab_id: &gitlab_discussion.id,
|
||||||
|
json_bytes: &payload_bytes,
|
||||||
|
compress: config.storage.compress_raw_payloads,
|
||||||
|
},
|
||||||
|
)?;
|
||||||
|
|
||||||
upsert_discussion(&tx, &normalized, payload_id)?;
|
upsert_discussion(&tx, &normalized, payload_id)?;
|
||||||
|
|
||||||
let local_discussion_id: i64 = tx.query_row(
|
let local_discussion_id: i64 = tx.query_row(
|
||||||
|
|||||||
Reference in New Issue
Block a user