fix: When assigning alert to in cident by alert id, get all alerts wi…

…th same fingerprints (#2208)

Co-authored-by: Matvey Kukuy <[email protected]>

keep/api/core/db.py

@@ -1231,7 +1231,7 @@ def get_last_alerts(
         if with_incidents:
             query = query.add_columns(AlertToIncident.incident_id.label("incident"))
             query = query.outerjoin(
-                AlertToIncident, 
+                AlertToIncident,
                 and_(AlertToIncident.alert_id == Alert.id,  AlertToIncident.deleted_at == NULL_FOR_DELETED_AT),
             )
 
@@ -2900,8 +2900,27 @@ def get_future_incidents_by_incident_id(
     return query.all(), total_count
 
 
+def get_all_same_alert_ids(
+    tenant_id: str,
+    alert_ids: List[str | UUID],
+    session: Optional[Session] = None
+):
+    with existed_or_new_session(session) as session:
+        fingerprints_subquery = session.query(Alert.fingerprint).where(
+            Alert.tenant_id == tenant_id,
+            col(Alert.id).in_(alert_ids)
+        ).subquery()
+        query = session.scalars(
+            select(Alert.id)
+            .where(
+            Alert.tenant_id == tenant_id,
+                col(Alert.fingerprint).in_(fingerprints_subquery)
+        ))
+        return query.all()
+
+
 def get_alerts_data_for_incident(
-    alert_ids: list[str | UUID], session: Optional[Session] = None
+    alert_ids: List[str | UUID], session: Optional[Session] = None
 ) -> dict:
     """
     Function to prepare aggregated data for incidents from the given list of alert_ids
@@ -2983,21 +3002,24 @@ def add_alerts_to_incident(
     )
 
     with existed_or_new_session(session) as session:
+
         with session.no_autoflush:
+            all_alert_ids = get_all_same_alert_ids(tenant_id, alert_ids, session)
+
             # Use a set for faster membership checks
             existing_alert_ids = set(
                 session.exec(
                     select(AlertToIncident.alert_id).where(
                         AlertToIncident.deleted_at == NULL_FOR_DELETED_AT,
                         AlertToIncident.tenant_id == tenant_id,
                         AlertToIncident.incident_id == incident.id,
-                        col(AlertToIncident.alert_id).in_(alert_ids),
+                        col(AlertToIncident.alert_id).in_(all_alert_ids),
                     )
                 ).all()
             )
 
             new_alert_ids = [
-                alert_id for alert_id in alert_ids if alert_id not in existing_alert_ids
+                alert_id for alert_id in all_alert_ids if alert_id not in existing_alert_ids
             ]
 
             if not new_alert_ids:
@@ -3106,22 +3128,24 @@ def remove_alerts_to_incident_by_incident_id(
         if not incident:
             return None
 
+        all_alert_ids = get_all_same_alert_ids(tenant_id, alert_ids, session)
+
         # Removing alerts-to-incident relation for provided alerts_ids
         deleted = (
             session.query(AlertToIncident)
             .where(
                 AlertToIncident.deleted_at == NULL_FOR_DELETED_AT,
                 AlertToIncident.tenant_id == tenant_id,
                 AlertToIncident.incident_id == incident.id,
-                col(AlertToIncident.alert_id).in_(alert_ids),
+                col(AlertToIncident.alert_id).in_(all_alert_ids),
             ).update({
                 "deleted_at": datetime.now(datetime.now().astimezone().tzinfo),
             })
         )
         session.commit()
 
         # Getting aggregated data for incidents for alerts which just was removed
-        alerts_data_for_incident = get_alerts_data_for_incident(alert_ids, session)
+        alerts_data_for_incident = get_alerts_data_for_incident(all_alert_ids, session)
 
         service_field = get_json_extract_field(session, Alert.event, "service")
 

keep/api/models/db/migrations/versions/2024-10-14-08-34_83c1020be97d.py

@@ -28,22 +28,34 @@ def drop_and_restore_f_keys(table_name, conn):
 
     # Drop all foreign keys
     for fk in existing_f_keys:
-        op.drop_constraint(fk['name'], table_name, type_='foreignkey')
-        print(f"Dropped foreign key: {fk['name']}")
+        try:
+            op.drop_constraint(fk['name'], table_name, type_='foreignkey')
+            print(f"Dropped foreign key: {fk['name']}")
+        except NotImplementedError as e:
+            if "No support for ALTER of constraints in SQLite dialect." in str(e):
+                print("No support for ALTER of constraints in SQLite dialect, constraint should be overriden later so skipping")
+            else:
+                raise e
     try:
         yield
     finally:
         # Restore all foreign keys
         for fk in existing_f_keys:
-            op.create_foreign_key(
-                fk['name'],
-                table_name,
-                fk['referred_table'],
-                fk['constrained_columns'],
-                fk['referred_columns'],
-                ondelete=fk['options'].get('ondelete')
-            )
-            print(f"Restored foreign key: {fk['name']}")
+            try:
+                op.create_foreign_key(
+                    fk['name'],
+                    table_name,
+                    fk['referred_table'],
+                    fk['constrained_columns'],
+                    fk['referred_columns'],
+                    ondelete=fk['options'].get('ondelete')
+                )
+                print(f"Restored foreign key: {fk['name']}")
+            except NotImplementedError as e:
+                if "No support for ALTER of constraints in SQLite dialect." in str(e):
+                    print("No support for ALTER of constraints in SQLite dialect, constraint should be overriden later so skipping")
+                else:
+                    raise e
 
 
 def upgrade() -> None:
@@ -64,11 +76,16 @@ def upgrade() -> None:
     conn = op.get_bind()
 
     with drop_and_restore_f_keys("alerttoincident", conn):
-
-        with op.batch_alter_table("alerttoincident", schema=None) as batch_op:
-            inspector = inspect(conn)
-            existing_primary_key = inspector.get_pk_constraint('alerttoincident', schema=None)
-            batch_op.drop_constraint(existing_primary_key['name'], type_="primary")
+        try:
+            with op.batch_alter_table("alerttoincident", schema=None) as batch_op:
+                inspector = inspect(conn)
+                existing_primary_key = inspector.get_pk_constraint('alerttoincident', schema=None)
+                batch_op.drop_constraint(existing_primary_key['name'], type_="primary")
+        except ValueError as e:
+            if "Constraint must have a name" in str(e):
+                print("Constraint must have a name, constraint should be overriden later so skipping")
+            else:
+                raise e
 
         with op.batch_alter_table("alerttoincident", schema=None) as batch_op:
             batch_op.create_primary_key(

tests/test_incidents.py

@@ -372,3 +372,56 @@ def test_incident_metadata(db_session, client, test_app, setup_stress_alerts_no_
     assert data["services"] == ["keep", "keep-test", "keep-test-2"]
     assert "sources" in data
     assert data["sources"] == ["keep", "keep-test", "keep-test-2"]
+
+
+def test_add_alerts_with_same_fingerprint_to_incident(db_session, create_alert):
+    create_alert(
+        "fp1",
+        AlertStatus.FIRING,
+        datetime.utcnow(),
+        {"severity": AlertSeverity.CRITICAL.value},
+    )
+    create_alert(
+        f"fp1",
+        AlertStatus.FIRING,
+        datetime.utcnow(),
+        {"severity": AlertSeverity.CRITICAL.value},
+    )
+    create_alert(
+        f"fp2",
+        AlertStatus.FIRING,
+        datetime.utcnow(),
+        {"severity": AlertSeverity.CRITICAL.value},
+    )
+
+    db_alerts = db_session.query(Alert).all()
+
+    fp1_alerts = [alert for alert in db_alerts if alert.fingerprint == "fp1"]
+    fp2_alerts = [alert for alert in db_alerts if alert.fingerprint == "fp2"]
+
+    assert len(db_alerts) == 3
+    assert len(fp1_alerts) == 2
+    assert len(fp2_alerts) == 1
+
+    incident = create_incident_from_dict(
+        SINGLE_TENANT_UUID, {"user_generated_name": "test", "user_summary": "test"}
+    )
+
+    assert len(incident.alerts) == 0
+
+    add_alerts_to_incident_by_incident_id(
+        SINGLE_TENANT_UUID, incident.id, [fp1_alerts[0].id]
+    )
+
+    incident = get_incident_by_id(SINGLE_TENANT_UUID, incident.id)
+
+    assert len(incident.alerts) == 2
+
+    remove_alerts_to_incident_by_incident_id(
+        SINGLE_TENANT_UUID, incident.id, [fp1_alerts[0].id]
+    )
+
+    incident = get_incident_by_id(SINGLE_TENANT_UUID, incident.id)
+
+    assert len(incident.alerts) == 0
+