fix(discussion): Make NormalizedDiscussion polymorphic for MR support
This is a P0 fix from the CP1-CP2 alignment audit. The original NormalizedDiscussion struct had issue_id as a non-optional i64 and hardcoded noteable_type to "Issue", making it incompatible with merge request discussions even though the database schema already supports both via nullable columns and a CHECK constraint. Changes: - Add NoteableRef enum with Issue(i64) and MergeRequest(i64) variants to provide compile-time safety against mixing up issue vs MR IDs - Change NormalizedDiscussion.issue_id from i64 to Option<i64> - Add NormalizedDiscussion.merge_request_id: Option<i64> - Update transform_discussion() signature to take NoteableRef instead of local_issue_id, deriving issue_id/merge_request_id/noteable_type from the enum variant - Update upsert_discussion() SQL to include merge_request_id column (now 12 parameters instead of 11) - Export NoteableRef from transformers module - Add test for MergeRequest discussion transformation - Update all existing tests to use NoteableRef::Issue(id) The database schema (migration 002) was forward-thinking and already supports both issue_id and merge_request_id as nullable columns with a CHECK constraint. This change prepares the application layer for CP2 merge request support without requiring any migrations. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -5,13 +5,22 @@ use chrono::DateTime;
|
|||||||
use crate::core::time::now_ms;
|
use crate::core::time::now_ms;
|
||||||
use crate::gitlab::types::{GitLabDiscussion, GitLabNote};
|
use crate::gitlab::types::{GitLabDiscussion, GitLabNote};
|
||||||
|
|
||||||
|
/// Reference to the parent noteable (Issue or MergeRequest).
|
||||||
|
/// Uses an enum to prevent accidentally mixing up issue vs MR IDs at compile time.
|
||||||
|
#[derive(Debug, Clone, Copy)]
|
||||||
|
pub enum NoteableRef {
|
||||||
|
Issue(i64),
|
||||||
|
MergeRequest(i64),
|
||||||
|
}
|
||||||
|
|
||||||
/// Normalized discussion for local storage.
|
/// Normalized discussion for local storage.
|
||||||
#[derive(Debug, Clone)]
|
#[derive(Debug, Clone)]
|
||||||
pub struct NormalizedDiscussion {
|
pub struct NormalizedDiscussion {
|
||||||
pub gitlab_discussion_id: String,
|
pub gitlab_discussion_id: String,
|
||||||
pub project_id: i64,
|
pub project_id: i64,
|
||||||
pub issue_id: i64,
|
pub issue_id: Option<i64>,
|
||||||
pub noteable_type: String, // "Issue"
|
pub merge_request_id: Option<i64>,
|
||||||
|
pub noteable_type: String, // "Issue" or "MergeRequest"
|
||||||
pub individual_note: bool,
|
pub individual_note: bool,
|
||||||
pub first_note_at: Option<i64>, // min(note.created_at) in ms epoch
|
pub first_note_at: Option<i64>, // min(note.created_at) in ms epoch
|
||||||
pub last_note_at: Option<i64>, // max(note.created_at) in ms epoch
|
pub last_note_at: Option<i64>, // max(note.created_at) in ms epoch
|
||||||
@@ -55,10 +64,16 @@ fn parse_timestamp(ts: &str) -> i64 {
|
|||||||
pub fn transform_discussion(
|
pub fn transform_discussion(
|
||||||
gitlab_discussion: &GitLabDiscussion,
|
gitlab_discussion: &GitLabDiscussion,
|
||||||
local_project_id: i64,
|
local_project_id: i64,
|
||||||
local_issue_id: i64,
|
noteable: NoteableRef,
|
||||||
) -> NormalizedDiscussion {
|
) -> NormalizedDiscussion {
|
||||||
let now = now_ms();
|
let now = now_ms();
|
||||||
|
|
||||||
|
// Derive issue_id, merge_request_id, and noteable_type from the enum
|
||||||
|
let (issue_id, merge_request_id, noteable_type) = match noteable {
|
||||||
|
NoteableRef::Issue(id) => (Some(id), None, "Issue"),
|
||||||
|
NoteableRef::MergeRequest(id) => (None, Some(id), "MergeRequest"),
|
||||||
|
};
|
||||||
|
|
||||||
// Compute first_note_at and last_note_at from notes
|
// Compute first_note_at and last_note_at from notes
|
||||||
let note_timestamps: Vec<i64> = gitlab_discussion
|
let note_timestamps: Vec<i64> = gitlab_discussion
|
||||||
.notes
|
.notes
|
||||||
@@ -86,8 +101,9 @@ pub fn transform_discussion(
|
|||||||
NormalizedDiscussion {
|
NormalizedDiscussion {
|
||||||
gitlab_discussion_id: gitlab_discussion.id.clone(),
|
gitlab_discussion_id: gitlab_discussion.id.clone(),
|
||||||
project_id: local_project_id,
|
project_id: local_project_id,
|
||||||
issue_id: local_issue_id,
|
issue_id,
|
||||||
noteable_type: "Issue".to_string(),
|
merge_request_id,
|
||||||
|
noteable_type: noteable_type.to_string(),
|
||||||
individual_note: gitlab_discussion.individual_note,
|
individual_note: gitlab_discussion.individual_note,
|
||||||
first_note_at,
|
first_note_at,
|
||||||
last_note_at,
|
last_note_at,
|
||||||
@@ -192,18 +208,39 @@ mod tests {
|
|||||||
)],
|
)],
|
||||||
);
|
);
|
||||||
|
|
||||||
let result = transform_discussion(&discussion, 100, 42);
|
let result = transform_discussion(&discussion, 100, NoteableRef::Issue(42));
|
||||||
|
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
result.gitlab_discussion_id,
|
result.gitlab_discussion_id,
|
||||||
"6a9c1750b37d513a43987b574953fceb50b03ce7"
|
"6a9c1750b37d513a43987b574953fceb50b03ce7"
|
||||||
);
|
);
|
||||||
assert_eq!(result.project_id, 100);
|
assert_eq!(result.project_id, 100);
|
||||||
assert_eq!(result.issue_id, 42);
|
assert_eq!(result.issue_id, Some(42));
|
||||||
|
assert_eq!(result.merge_request_id, None);
|
||||||
assert_eq!(result.noteable_type, "Issue");
|
assert_eq!(result.noteable_type, "Issue");
|
||||||
assert!(!result.individual_note);
|
assert!(!result.individual_note);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn transforms_merge_request_discussion() {
|
||||||
|
let discussion = make_test_discussion(
|
||||||
|
false,
|
||||||
|
vec![make_test_note(
|
||||||
|
1,
|
||||||
|
"2024-01-16T09:00:00.000Z",
|
||||||
|
false,
|
||||||
|
false,
|
||||||
|
false,
|
||||||
|
)],
|
||||||
|
);
|
||||||
|
|
||||||
|
let result = transform_discussion(&discussion, 100, NoteableRef::MergeRequest(99));
|
||||||
|
|
||||||
|
assert_eq!(result.issue_id, None);
|
||||||
|
assert_eq!(result.merge_request_id, Some(99));
|
||||||
|
assert_eq!(result.noteable_type, "MergeRequest");
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn extracts_notes_array_from_discussion() {
|
fn extracts_notes_array_from_discussion() {
|
||||||
let discussion = make_test_discussion(
|
let discussion = make_test_discussion(
|
||||||
@@ -244,8 +281,8 @@ mod tests {
|
|||||||
)],
|
)],
|
||||||
);
|
);
|
||||||
|
|
||||||
assert!(!transform_discussion(&threaded, 100, 42).individual_note);
|
assert!(!transform_discussion(&threaded, 100, NoteableRef::Issue(42)).individual_note);
|
||||||
assert!(transform_discussion(&standalone, 100, 42).individual_note);
|
assert!(transform_discussion(&standalone, 100, NoteableRef::Issue(42)).individual_note);
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
@@ -293,7 +330,7 @@ mod tests {
|
|||||||
],
|
],
|
||||||
);
|
);
|
||||||
|
|
||||||
let result = transform_discussion(&discussion, 100, 42);
|
let result = transform_discussion(&discussion, 100, NoteableRef::Issue(42));
|
||||||
|
|
||||||
// first_note_at should be 09:00 (note 1)
|
// first_note_at should be 09:00 (note 1)
|
||||||
assert_eq!(result.first_note_at, Some(1705395600000));
|
assert_eq!(result.first_note_at, Some(1705395600000));
|
||||||
@@ -314,7 +351,7 @@ mod tests {
|
|||||||
)],
|
)],
|
||||||
);
|
);
|
||||||
|
|
||||||
let result = transform_discussion(&discussion, 100, 42);
|
let result = transform_discussion(&discussion, 100, NoteableRef::Issue(42));
|
||||||
|
|
||||||
assert_eq!(result.first_note_at, result.last_note_at);
|
assert_eq!(result.first_note_at, result.last_note_at);
|
||||||
assert_eq!(result.first_note_at, Some(1705395600000));
|
assert_eq!(result.first_note_at, Some(1705395600000));
|
||||||
@@ -338,8 +375,8 @@ mod tests {
|
|||||||
],
|
],
|
||||||
);
|
);
|
||||||
|
|
||||||
assert!(!transform_discussion(¬_resolvable, 100, 42).resolvable);
|
assert!(!transform_discussion(¬_resolvable, 100, NoteableRef::Issue(42)).resolvable);
|
||||||
assert!(transform_discussion(&resolvable, 100, 42).resolvable);
|
assert!(transform_discussion(&resolvable, 100, NoteableRef::Issue(42)).resolvable);
|
||||||
}
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
@@ -374,8 +411,8 @@ mod tests {
|
|||||||
)],
|
)],
|
||||||
);
|
);
|
||||||
|
|
||||||
assert!(!transform_discussion(&partial, 100, 42).resolved);
|
assert!(!transform_discussion(&partial, 100, NoteableRef::Issue(42)).resolved);
|
||||||
assert!(transform_discussion(&fully_resolved, 100, 42).resolved);
|
assert!(transform_discussion(&fully_resolved, 100, NoteableRef::Issue(42)).resolved);
|
||||||
assert!(!transform_discussion(&no_resolvable, 100, 42).resolved);
|
assert!(!transform_discussion(&no_resolvable, 100, NoteableRef::Issue(42)).resolved);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -3,5 +3,5 @@
|
|||||||
pub mod discussion;
|
pub mod discussion;
|
||||||
pub mod issue;
|
pub mod issue;
|
||||||
|
|
||||||
pub use discussion::{NormalizedDiscussion, NormalizedNote, transform_discussion, transform_notes};
|
pub use discussion::{NormalizedDiscussion, NormalizedNote, NoteableRef, transform_discussion, transform_notes};
|
||||||
pub use issue::{IssueRow, IssueWithMetadata, MilestoneRow, transform_issue};
|
pub use issue::{IssueRow, IssueWithMetadata, MilestoneRow, transform_issue};
|
||||||
|
|||||||
@@ -14,7 +14,7 @@ use crate::Config;
|
|||||||
use crate::core::error::Result;
|
use crate::core::error::Result;
|
||||||
use crate::core::payloads::{StorePayloadOptions, store_payload};
|
use crate::core::payloads::{StorePayloadOptions, store_payload};
|
||||||
use crate::gitlab::GitLabClient;
|
use crate::gitlab::GitLabClient;
|
||||||
use crate::gitlab::transformers::{transform_discussion, transform_notes};
|
use crate::gitlab::transformers::{NoteableRef, transform_discussion, transform_notes};
|
||||||
|
|
||||||
use super::issues::IssueForDiscussionSync;
|
use super::issues::IssueForDiscussionSync;
|
||||||
|
|
||||||
@@ -129,8 +129,11 @@ async fn ingest_discussions_for_issue(
|
|||||||
)?;
|
)?;
|
||||||
|
|
||||||
// Transform and store discussion
|
// Transform and store discussion
|
||||||
let normalized =
|
let normalized = transform_discussion(
|
||||||
transform_discussion(&gitlab_discussion, local_project_id, issue.local_issue_id);
|
&gitlab_discussion,
|
||||||
|
local_project_id,
|
||||||
|
NoteableRef::Issue(issue.local_issue_id),
|
||||||
|
);
|
||||||
|
|
||||||
// Wrap all discussion+notes operations in a transaction for atomicity
|
// Wrap all discussion+notes operations in a transaction for atomicity
|
||||||
let tx = conn.unchecked_transaction()?;
|
let tx = conn.unchecked_transaction()?;
|
||||||
@@ -217,10 +220,10 @@ fn upsert_discussion(
|
|||||||
) -> Result<()> {
|
) -> Result<()> {
|
||||||
conn.execute(
|
conn.execute(
|
||||||
"INSERT INTO discussions (
|
"INSERT INTO discussions (
|
||||||
gitlab_discussion_id, project_id, issue_id, noteable_type,
|
gitlab_discussion_id, project_id, issue_id, merge_request_id, noteable_type,
|
||||||
individual_note, first_note_at, last_note_at, last_seen_at,
|
individual_note, first_note_at, last_note_at, last_seen_at,
|
||||||
resolvable, resolved, raw_payload_id
|
resolvable, resolved, raw_payload_id
|
||||||
) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11)
|
) VALUES (?1, ?2, ?3, ?4, ?5, ?6, ?7, ?8, ?9, ?10, ?11, ?12)
|
||||||
ON CONFLICT(project_id, gitlab_discussion_id) DO UPDATE SET
|
ON CONFLICT(project_id, gitlab_discussion_id) DO UPDATE SET
|
||||||
first_note_at = excluded.first_note_at,
|
first_note_at = excluded.first_note_at,
|
||||||
last_note_at = excluded.last_note_at,
|
last_note_at = excluded.last_note_at,
|
||||||
@@ -232,6 +235,7 @@ fn upsert_discussion(
|
|||||||
&discussion.gitlab_discussion_id,
|
&discussion.gitlab_discussion_id,
|
||||||
discussion.project_id,
|
discussion.project_id,
|
||||||
discussion.issue_id,
|
discussion.issue_id,
|
||||||
|
discussion.merge_request_id,
|
||||||
&discussion.noteable_type,
|
&discussion.noteable_type,
|
||||||
discussion.individual_note,
|
discussion.individual_note,
|
||||||
discussion.first_note_at,
|
discussion.first_note_at,
|
||||||
|
|||||||
Reference in New Issue
Block a user