Auto-repair missing IntegrationConfiguration rows when IntegrationType gains actions#406
Merged
Merged
Conversation
Adds two layers that backfill IntegrationConfiguration rows when an IntegrationType gains an action after Integrations of that type already exist. Both reuse the existing idempotent Integration.create_missing_configurations(). - post_save signal on IntegrationAction (gated on created=True, fires via transaction.on_commit) automatically backfills every Integration of the action's type. - New management command repair_integration_configurations with --integration, --integration-type, and --dry-run flags for ad-hoc recovery. - Tests cover signal auto-fill (incl. periodic action -> PeriodicTask), no-op on action edits, and the management command's three modes plus idempotency. - New opt-in pytest docker-compose service (profile "test") for running the suite against real Postgres without a host install. Refs GUNDI-5297 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds an automatic repair path for v2 integrations that are missing IntegrationConfiguration rows after new IntegrationActions are added, plus a manual management command and test/dev tooling to exercise that behavior. It fits into the integrations subsystem by extending the existing Integration.create_missing_configurations() backfill flow to both ongoing writes and ad-hoc recovery.
Changes:
- Add a
post_savesignal onIntegrationActionto backfill configs for existing integrations of the same type. - Add a
repair_integration_configurationsmanagement command with--integration,--integration-type, and--dry-run. - Add tests for the new signal/command behavior and an opt-in
pytestdocker-compose service.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
docker-compose.yml |
Adds an opt-in pytest service for running the suite in Docker against local Postgres/Redis. |
cdip_admin/integrations/tests/test_signals.py |
Adds coverage for signal-driven backfill and periodic-task creation. |
cdip_admin/integrations/tests/test_commands.py |
Adds coverage for the repair management command’s happy path, dry-run, and idempotency. |
cdip_admin/integrations/signals.py |
Introduces the new IntegrationAction post-save backfill signal. |
cdip_admin/integrations/management/commands/repair_integration_configurations.py |
Adds the ad-hoc repair command for missing integration configurations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+34
to
+46
| qs = Integration.objects.all().select_related("type") | ||
| if integration_id: | ||
| qs = qs.filter(id=integration_id) | ||
| if not qs.exists(): | ||
| self.stderr.write(f"Integration '{integration_id}' not found.") | ||
| return | ||
| elif integration_type_slug: | ||
| try: | ||
| integration_type = IntegrationType.objects.get(value=integration_type_slug) | ||
| except IntegrationType.DoesNotExist: | ||
| self.stderr.write(f"Integration type '{integration_type_slug}' not found.") | ||
| return | ||
| qs = qs.filter(type=integration_type) |
Comment on lines
+28
to
+32
|
|
||
| def _backfill(): | ||
| integrations = Integration.objects.filter(type_id=integration_type_id) | ||
| for integration in integrations.iterator(): | ||
| integration.create_missing_configurations() |
Comment on lines
+66
to
+67
| if not dry_run: | ||
| integration.create_missing_configurations() |
Comment on lines
+29
to
+34
| def _backfill(): | ||
| integrations = Integration.objects.filter(type_id=integration_type_id) | ||
| for integration in integrations.iterator(): | ||
| integration.create_missing_configurations() | ||
|
|
||
| transaction.on_commit(_backfill) |
- repair_integration_configurations now refuses to run without --integration, --integration-type, or an explicit --all flag. Prevents accidental whole-database mutation from a stray invocation. - Add UniqueConstraint on IntegrationConfiguration(integration, action) with a dedup data migration. Closes the exists+create race in Integration.create_missing_configurations() that could insert duplicate config rows under concurrent backfill (signal + repair command running together). - Switch create_missing_configurations() to get_or_create so the new constraint guards against any remaining race window. - Move the post_save signal's backfill out of the on_commit callback into a Celery task (backfill_action_configurations_for_type) so IntegrationAction creation doesn't block on an O(integrations × actions) loop in the request thread. - Update tests for the new selector requirement and async dispatch; add coverage for the no-selector CommandError and --all flag. Refs GUNDI-5297 Addresses PR #406 review comments (Copilot) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressed all four Copilot review comments in 7977643. Mapping:
Test updates:
|
Contributor
Author
|
Addressed all four Copilot review comments in 7977643. Mapping:
Test updates: |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+16
to
+22
| for dup in duplicates: | ||
| rows = IntegrationConfiguration.objects.filter( | ||
| integration_id=dup["integration_id"], | ||
| action_id=dup["action_id"], | ||
| ).order_by("created_at", "id") | ||
| keep = rows.first() | ||
| rows.exclude(pk=keep.pk).delete() |
Comment on lines
+4
to
+22
| def dedupe_integration_configurations(apps, schema_editor): | ||
| # Defensively remove any existing duplicate (integration, action) pairs | ||
| # before adding the unique constraint. The previous create_missing_configurations | ||
| # had a non-atomic exists+create that could race under concurrent calls, | ||
| # so production may have stray duplicates. Keep the oldest row (smallest | ||
| # created_at) since downstream PeriodicTask references hang off it. | ||
| IntegrationConfiguration = apps.get_model("integrations", "IntegrationConfiguration") | ||
| duplicates = ( | ||
| IntegrationConfiguration.objects.values("integration_id", "action_id") | ||
| .annotate(row_count=models.Count("id")) | ||
| .filter(row_count__gt=1) | ||
| ) | ||
| for dup in duplicates: | ||
| rows = IntegrationConfiguration.objects.filter( | ||
| integration_id=dup["integration_id"], | ||
| action_id=dup["action_id"], | ||
| ).order_by("created_at", "id") | ||
| keep = rows.first() | ||
| rows.exclude(pk=keep.pk).delete() |
| except Exception as e: | ||
| logger.exception( | ||
| f"Error backfilling configurations for integration {integration.id}: {e}" | ||
| ) |
Comment on lines
+54
to
+61
| self.stderr.write(f"Integration '{integration_id}' not found.") | ||
| return | ||
| elif integration_type_slug: | ||
| try: | ||
| integration_type = IntegrationType.objects.get(value=integration_type_slug) | ||
| except IntegrationType.DoesNotExist: | ||
| self.stderr.write(f"Integration type '{integration_type_slug}' not found.") | ||
| return |
- Migration 0115 now manually deletes attached PeriodicTask rows before removing duplicate IntegrationConfiguration rows. Historical-model delete bypasses the pre_delete signal that normally cleans them up, so periodic beat jobs would otherwise be orphaned. - Migration 0115 winner-selection no longer keeps the oldest row unconditionally — picks by (has data, has periodic_task, latest updated_at) so a meaningful payload isn't replaced with an empty duplicate. - backfill_action_configurations_for_type Celery task no longer wraps each integration in try/except. Errors propagate so autoretry_for fires; create_missing_configurations() is idempotent against the new unique constraint, so whole-task retry is safe. - repair_integration_configurations now raises CommandError (non-zero exit) when the requested integration or type doesn't exist, instead of writing to stderr and returning 0. - Tests cover the new CommandError paths. Refs GUNDI-5297 Addresses second-round PR #406 review comments (Copilot) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressed all four second-round Copilot review comments in 8744720. Mapping:
Test additions: |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+34
to
+49
| def winner_score(row): | ||
| return ( | ||
| bool(row.data), # has meaningful payload | ||
| row.periodic_task_id is not None, # has scheduled task | ||
| row.updated_at, # most recent edit | ||
| ) | ||
|
|
||
| winner = max(rows, key=winner_score) | ||
| for row in rows: | ||
| if row.pk == winner.pk: | ||
| continue | ||
| if row.periodic_task_id: | ||
| # pre_delete on the historical IntegrationConfiguration is not | ||
| # wired up, so clean its PeriodicTask explicitly. | ||
| PeriodicTask.objects.filter(pk=row.periodic_task_id).delete() | ||
| row.delete() |
Contributor
There was a problem hiding this comment.
We could run a query to check if that scenario even happens for any integration, probably not
Contributor
Author
There was a problem hiding this comment.
Good call — agree it's likely defensive only. The donor-task path costs nothing when there are no duplicates (the outer duplicate_keys loop is empty), so I'd keep the branch as correctness insurance even if real data shows zero hits.
I'll run this against staging before deploy:
from integrations.models import IntegrationConfiguration
from django.db.models import Count
dups = (IntegrationConfiguration.objects
.values("integration_id", "action_id")
.annotate(n=Count("id")).filter(n__gt=1))
for d in dups:
rows = list(IntegrationConfiguration.objects.filter(
integration_id=d["integration_id"], action_id=d["action_id"]))
print(d, [(bool(r.data), bool(r.periodic_task_id)) for r in rows])If the count is zero, no surprises during the migration. If non-zero, the result tuples tell us whether any (data, periodic_task) splits exist that would exercise the donor-task transfer.
Comment on lines
+433
to
+434
| integrations = Integration.objects.filter(type_id=integration_type_id) | ||
| for integration in integrations.iterator(): |
Comment on lines
+66
to
+73
| for integration in qs.iterator(): | ||
| existing_action_ids = set( | ||
| integration.configurations.values_list("action_id", flat=True) | ||
| ) | ||
| missing = [ | ||
| action | ||
| for action in integration.type.actions.all() | ||
| if action.id not in existing_action_ids |
Comment on lines
+44
to
+62
| if not (integration_id or integration_type_slug or repair_all): | ||
| raise CommandError( | ||
| "Refusing to run without a target. Pass --integration <uuid>, " | ||
| "--integration-type <slug>, or --all to repair every integration." | ||
| ) | ||
|
|
||
| qs = Integration.objects.all().select_related("type") | ||
| if integration_id: | ||
| qs = qs.filter(id=integration_id) | ||
| if not qs.exists(): | ||
| raise CommandError(f"Integration '{integration_id}' not found.") | ||
| elif integration_type_slug: | ||
| try: | ||
| integration_type = IntegrationType.objects.get(value=integration_type_slug) | ||
| except IntegrationType.DoesNotExist: | ||
| raise CommandError( | ||
| f"Integration type '{integration_type_slug}' not found." | ||
| ) | ||
| qs = qs.filter(type=integration_type) |
- Migration 0115 now transfers a duplicate-row's PeriodicTask to the winner before deleting it, so a periodic action keeps its scheduler even when the winner row was selected on data and the only attached schedule lived on a loser row. - Hoist type.actions query out of the per-integration loop in both the Celery backfill task and the repair management command. Added an optional `actions=` arg to Integration.create_missing_configurations so callers iterating many integrations of the same type can pass a cached action list and skip the redundant re-query. - repair_integration_configurations now rejects conflicting selectors (more than one of --integration, --integration-type, --all). Silently letting --integration win could mask a mistyped --integration-type. - Tests cover the two combined-selector error paths. Refs GUNDI-5297 Addresses third-round PR #406 review comments (Copilot) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Addressed all four third-round Copilot review comments in 27b9202. Mapping:
Also added the Tests: |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+441
to
+446
| constraints = [ | ||
| models.UniqueConstraint( | ||
| fields=["integration", "action"], | ||
| name="unique_integration_action_configuration", | ||
| ), | ||
| ] |
Contributor
Author
There was a problem hiding this comment.
@marianobrc I'm interested to know what you think of this PR and the change to the constraints on IntegrationConfiguration.
Contributor
There was a problem hiding this comment.
I think it's ok, I don't know any genuine use case requiring multiple configs for the same [integration, action]. The validation in the API would be nice to have, but our only client now is our Portal, and that shouldn't happen.
marianobrc
reviewed
May 6, 2026
| Integration = apps.get_model("integrations", "Integration") | ||
| integration_type = IntegrationType.objects.filter(id=integration_type_id).first() | ||
| if integration_type is None: | ||
| return |
Contributor
There was a problem hiding this comment.
Maybe we could log a warning here, so that we know the task is running, and why it isn't creating the configs
marianobrc
reviewed
May 6, 2026
| config.periodic_task.delete() | ||
|
|
||
|
|
||
| @receiver(post_save, sender=IntegrationAction) |
Contributor
There was a problem hiding this comment.
non-blocking: An alternative to signals would be to move this to Model.save() -> Model._post_save() as we do in other models v2 in the project (the pre_delete signal usage was an exception due to technical limitations). This is an opinionated preference because having this code in the model is more visible, easier to find, and makes it clear that this post-save logic exists, whereas a signals file is easier to miss. Otoh, a comment somewhere in the model to highlight the existence of the post_save signal receiver may solve it too.
marianobrc
reviewed
May 6, 2026
Comment on lines
+43
to
+48
| # If the winner has no PeriodicTask but a loser does, that's the only | ||
| # live schedule for this (integration, action) pair — move it onto the | ||
| # winner before the delete loop runs, so we don't drop a periodic | ||
| # action's scheduler. The OneToOne constraint requires we null the | ||
| # donor's FK first. | ||
| if not winner.periodic_task_id: |
marianobrc
reviewed
May 6, 2026
| IntegrationConfiguration.objects.get_or_create( | ||
| integration=self, | ||
| action=action, | ||
| defaults={"data": {}}, |
Contributor
There was a problem hiding this comment.
Idea: Get defaults from the json schema stored in IntegrationAction, if available.
Contributor
Author
There was a problem hiding this comment.
Good idea, I'll make a follow-up task for this ide.
marianobrc
approved these changes
May 6, 2026
Contributor
marianobrc
left a comment
marianobrc
left a comment
There was a problem hiding this comment.
LGTM. Looking great @chrisdoehring. I only left a few ideas and non-blocking comments.
- Move IntegrationAction post-save backfill dispatch from signals.py into IntegrationAction._post_save (with custom save() and FieldTracker) to match the v2 model lifecycle convention used by Integration and IntegrationConfiguration. The pre_delete handler for IntegrationConfiguration stays in signals.py (per reviewer's note that it's the documented exception). - backfill_action_configurations_for_type now logs at info on normal start and logs a warning when the IntegrationType has been deleted between signal dispatch and worker pickup, so operators can correlate skipped backfills with a log line. - Tests: mock paths updated to the new dispatch location (integrations.models.v2.models.transaction.on_commit and the .delay reference imported there). Refs GUNDI-5297 Addresses PR #406 review comments from @marianobrc Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Thanks for the review @marianobrc — addressed your actionable comments in c929a1c.
Tests updated to point at the new dispatch location ( |
Comment on lines
+1
to
+10
| import logging | ||
|
|
||
| from django.db.models.signals import pre_delete | ||
| from django.dispatch import receiver | ||
|
|
||
| from .models.v2 import IntegrationConfiguration | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
| f"{[a.value for a in missing]}" | ||
| ) | ||
| if not dry_run: | ||
| integration.create_missing_configurations(actions=actions) |
Comment on lines
+446
to
+452
| actions = list(integration_type.actions.all()) | ||
| logger.info( | ||
| "Backfilling configurations for IntegrationType %s (value=%s, actions=%d)", | ||
| integration_type_id, integration_type.value, len(actions), | ||
| ) | ||
| for integration in Integration.objects.filter(type_id=integration_type_id).iterator(): | ||
| integration.create_missing_configurations(actions=actions) |
The test asserted total config count grew by exactly 1, but the backfill is "make integration complete for its type", not "create exactly one row". Since the er_destination_without_show_permissions_config fixture deliberately leaves show_permissions unconfigured, creating a new action triggers backfill of *all* missing configs (correct behavior). The test still verifies the new action's config exists, which is the actual property under test. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
post_savesignal onIntegrationAction(created=True, viatransaction.on_commit) that backfillsIntegrationConfigurationrows on every existingIntegrationof the action's type.repair_integration_configurationsmanagement command for ad-hoc recovery (--integration,--integration-type,--dry-run).pytestdocker-compose service (profiletest) so the suite can run against real Postgres without a host install.Both layers reuse the existing idempotent
Integration.create_missing_configurations()(cdip_admin/integrations/models/v2/models.py:367-374), so re-runs are safe.Why
When an
Integrationis created, its serializer fills in one emptyIntegrationConfigurationperIntegrationActionattached to the type at that moment. If a newIntegrationActionis added later, pre-existingIntegrations of that type are stuck missing a row for it — there's no automatic backfill. A real Integration was observed in this state. The signal closes the gap going forward; the management command repairs the existing strays.Periodic actions are included:
IntegrationConfiguration._post_save(models/v2/models.py:451-476) creates the correspondingPeriodicTaskinitiallyenabled=integration.is_used_as_provider, matching the existing create flow.Files
cdip_admin/integrations/signals.py— newpost_savereceivercdip_admin/integrations/management/commands/repair_integration_configurations.py— new commandcdip_admin/integrations/tests/test_signals.py— signal coveragecdip_admin/integrations/tests/test_commands.py— extended for the new commanddocker-compose.yml— new opt-inpytestserviceOut of scope
django-celery-beatPeriodicTaskrow usingdjango.core.management.call_command.IntegrationActiondeletion already cascades toIntegrationConfiguration(on_delete=CASCADE).Test plan
docker compose run --rm pytest integrations/tests/test_signals.py integrations/tests/test_commands.py -vpasses locallyIntegrationAction.objects.create(integration_type=..., ...)triggers backfill on every existing Integration of that typepython3 manage.py repair_integration_configurations --integration <uuid>creates missing configs; second run is a no-op--dry-runreports without writingRefs GUNDI-5297
🤖 Generated with Claude Code