Skip to content

Release 20260508 hotfix#412

Merged
chrisdoehring merged 8 commits into
mainfrom
release-20260508
May 21, 2026
Merged

Release 20260508 hotfix#412
chrisdoehring merged 8 commits into
mainfrom
release-20260508

Conversation

@chrisdoehring
Copy link
Copy Markdown
Contributor

@chrisdoehring chrisdoehring commented May 11, 2026

@marianobrc this is a hotfix I pushed to rectify a bug I introduced in my last PR (re: adding a uniqueness constraint on IntegrationConfiguration).

...and here is Claude's explanation below:

This pull request improves the validation logic for integration configuration updates, ensuring more user-friendly error handling and preventing duplicate or conflicting configurations. It also adds targeted tests to cover these scenarios and disables redundant validation that conflicted with the intended API design.

Validation improvements:

  • Disabled the auto-generated UniqueTogetherValidator in IntegrationConfigurationCreateUpdateSerializer.Meta to prevent it from incorrectly requiring the integration field, since this field is implied by the URL and handled elsewhere.
  • Enhanced the validate method in IntegrationCreateUpdateSerializer to:
    • Return a clear 400 error if a PATCH request tries to add a new configuration for an action that already exists, avoiding a database-level 500 error.
    • Return a 400 error if the request payload contains duplicate configurations for the same action.
  • Hardened update() so an action echoed back alongside an id cannot silently repoint an existing configuration row — it's popped before reaching update_or_create. Pairs with the validation above so a row's (integration, action) identity is effectively immutable once created.

Test coverage:

  • Added tests to verify:
    • The API accepts payloads that omit the integration field in configuration items, matching the expected portal behavior.
    • The API returns a 400 error for attempts to add a new configuration for an action that already exists.
    • The API returns a 400 error for duplicate actions in the same payload.
    • PATCH with a config id that belongs to a different integration returns 400 (rather than silently repointing the row).
    • PATCH with id + a different action is accepted, but the row's action is NOT repointed — only data updates.

CI/CD pipeline migration (DASOPS-2057)

Beyond the serializer hotfix, this release branch also picks up the operational work tracked under DASOPS-2057. Splitting it out at this point isn't practical — the release-20260508 branch is the cutoff for this release and the CI/CD migration needs to ride along — so calling it out here explicitly:

  • Container registry: build/push targets the new serca-artifact-registry (single regional registry) instead of the legacy registry path.
  • Deploy jobs: stage and prod deploys flip from the legacy GKE-credentialed workflow to the ArgoCD image-bump reusable workflow.
  • Approval gate: a manual approve_prod job now guards production deploys (previously implicit via branch protections).
  • deploy_prod_legacy retained as a fallback path with direct GKE credentials in case the ArgoCD-driven prod deploy needs to be bypassed during the cutover window.

Operational prerequisites already in place: the serca-artifact-registry GCP project + IAM bindings for the workflow's service account, and the GitHub environment(s) referenced by approve_prod. No new application secrets are required by this PR.

chrisdoehring and others added 3 commits May 11, 2026 14:44
…ializer

The UniqueConstraint(integration, action) added in #406 caused DRF to
auto-attach a UniqueTogetherValidator to IntegrationConfigurationCreate
UpdateSerializer. That validator forces `integration` and `action` to be
required regardless of their declared `required=False`, which broke
PATCH /v2/integrations/{id}/ from the portal — the portal omits
`integration` on each configuration item because it is implied by the URL.

The parent IntegrationCreateUpdateSerializer.update() already injects
`integration` from the URL post-validation and uses update_or_create on
the configuration id, and the DB-level UniqueConstraint enforces
uniqueness on insert. The serializer-level UniqueTogetherValidator is
redundant.

Adds a regression test mirroring the portal's payload shape
({id, action, data} with no `integration`).

Fixes GUNDI-5313

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses PR feedback. With the auto-generated UniqueTogetherValidator
disabled (previous commit), the (integration, action) UniqueConstraint
could be hit at insert time and surface as an unhandled 500. Add
explicit duplicate checks in IntegrationCreateUpdateSerializer.validate():

1. On PATCH, a new (no-id) configuration item for an action that already
   has a config raises 400 with a clear message ("A configuration for
   action X already exists. Include 'id' in the payload to update it.").
2. Two configuration items in the same request targeting the same action
   raises 400 with "Duplicate configurations for action X in the request."

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lidator

fix: PATCH /v2/integrations/ rejects nested configurations (GUNDI-5313)
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adjusts the integrations v2 update/create validation flow to better match the portal’s payload shape (configuration items omitting integration) and to proactively reject conflicting configuration updates with clearer 400-level validation errors.

Changes:

  • Disabled DRF’s auto-generated UniqueTogetherValidator for IntegrationConfigurationCreateUpdateSerializer to avoid incorrectly requiring integration in nested configuration payloads.
  • Added request-level validation in IntegrationCreateUpdateSerializer.validate() to reject (a) adding a new configuration for an action that already has one and (b) duplicate actions in the same request payload.
  • Added targeted API tests for the above scenarios.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
cdip_admin/api/v2/serializers.py Disables nested unique-together validator and adds additional validation for PATCH/create payloads (duplicate actions, conflicting new configs).
cdip_admin/api/v2/tests/test_integrations_api.py Adds regression and validation tests for portal-shaped payloads and duplicate/conflicting configuration updates.
Comments suppressed due to low confidence (1)

cdip_admin/api/v2/serializers.py:532

  • When an item includes an id, the code does IntegrationConfiguration.objects.get(id=...) without scoping to self.instance (the integration being patched). This allows a client to reference a configuration from a different integration (and potentially reassign it in update_or_create), and a nonexistent id will raise DoesNotExist and surface as a 500. Prefer fetching with IntegrationConfiguration.objects.get(id=..., integration=self.instance) (when self.instance is set) and raising a ValidationError if not found/belongs to a different integration.
            if self.instance and "id" in configuration:  # Integration Update
                action = IntegrationConfiguration.objects.get(id=configuration["id"]).action
            else:  # Create a new integration or new config

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread cdip_admin/api/v2/serializers.py
Comment thread cdip_admin/api/v2/tests/test_integrations_api.py Outdated
…action on id-update; tighten test assertions)

Three Copilot review comments on #412:

1. `IntegrationConfiguration.objects.get(id=...)` in validate() wasn't
   scoped to the integration being patched. A nonexistent id surfaced
   as a 500 from DoesNotExist, and an id belonging to a different
   integration was accepted and later repointed via update_or_create's
   defaults. Scope the lookup to `self.instance.configurations` and
   raise a clean 400 with "Configuration '...' was not found on this
   integration."

2. `update()` passed the full `config_data` (including any payload
   `action`) as `defaults` to `update_or_create`. When `id` was present
   that could repoint the row's action — colliding with the
   (integration, action) UniqueConstraint and bypassing the schema
   validation in validate(). Pop `action` from defaults when `id` is
   present so id-updates can only change `data`.

3. Tests asserted on `str(response.json())` — brittle. Assert against
   `response.json()["non_field_errors"][0]` instead.

Added three regression tests:
- unknown config id → 400 (not 500)
- id belonging to a different integration → 400, original config
  untouched
- payload `action` alongside `id` is ignored (action stays put)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@chrisdoehring
Copy link
Copy Markdown
Contributor Author

Addressing the three review comments here: #413 (into release-20260508, will flow into this PR on merge).

Summary of fixes:

  1. Scoped id lookupIntegrationConfiguration.objects.get(id=...) now uses self.instance.configurations.get(id=...), so a bogus id surfaces as a 400 instead of a 500, and an id from a different integration is rejected rather than being repointed by update_or_create's defaults.

  2. Freeze action on id-updatesupdate() now pops action from defaults when id is present. The portal sometimes echoes back action alongside id for client convenience; it's now correctly ignored instead of (silently) trying to repoint the row's action.

  3. Tighter test assertions — switched from str(response.json()) to response.json()["non_field_errors"][0].

Three new regression tests added: unknown id, cross-integration id, and action-repointing attempt.

fix: address Copilot review comments from PR #412
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

@chrisdoehring chrisdoehring requested a review from marianobrc May 12, 2026 03:00
…[release-20260508] (#422)

v12 uses direct gcloud container clusters get-credentials instead of
Fleet Connect Gateway, fixing the deploy_prod_legacy failure caused by
cdip-prod01 not being registered in the serca-tools Fleet.
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Comment thread .github/workflows/main.yml
@chrisdoehring chrisdoehring merged commit d90190e into main May 21, 2026
17 checks passed
@chrisdoehring chrisdoehring deleted the release-20260508 branch May 21, 2026 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants