From d9d749ac57efb8aee6d42d58034207f6d6edfd34 Mon Sep 17 00:00:00 2001 From: Taylor Eernisse Date: Mon, 26 Jan 2026 17:00:49 -0500 Subject: [PATCH] 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 - Add NormalizedDiscussion.merge_request_id: Option - 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 --- src/gitlab/transformers/discussion.rs | 69 ++++++++++++++++++++------- src/gitlab/transformers/mod.rs | 2 +- src/ingestion/discussions.rs | 14 ++++-- 3 files changed, 63 insertions(+), 22 deletions(-) diff --git a/src/gitlab/transformers/discussion.rs b/src/gitlab/transformers/discussion.rs index 0bb46a2..7ffe43f 100644 --- a/src/gitlab/transformers/discussion.rs +++ b/src/gitlab/transformers/discussion.rs @@ -5,13 +5,22 @@ use chrono::DateTime; use crate::core::time::now_ms; 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. #[derive(Debug, Clone)] pub struct NormalizedDiscussion { pub gitlab_discussion_id: String, pub project_id: i64, - pub issue_id: i64, - pub noteable_type: String, // "Issue" + pub issue_id: Option, + pub merge_request_id: Option, + pub noteable_type: String, // "Issue" or "MergeRequest" pub individual_note: bool, pub first_note_at: Option, // min(note.created_at) in ms epoch pub last_note_at: Option, // max(note.created_at) in ms epoch @@ -55,10 +64,16 @@ fn parse_timestamp(ts: &str) -> i64 { pub fn transform_discussion( gitlab_discussion: &GitLabDiscussion, local_project_id: i64, - local_issue_id: i64, + noteable: NoteableRef, ) -> NormalizedDiscussion { 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 let note_timestamps: Vec = gitlab_discussion .notes @@ -86,8 +101,9 @@ pub fn transform_discussion( NormalizedDiscussion { gitlab_discussion_id: gitlab_discussion.id.clone(), project_id: local_project_id, - issue_id: local_issue_id, - noteable_type: "Issue".to_string(), + issue_id, + merge_request_id, + noteable_type: noteable_type.to_string(), individual_note: gitlab_discussion.individual_note, first_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!( result.gitlab_discussion_id, "6a9c1750b37d513a43987b574953fceb50b03ce7" ); 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!(!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] fn extracts_notes_array_from_discussion() { let discussion = make_test_discussion( @@ -244,8 +281,8 @@ mod tests { )], ); - assert!(!transform_discussion(&threaded, 100, 42).individual_note); - assert!(transform_discussion(&standalone, 100, 42).individual_note); + assert!(!transform_discussion(&threaded, 100, NoteableRef::Issue(42)).individual_note); + assert!(transform_discussion(&standalone, 100, NoteableRef::Issue(42)).individual_note); } #[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) 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, Some(1705395600000)); @@ -338,8 +375,8 @@ mod tests { ], ); - assert!(!transform_discussion(¬_resolvable, 100, 42).resolvable); - assert!(transform_discussion(&resolvable, 100, 42).resolvable); + assert!(!transform_discussion(¬_resolvable, 100, NoteableRef::Issue(42)).resolvable); + assert!(transform_discussion(&resolvable, 100, NoteableRef::Issue(42)).resolvable); } #[test] @@ -374,8 +411,8 @@ mod tests { )], ); - assert!(!transform_discussion(&partial, 100, 42).resolved); - assert!(transform_discussion(&fully_resolved, 100, 42).resolved); - assert!(!transform_discussion(&no_resolvable, 100, 42).resolved); + assert!(!transform_discussion(&partial, 100, NoteableRef::Issue(42)).resolved); + assert!(transform_discussion(&fully_resolved, 100, NoteableRef::Issue(42)).resolved); + assert!(!transform_discussion(&no_resolvable, 100, NoteableRef::Issue(42)).resolved); } } diff --git a/src/gitlab/transformers/mod.rs b/src/gitlab/transformers/mod.rs index 5bbbb54..46ce550 100644 --- a/src/gitlab/transformers/mod.rs +++ b/src/gitlab/transformers/mod.rs @@ -3,5 +3,5 @@ pub mod discussion; 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}; diff --git a/src/ingestion/discussions.rs b/src/ingestion/discussions.rs index 62ab3d8..46b57b7 100644 --- a/src/ingestion/discussions.rs +++ b/src/ingestion/discussions.rs @@ -14,7 +14,7 @@ use crate::Config; use crate::core::error::Result; use crate::core::payloads::{StorePayloadOptions, store_payload}; 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; @@ -129,8 +129,11 @@ async fn ingest_discussions_for_issue( )?; // Transform and store discussion - let normalized = - transform_discussion(&gitlab_discussion, local_project_id, issue.local_issue_id); + let normalized = transform_discussion( + &gitlab_discussion, + local_project_id, + NoteableRef::Issue(issue.local_issue_id), + ); // Wrap all discussion+notes operations in a transaction for atomicity let tx = conn.unchecked_transaction()?; @@ -217,10 +220,10 @@ fn upsert_discussion( ) -> Result<()> { conn.execute( "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, 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 first_note_at = excluded.first_note_at, last_note_at = excluded.last_note_at, @@ -232,6 +235,7 @@ fn upsert_discussion( &discussion.gitlab_discussion_id, discussion.project_id, discussion.issue_id, + discussion.merge_request_id, &discussion.noteable_type, discussion.individual_note, discussion.first_note_at,