Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions src/firetower/incidents/migrations/0008_gapless_incident_ids.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
from django.db import migrations, models


def seed_counter(apps, schema_editor):
"""Seed the counter with MAX(id) + 1 or 2000 if no incidents exist."""
IncidentCounter = apps.get_model("incidents", "IncidentCounter")
Incident = apps.get_model("incidents", "Incident")

max_id = Incident.objects.aggregate(max_id=models.Max("id"))["max_id"]
next_id = (max_id + 1) if max_id is not None else 2000

IncidentCounter.objects.create(next_id=next_id)


def reverse_seed_counter(apps, schema_editor):
"""Remove the counter row."""
IncidentCounter = apps.get_model("incidents", "IncidentCounter")
IncidentCounter.objects.all().delete()


class Migration(migrations.Migration):
dependencies = [
("incidents", "0007_add_milestone_timestamps"),
]

operations = [
# Change id field from AutoField to PositiveIntegerField
migrations.AlterField(
model_name="incident",
name="id",
field=models.PositiveIntegerField(primary_key=True, serialize=False),
),
# Create the counter table
migrations.CreateModel(
name="IncidentCounter",
fields=[
(
"id",
models.BigAutoField(
auto_created=True,
primary_key=True,
serialize=False,
verbose_name="ID",
),
),
("next_id", models.PositiveIntegerField(default=2000)),
],
options={
"db_table": "incidents_incident_counter",
},
),
# Seed the counter with the appropriate starting value
migrations.RunPython(seed_counter, reverse_seed_counter),
]
35 changes: 30 additions & 5 deletions src/firetower/incidents/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,30 @@
from django.conf import settings
from django.contrib.auth.models import AbstractUser, User
from django.core.exceptions import ValidationError
from django.db import models
from django.db import models, transaction
from django.db.models import Q, QuerySet

INCIDENT_ID_START = 2000


class IncidentCounter(models.Model):
"""Stores the next available incident ID for gapless sequencing."""

next_id = models.PositiveIntegerField(default=INCIDENT_ID_START)

class Meta:
db_table = "incidents_incident_counter"


def get_next_incident_id() -> int:
"""Atomically get and increment the incident ID counter."""
with transaction.atomic():
counter = IncidentCounter.objects.select_for_update().get()
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The get_next_incident_id function will crash if the single row in the IncidentCounter table is missing or duplicated, as there are no safeguards.
Severity: HIGH

Suggested Fix

Strengthen the get_next_incident_id function to be resilient to a missing counter row. Use update_or_create or a similar pattern to ensure the counter row exists before attempting to read and increment it. This prevents crashes if the row is accidentally deleted.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: src/firetower/incidents/models.py#L24

Potential issue: The `get_next_incident_id` function retrieves the next incident ID by
calling `IncidentCounter.objects.select_for_update().get()`. This operation assumes
exactly one row exists in the `incidents_incident_counter` table. However, there are no
database-level constraints or application-level safeguards to prevent this row from
being accidentally deleted or duplicated through manual database operations. If the row
is missing, any attempt to create an incident will raise an unhandled
`IncidentCounter.DoesNotExist` exception, blocking all new incident creation. If the row
is duplicated, it will raise `MultipleObjectsReturned`.

Did we get this right? 👍 / 👎 to inform future reviews.

next_id = counter.next_id
counter.next_id += 1
counter.save()
return next_id


class IncidentStatus(models.TextChoices):
ACTIVE = "Active", "Active"
Expand Down Expand Up @@ -100,7 +121,8 @@ class Incident(models.Model):
"""

# Primary key - numeric ID (2000, 2001, etc.)
id = models.AutoField(primary_key=True)
# Uses IncidentCounter for gapless sequential IDs
id = models.PositiveIntegerField(primary_key=True)

# Core fields
title = models.CharField(max_length=500)
Expand Down Expand Up @@ -235,9 +257,12 @@ def clean(self) -> None:
raise ValidationError({"severity": "Severity must be set"})

def save(self, *args: Any, **kwargs: Any) -> None:
"""Override save to run validation"""
self.full_clean()
super().save(*args, **kwargs)
"""Override save to run validation and assign gapless ID"""
with transaction.atomic():
if self._state.adding and self.id is None:
self.id = get_next_incident_id()
self.full_clean()
super().save(*args, **kwargs)

def __str__(self) -> str:
return f"{self.incident_number}: {self.title}"
Expand Down
30 changes: 30 additions & 0 deletions src/firetower/incidents/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
ExternalLink,
ExternalLinkType,
Incident,
IncidentCounter,
IncidentSeverity,
IncidentStatus,
Tag,
Expand Down Expand Up @@ -47,6 +48,35 @@ def test_incident_id_starts_at_2000(self):
# Should be 2000 or higher (in case other tests ran first)
assert incident.id >= 2000

def test_incident_ids_are_gapless_after_failed_save(self):
"""Test that failed saves don't consume IDs (gapless sequence)"""
incident1 = Incident.objects.create(
title="First",
status=IncidentStatus.ACTIVE,
severity=IncidentSeverity.P1,
)
first_id = incident1.id

# Attempt to create an incident that will fail validation (empty title)
with pytest.raises(ValidationError):
Incident.objects.create(
title="", # Invalid - will fail validation
status=IncidentStatus.ACTIVE,
severity=IncidentSeverity.P1,
)

# The counter should NOT have incremented due to the failed save
counter = IncidentCounter.objects.get()
assert counter.next_id == first_id + 1

# Create another valid incident - should be exactly first_id + 1
incident2 = Incident.objects.create(
title="Second",
status=IncidentStatus.ACTIVE,
severity=IncidentSeverity.P1,
)
assert incident2.id == first_id + 1

def test_incident_number_property(self):
"""Test incident_number property returns correct format"""
incident = Incident.objects.create(
Expand Down
Loading