diff --git a/api/changelog.d/findings-partition-max-age-unit.fixed.md b/api/changelog.d/findings-partition-max-age-unit.fixed.md new file mode 100644 index 0000000000..4b6cd02146 --- /dev/null +++ b/api/changelog.d/findings-partition-max-age-unit.fixed.md @@ -0,0 +1 @@ +`FINDINGS_TABLE_PARTITION_MAX_AGE_MONTHS` is now applied in months instead of days, and negative values are rejected diff --git a/api/src/backend/api/partitions.py b/api/src/backend/api/partitions.py index 8903c4504b..4d60b46b91 100644 --- a/api/src/backend/api/partitions.py +++ b/api/src/backend/api/partitions.py @@ -6,6 +6,7 @@ from api.rls import RowLevelSecurityConstraint from api.uuid_utils import datetime_to_uuid7 from dateutil.relativedelta import relativedelta from django.conf import settings +from django.core.exceptions import ImproperlyConfigured from psqlextra.partitioning import ( PostgresPartitioningError, PostgresPartitioningManager, @@ -153,10 +154,17 @@ class PostgresUUIDv7PartitioningStrategy(PostgresRangePartitioningStrategy): ) -def relative_days_or_none(value): - if value is None: +def relative_months_or_none(value): + # A negative value would set the cutoff in the future and delete every + # partition, so it is rejected rather than silently ignored. + if value is not None and value < 0: + raise ImproperlyConfigured( + "FINDINGS_TABLE_PARTITION_MAX_AGE_MONTHS must not be negative; " + "leave it unset or use 0 to keep partitions indefinitely" + ) + if not value: return None - return relativedelta(days=value) + return relativedelta(months=value) # @@ -173,7 +181,7 @@ manager = PostgresPartitioningManager( months=settings.FINDINGS_TABLE_PARTITION_MONTHS ), count=settings.FINDINGS_TABLE_PARTITION_COUNT, - max_age=relative_days_or_none( + max_age=relative_months_or_none( settings.FINDINGS_TABLE_PARTITION_MAX_AGE_MONTHS ), name_format="%Y_%b", @@ -189,7 +197,7 @@ manager = PostgresPartitioningManager( months=settings.FINDINGS_TABLE_PARTITION_MONTHS ), count=settings.FINDINGS_TABLE_PARTITION_COUNT, - max_age=relative_days_or_none( + max_age=relative_months_or_none( settings.FINDINGS_TABLE_PARTITION_MAX_AGE_MONTHS ), name_format="%Y_%b", diff --git a/api/src/backend/api/tests/test_partitions.py b/api/src/backend/api/tests/test_partitions.py new file mode 100644 index 0000000000..d8e0edbb9e --- /dev/null +++ b/api/src/backend/api/tests/test_partitions.py @@ -0,0 +1,63 @@ +from datetime import UTC, datetime +from itertools import islice + +import pytest +from api.partitions import ( + PostgresUUIDv7PartitioningStrategy, + relative_months_or_none, +) +from dateutil.relativedelta import relativedelta +from django.core.exceptions import ImproperlyConfigured +from psqlextra.partitioning import PostgresTimePartitionSize + + +def build_strategy(max_age): + return PostgresUUIDv7PartitioningStrategy( + size=PostgresTimePartitionSize(months=1), + count=1, + start_date=datetime.now(UTC), + max_age=max_age, + name_format="%Y_%b", + ) + + +class TestRelativeMonthsOrNone: + @pytest.mark.parametrize("value", [None, 0]) + def test_unset_or_zero_keeps_partitions_indefinitely(self, value): + assert relative_months_or_none(value) is None + + @pytest.mark.parametrize("months", [1, 3, 12]) + def test_value_is_interpreted_as_months(self, months): + assert relative_months_or_none(months) == relativedelta(months=months) + + def test_value_is_not_interpreted_as_days(self): + assert relative_months_or_none(12) != relativedelta(days=12) + + def test_negative_is_rejected(self): + with pytest.raises(ImproperlyConfigured): + relative_months_or_none(-12) + + +class TestToDelete: + @pytest.mark.parametrize("max_age", [None, relative_months_or_none(0)]) + def test_nothing_is_deleted_without_max_age(self, max_age): + strategy = build_strategy(max_age) + + assert list(islice(strategy.to_delete(), 5)) == [] + + def test_first_deleted_partition_is_max_age_old(self): + months = 3 + strategy = build_strategy(relative_months_or_none(months)) + + first = next(strategy.to_delete()) + + expected = strategy.get_start_datetime() - relativedelta(months=months) + assert first.name() == expected.strftime("%Y_%b").lower() + + def test_deleted_partitions_go_further_back_in_time(self): + strategy = build_strategy(relative_months_or_none(3)) + + names = [p.name() for p in islice(strategy.to_delete(), 3)] + starts = [datetime.strptime(n, "%Y_%b") for n in names] + + assert starts == sorted(starts, reverse=True)