AAP-87798 fix: denormalize object_ansible_id onto assignment models - #1120
Draft
AlanCoding wants to merge 17 commits into
Draft
AAP-87798 fix: denormalize object_ansible_id onto assignment models#1120AlanCoding wants to merge 17 commits into
AlanCoding wants to merge 17 commits into
Conversation
Contributor
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueComment |
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## devel #1120 +/- ##
==========================================
- Coverage 94.69% 94.63% -0.07%
==========================================
Files 259 261 +2
Lines 14728 14805 +77
Branches 2274 2290 +16
==========================================
+ Hits 13946 14010 +64
- Misses 782 795 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
AlanCoding
commented
Sep 1, 2026
AlanCoding
force-pushed
the
AAP-87798-denormalize-object-ansible-id
branch
from
September 2, 2026 12:54
d0ef975 to
3bf8cf5
Compare
Member
Author
|
Sonar:
codecov
Also, most of the uncovered code is from handling the RBAC app not being installed... which it is in test_app. This wouldn't really be valuable coverage. |
AlanCoding
force-pushed
the
AAP-87798-denormalize-object-ansible-id
branch
from
September 9, 2026 13:21
82ef653 to
3ff19e9
Compare
Replace the AssignmentResourceField JOIN (introduced in ansible#1093) with a denormalized object_ansible_id UUIDField on RoleUserAssignment and RoleTeamAssignment. The JOIN approach was unsafe: it compared DABContentType IDs with Django ContentType IDs across different tables. Because DABContentType allocates IDs independently for remote types, a remote ID could collide with a local Django ContentType ID, causing the JOIN to select the wrong Resource and store its ansible_id as object_id for integer-PK assignments. The new approach: - Adds object_ansible_id (nullable UUID) to AssignmentBase. - Populates it at write time via a Resource lookup bridged through ContentType.get_for_model (correct, no cross-table ID assumption). - Migration 0011 repairs corrupt assignments (delete those with a UUID object_id under an integer-PK content type) then backfills the new field from Resource via (app_label, model) matching. - Repair logic lives in rbac/repair.py and can be called standalone. - Service API views drop the JOIN annotation; serializer reads the field directly. - Removes AssignmentResourceField and _ContentTypeBridgeLookup entirely. Covers: backfill of object_ansible_id from Resource, deletion of corrupt assignments (UUID object_id under integer-PK content type), idempotence when the field is already populated, no-op for global assignments, null result when no Resource exists, and correct split when valid and corrupt assignments coexist. Tests exercise both user and team assignment models. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Replace per-object Resource.get() calls (O(N) queries for N assignments) with a sentinel + batch pattern: - _ANSIBLE_ID_NOT_FETCHED sentinel distinguishes "not yet looked up" from None (remote object, global assignment, resource not found). - _resolve_content_object emits the sentinel for local objects; remote objects still emit None immediately. - _batch_fill_ansible_ids groups pending assignments by Django ContentType and issues one Resource query per distinct type, regardless of how many objects share that type. - give_assignments calls _batch_fill_ansible_ids before _create_assignments, covering both the bulk_give_permissions path and direct callers. - _create_assignments sanitizes any remaining sentinel to None as a safety net. - ResolvedAssignment.object_ansible_id is typed Any to satisfy typeguard runtime checks with the sentinel value. The old implementation needed get_attribute/to_representation overrides because it accessed the instance directly to work around the JOIN annotation. With object_ansible_id now a real denormalized model field, DRF's UUIDField handles read representation correctly without any override. AAP-87798 refactor: move ansible_id→object_id resolution into validate() ObjectAnsibleIdField existed only to resolve an incoming ansible_id UUID into a local object_id on the write path. That logic belongs next to the mutual-exclusion rules in validate(), not hidden in a custom field. Replace the custom field with a plain UUIDField and do the Resource lookup inline. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> AAP-87798 fix: update object_ansible_id on assignments when Resource.ansible_id changes During migrate_service_data, the gateway merge path sends new_ansible_id to the upstream service to unify two objects' identifiers. update_resource() now propagates that change to RoleUserAssignment/RoleTeamAssignment rows whose object_ansible_id matched the old value — keeping the cached UUID consistent. Replace the hand-rolled django_ct_cache dict and per-type get_by_natural_key loop with a single get_for_models() call, which uses Django's built-in ContentType manager cache. Remote types (not registered locally) are handled by LookupError from django_apps.get_model rather than ContentType.DoesNotExist. AAP-87798 fix: repair corruption filter uses exclude(uuid) not match(integer) AutoField/BigAutoField report 'serial'/'bigserial' on PostgreSQL but 'integer' on SQLite, so filtering on pk_field_type='integer' silently missed all corrupt rows in CI. Flip to exclude(pk_field_type='uuid') which correctly captures all non-UUID pk types regardless of backend. Add two tests for previously-uncovered paths in repair_assignment_corruption: - LookupError path: migration mode with no resource_registry available skips backfill but still runs the corrupt-deletion step - Remote content type: assignments whose DABContentType has no local Django ContentType match (remote service types) are left untouched during backfill Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…in-place - Takes user_resolved and team_resolved separately; no more n_user split - Uses itertools.chain over both lists in one comprehension to build needs_fetch, carrying the list reference for direct in-place _replace() write-back - Caller drops the concatenation, slice, and intermediate variables Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
chain, django_apps, and ContentType are unconditional dependencies and belong at the module level. Resource stays inline because resource_registry is optional. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ine import Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… usage chain(user_resolved, team_resolved) handles the read phases directly. Write-back is a simple for-resolved-in loop. No (lst, i, ra) tuple carrying needed; by_django_ct becomes a set of object_ids per CT. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…earer names
- Try Resource import at top of method; set None on ImportError and skip queries
- .values('object_id', 'ansible_id') avoids instantiating model objects we discard immediately
- Rename ct_by_model/ct_by_key to dab_ct_by_model/dab_ct_by_key for clarity
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
try/except ImportError doesn't catch apps-not-ready or other Django initialization errors. is_installed() is the correct check for whether an app is in INSTALLED_APPS. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ents Sequence doesn't support +; chain() is both correct and already imported. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…through it Remove the standalone/migration dual-mode footgun. apps is now a required argument; callers outside migrations pass django.apps.apps directly. ContentType is fetched via apps.get_model like all other models. Also switch Resource query to .values() to avoid model instantiation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…g all assignments The old code pulled every null-ansible_id assignment into a Python dict just to group by content_type. Now we query distinct content_type_ids and fetch assignments one content type at a time, keeping only one batch in memory. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Separate corrupt-assignment deletion from object_ansible_id backfill so repair runs before the field exists and backfill runs after it is added. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
repair.py is for data corruption fixes; backfill.py is for population of newly-added denormalized fields. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
repair_assignment_corruption now deletes assignments where the referenced object does not exist in the model's table, covering int-for-int swaps and not just UUID-under-integer-PK cases. backfill_object_ansible_id removes object_id__isnull=False from the base queryset and instead warns on inconsistent rows (content_type set, object_id null). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
AlanCoding
force-pushed
the
AAP-87798-denormalize-object-ansible-id
branch
from
September 9, 2026 14:05
3ff19e9 to
f9d84c3
Compare
|
DVCS PR Check Results: PR appears valid (JIRA key(s) found) |
|
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
1 participant
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.
Description
Trying to fix the root cause I described for #1113
Basically, have to undo the change #1093 and re-do it a completely different way. Adding a new field was already the obvious runner-up approach.
This adds a data migration to repair the bad data.
Type of Change
Self-Review Checklist