From c1bd2eb3387948e11dc638258ba33c7fd1b5d19b Mon Sep 17 00:00:00 2001 From: Dirk Julich Date: Thu, 18 Jun 2026 18:35:22 +0200 Subject: [PATCH] [AAP-72817] Fix cartesian product in organization user/admin count queries (#16501) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix cartesian product in organization user/admin count queries The organizations list and detail endpoints annotated each org with user and admin counts using two Count() calls that traverse the Role.members M2M. Django generated two LEFT JOINs on the same through table, crossing every member row with every admin row before COUNT(DISTINCT) reduced the product. At scale (2,617 members × 46,233 admins) this produced 120M intermediate rows and 96-second query times, causing 504 timeouts. Replace with independent Subquery expressions that each query main_rbac_roles_members separately - no cross product. Fixes: AAP-72817 Fixes: AAP-72480 * Fix variable names which do not meet coding standards * Fix formatting inconsistency in organization detail subquery annotation Break the long .annotate() line across multiple lines to match the style used in mixin.py. * Rewrite org count subqueries to use DAB RBAC models Replace old RBAC Role.members.through subqueries with RoleUserAssignment-based correlated subqueries, querying managed RoleDefinitions ('Organization Member' / 'Organization Admin') directly. This aligns with the DAB RBAC migration direction and eliminates dependency on the deprecated ImplicitRoleField M2M tables for these counts. Update test fixtures to use RoleDefinition.give_permission() and add setup_managed_roles where needed. * Fix collection tests: set up managed role definitions The DAB RBAC migration to use RoleUserAssignment subqueries in organization views requires managed role definitions (Organization Member, Organization Admin) to exist in the test database. Add an autouse fixture to the collection test conftest that calls setup_managed_role_definitions() before each test. * Add setup_managed_roles fixture to functional tests hitting org views Tests that hit organization list/detail views now require the setup_managed_roles fixture to pre-create the Organization Member and Organization Admin RoleDefinition objects used by the DAB RBAC subqueries. * Revert setup_managed_roles from ext_auditor tests The setup_managed_roles fixture conflicts with the ext_auditor_rd fixture by deleting the Alien Auditor role definition. These tests don't need it — the defensive view code handles missing role definitions gracefully. * Handle missing Organization Member/Admin role definitions gracefully Use filter().first() instead of get() for RoleDefinition lookups in organization list and detail views. Returns 0 for user/admin counts when role definitions are not yet created, preventing 500 errors in environments where post_migrate signals haven't run. * Cast OuterRef('pk') to TextField for RoleUserAssignment.object_id comparison RoleUserAssignment.object_id is a TextField, but OuterRef('pk') on Organization produces an integer. PostgreSQL strictly rejects text = integer comparisons. Use Cast() to explicitly convert the PK to text. --------- Co-authored-by: Claude Opus 4.6 --- awx/api/views/mixin.py | 31 ++++++++++--- awx/api/views/organization.py | 45 +++++++++++++++---- .../api/test_organization_counts.py | 9 ++-- .../functional/api/test_organizations.py | 14 +++--- awx/main/tests/functional/api/test_user.py | 2 +- .../test_ansible_id_display.py | 2 +- .../tests/functional/rbac/test_rbac_api.py | 4 +- awx/main/tests/functional/test_named_url.py | 2 +- awx_collection/test/awx/conftest.py | 8 ++++ 9 files changed, 88 insertions(+), 29 deletions(-) diff --git a/awx/api/views/mixin.py b/awx/api/views/mixin.py index eda90edbfa..beb0aca007 100644 --- a/awx/api/views/mixin.py +++ b/awx/api/views/mixin.py @@ -4,7 +4,8 @@ import dateutil import logging -from django.db.models import Count +from django.db.models import Count, OuterRef, Subquery, TextField +from django.db.models.functions import Cast, Coalesce from django.db import transaction from django.shortcuts import get_object_or_404 from django.utils.timezone import now @@ -15,6 +16,7 @@ from rest_framework.response import Response from rest_framework import status from awx.main.constants import ACTIVE_STATES +from ansible_base.rbac.models import RoleDefinition, RoleUserAssignment from awx.main.models import Organization from awx.main.utils import get_object_or_400 from awx.main.models.ha import Instance, InstanceGroup, schedule_policy_task @@ -177,10 +179,29 @@ class OrganizationCountsMixin(object): db_results['projects'] = project_qs.values('organization').annotate(Count('organization')).order_by('organization') - # Other members and admins of organization are always viewable - db_results['users'] = org_qs.annotate(users=Count('member_role__members', distinct=True), admins=Count('admin_role__members', distinct=True)).values( - 'id', 'users', 'admins' - ) + member_rd = RoleDefinition.objects.filter(name='Organization Member').first() + admin_rd = RoleDefinition.objects.filter(name='Organization Admin').first() + + if member_rd and admin_rd: + + def assignment_count(rd): + return Coalesce( + Subquery( + RoleUserAssignment.objects.filter( + object_id=Cast(OuterRef('pk'), output_field=TextField()), + role_definition=rd, + ) + .values('role_definition') + .annotate(c=Count('pk')) + .values('c') + ), + 0, + ) + + db_results['users'] = org_qs.annotate( + users=assignment_count(member_rd), + admins=assignment_count(admin_rd), + ).values('id', 'users', 'admins') count_context = {} for org in org_id_list: diff --git a/awx/api/views/organization.py b/awx/api/views/organization.py index 8795c2ed12..24054383f1 100644 --- a/awx/api/views/organization.py +++ b/awx/api/views/organization.py @@ -5,11 +5,13 @@ import logging # Django -from django.db.models import Count +from django.db.models import Count, OuterRef, Subquery, TextField +from django.db.models.functions import Cast, Coalesce from django.contrib.contenttypes.models import ContentType from django.utils.translation import gettext_lazy as _ # AWX +from ansible_base.rbac.models import RoleDefinition, RoleUserAssignment from awx.main.models import ( ActivityStream, Inventory, @@ -77,16 +79,41 @@ class OrganizationDetail(RelatedJobsPreventDeleteMixin, RetrieveUpdateDestroyAPI org_counts = {} access_kwargs = {'accessor': self.request.user, 'role_field': 'read_role'} - direct_counts = ( - Organization.objects.filter(id=org_id) - .annotate(users=Count('member_role__members', distinct=True), admins=Count('admin_role__members', distinct=True)) - .values('users', 'admins') - ) + member_rd = RoleDefinition.objects.filter(name='Organization Member').first() + admin_rd = RoleDefinition.objects.filter(name='Organization Admin').first() - if not direct_counts: + if member_rd and admin_rd: + + def assignment_count(rd): + return Coalesce( + Subquery( + RoleUserAssignment.objects.filter( + object_id=Cast(OuterRef('pk'), output_field=TextField()), + role_definition=rd, + ) + .values('role_definition') + .annotate(c=Count('pk')) + .values('c') + ), + 0, + ) + + direct_counts = ( + Organization.objects.filter(id=org_id) + .annotate( + users=assignment_count(member_rd), + admins=assignment_count(admin_rd), + ) + .values('users', 'admins') + ) + + if direct_counts: + org_counts = direct_counts[0] + else: + org_counts = {'users': 0, 'admins': 0} + + if not org_counts: return full_context - - org_counts = direct_counts[0] org_counts['inventories'] = Inventory.accessible_objects(**access_kwargs).filter(organization__id=org_id).count() org_counts['teams'] = Team.accessible_objects(**access_kwargs).filter(organization__id=org_id).count() org_counts['projects'] = Project.accessible_objects(**access_kwargs).filter(organization__id=org_id).count() diff --git a/awx/main/tests/functional/api/test_organization_counts.py b/awx/main/tests/functional/api/test_organization_counts.py index d1790f413c..f647cf7d53 100644 --- a/awx/main/tests/functional/api/test_organization_counts.py +++ b/awx/main/tests/functional/api/test_organization_counts.py @@ -1,20 +1,23 @@ import pytest +from ansible_base.rbac.models import RoleDefinition from awx.api.versioning import reverse from awx.main.models import Project, Host @pytest.fixture -def organization_resource_creator(organization, user): +def organization_resource_creator(organization, user, setup_managed_roles): def rf(users, admins, job_templates, projects, inventories, teams): + member_rd = RoleDefinition.objects.get(name='Organization Member') + admin_rd = RoleDefinition.objects.get(name='Organization Admin') # Associate one resource of every type with the organization for i in range(users): member_user = user('org-member %s' % i) - organization.member_role.members.add(member_user) + member_rd.give_permission(member_user, organization) for i in range(admins): admin_user = user('org-admin %s' % i) - organization.admin_role.members.add(admin_user) + admin_rd.give_permission(admin_user, organization) for i in range(teams): organization.teams.create(name='org-team %s' % i) for i in range(inventories): diff --git a/awx/main/tests/functional/api/test_organizations.py b/awx/main/tests/functional/api/test_organizations.py index af2f918ba0..6a234c09f5 100644 --- a/awx/main/tests/functional/api/test_organizations.py +++ b/awx/main/tests/functional/api/test_organizations.py @@ -56,7 +56,7 @@ def test_organization_list_access_tests(options, head, get, admin, alice): @pytest.mark.django_db -def test_organization_access_tests(organization, get, admin, alice, bob): +def test_organization_access_tests(organization, get, admin, alice, bob, setup_managed_roles): organization.member_role.members.add(alice) get(reverse('api:organization_detail', kwargs={'pk': organization.id}), user=admin, expect=200) get(reverse('api:organization_detail', kwargs={'pk': organization.id}), user=alice, expect=200) @@ -65,14 +65,14 @@ def test_organization_access_tests(organization, get, admin, alice, bob): @pytest.mark.django_db -def test_organization_list_integrity(organization, get, admin, alice): +def test_organization_list_integrity(organization, get, admin, alice, setup_managed_roles): res = get(reverse('api:organization_list'), user=admin) for field in ['id', 'url', 'name', 'description', 'created']: assert field in res.data['results'][0] @pytest.mark.django_db -def test_organization_list_order_integrity(organizations, get, admin): +def test_organization_list_order_integrity(organizations, get, admin, setup_managed_roles): # check that the order of the organization list retains integrity. orgs = organizations(4) org_ids = {} @@ -83,7 +83,7 @@ def test_organization_list_order_integrity(organizations, get, admin): @pytest.mark.django_db -def test_organization_list_visibility(organizations, get, admin, alice): +def test_organization_list_visibility(organizations, get, admin, alice, setup_managed_roles): orgs = organizations(2) res = get(reverse('api:organization_list'), user=admin) @@ -151,7 +151,7 @@ def test_organization_inventory_list_order_integrity(organization, admin, invent @pytest.mark.django_db -def test_create_organization(post, admin, alice): +def test_create_organization(post, admin, alice, setup_managed_roles): new_org = {'name': 'new org', 'description': 'my description'} res = post(reverse('api:organization_list'), new_org, user=admin, expect=201) assert res.data['name'] == new_org['name'] @@ -197,7 +197,7 @@ def test_add_admin_to_organization_xfail(post, organization, alice, bob): @pytest.mark.django_db -def test_update_organization(get, put, organization, alice, bob): +def test_update_organization(get, put, organization, alice, bob, setup_managed_roles): organization.admin_role.members.add(alice) data = get(reverse('api:organization_detail', kwargs={'pk': organization.id}), user=alice, expect=200).data data['description'] = 'hi' @@ -209,7 +209,7 @@ def test_update_organization(get, put, organization, alice, bob): @pytest.mark.django_db -def test_update_organization_max_hosts(get, put, organization, admin, alice, bob): +def test_update_organization_max_hosts(get, put, organization, admin, alice, bob, setup_managed_roles): # Admin users can get and update max_hosts data = get(reverse('api:organization_detail', kwargs={'pk': organization.id}), user=admin, expect=200).data assert organization.max_hosts == 0 diff --git a/awx/main/tests/functional/api/test_user.py b/awx/main/tests/functional/api/test_user.py index 366f6f943c..202cf56ccc 100644 --- a/awx/main/tests/functional/api/test_user.py +++ b/awx/main/tests/functional/api/test_user.py @@ -259,7 +259,7 @@ def test_user_verify_attribute_created(admin, get): @pytest.mark.django_db -def test_org_not_shown_in_admin_user_sublists(admin_user, get, organization): +def test_org_not_shown_in_admin_user_sublists(admin_user, get, organization, setup_managed_roles): for view_name in ('user_admin_of_organizations_list', 'user_organizations_list'): url = reverse(f'api:{view_name}', kwargs={'pk': admin_user.pk}) r = get(url, user=admin_user, expect=200) diff --git a/awx/main/tests/functional/dab_resource_registry/test_ansible_id_display.py b/awx/main/tests/functional/dab_resource_registry/test_ansible_id_display.py index bf0d550262..51b6e1ffb6 100644 --- a/awx/main/tests/functional/dab_resource_registry/test_ansible_id_display.py +++ b/awx/main/tests/functional/dab_resource_registry/test_ansible_id_display.py @@ -19,7 +19,7 @@ def assert_has_resource(list_response, obj=None): @pytest.mark.django_db -def test_organization_ansible_id(organization, admin_user, get): +def test_organization_ansible_id(organization, admin_user, get, setup_managed_roles): url = reverse('api:organization_list') response = get(url=url, user=admin_user, expect=200) assert_has_resource(response, obj=organization) diff --git a/awx/main/tests/functional/rbac/test_rbac_api.py b/awx/main/tests/functional/rbac/test_rbac_api.py index 61e7425c4d..3207971676 100644 --- a/awx/main/tests/functional/rbac/test_rbac_api.py +++ b/awx/main/tests/functional/rbac/test_rbac_api.py @@ -393,7 +393,7 @@ def test_remove_team_from_role(post, team, admin, role): @pytest.mark.django_db -def test_ensure_rbac_fields_are_present(organization, get, admin): +def test_ensure_rbac_fields_are_present(organization, get, admin, setup_managed_roles): url = reverse('api:organization_detail', kwargs={'pk': organization.id}) response = get(url, admin) assert response.status_code == 200 @@ -412,7 +412,7 @@ def test_ensure_rbac_fields_are_present(organization, get, admin): @pytest.mark.django_db -def test_ensure_role_summary_is_present(organization, get, user): +def test_ensure_role_summary_is_present(organization, get, user, setup_managed_roles): url = reverse('api:organization_detail', kwargs={'pk': organization.id}) response = get(url, user('admin', True)) assert response.status_code == 200 diff --git a/awx/main/tests/functional/test_named_url.py b/awx/main/tests/functional/test_named_url.py index 5bf12653ac..9650fd7b60 100644 --- a/awx/main/tests/functional/test_named_url.py +++ b/awx/main/tests/functional/test_named_url.py @@ -38,7 +38,7 @@ def test_team(get, admin_user): @pytest.mark.django_db -def test_organization(get, admin_user): +def test_organization(get, admin_user, setup_managed_roles): test_org = Organization.objects.create(name='test_org') url = reverse('api:organization_detail', kwargs={'pk': test_org.pk}) response = get(url, user=admin_user, expect=200) diff --git a/awx_collection/test/awx/conftest.py b/awx_collection/test/awx/conftest.py index 4fe89f8b72..fb122ac536 100644 --- a/awx_collection/test/awx/conftest.py +++ b/awx_collection/test/awx/conftest.py @@ -20,6 +20,7 @@ from ansible.module_utils.six import raise_from from ansible_base.rbac.models import RoleDefinition, DABPermission from ansible_base.rbac import permission_registry +from awx.main.migrations._dab_rbac import setup_managed_role_definitions from awx.main.tests.conftest import load_all_credentials # noqa: F401; pylint: disable=unused-import from awx.main.tests.functional.conftest import _request from awx.main.tests.functional.conftest import credentialtype_scm, credentialtype_ssh # noqa: F401; pylint: disable=unused-import @@ -92,6 +93,13 @@ def sanitize_dict(din): return str(din) # translation proxies often not string but stringlike +@pytest.fixture(autouse=True) +def _setup_managed_roles(db): + from django.apps import apps as global_apps + + setup_managed_role_definitions(global_apps, None) + + @pytest.fixture(autouse=True) def collection_path_set(monkeypatch): """Monkey patch sys.path, insert the root of the collection folder