diff --git a/awx/api/generics.py b/awx/api/generics.py index 5e92842b39..ae2fceb26a 100644 --- a/awx/api/generics.py +++ b/awx/api/generics.py @@ -728,7 +728,7 @@ class SubListCreateAttachDetachAPIView(SubListCreateAPIView): return Response(attach_errors, status=status.HTTP_400_BAD_REQUEST) # Attach the object to the collection. - if sub not in relationship.all(): + if not relationship.contains(sub): relationship.add(sub) if created: diff --git a/awx/main/tests/functional/api/test_inventory.py b/awx/main/tests/functional/api/test_inventory.py index d77f1a9f8c..f8491c6692 100644 --- a/awx/main/tests/functional/api/test_inventory.py +++ b/awx/main/tests/functional/api/test_inventory.py @@ -7,9 +7,13 @@ from django.core.exceptions import ValidationError from awx.api.versioning import reverse -from awx.main.models import InventorySource, Inventory, ActivityStream, Organization +from awx.main.models import InventorySource, Inventory, ActivityStream, Organization, Host from awx.main.utils.inventory_vars import update_group_variables +from django.db import connection +from django.test.utils import CaptureQueriesContext +from django.utils.timezone import now + @pytest.fixture def scm_inventory(inventory, project): @@ -118,6 +122,27 @@ def test_inventory_host_list_ordering(scm_inventory, get, admin_user): assert host_list == expected_ids +@pytest.mark.django_db +def test_inventory_hosts_attach_uses_bounded_membership_check(inventory, post, admin_user): + """attach()'s membership check must filter by the host's own pk and be LIMIT-bounded, not scan every host in the inventory.""" + _now = now() + existing_hosts = [Host(name=f'existing-host-{i}', inventory=inventory, created=_now, modified=_now) for i in range(5)] + Host.objects.bulk_create(existing_hosts) + + with CaptureQueriesContext(connection) as ctx: + post(reverse('api:inventory_hosts_list', kwargs={'pk': inventory.id}), {'name': 'new-host'}, admin_user, expect=201) + + membership_check_queries = [ + q['sql'] + for q in ctx.captured_queries + if 'FROM "main_host"' in q['sql'] and '"main_host"."inventory_id" = ' in q['sql'] and '"main_host"."name" = ' not in q['sql'] + ] + assert membership_check_queries, "expected a query checking host membership in the inventory" + for sql in membership_check_queries: + assert '"main_host"."id" = ' in sql, f"attach() membership check is not filtered by the host's own pk: {sql}" + assert 'LIMIT' in sql, f"attach() membership check is not LIMIT-bounded: {sql}" + + @pytest.mark.django_db def test_inventory_group_name_unique(scm_inventory, post, admin_user): inv_src = scm_inventory.inventory_sources.first() diff --git a/awx/main/tests/unit/api/test_generics.py b/awx/main/tests/unit/api/test_generics.py index 05cc72cc19..69928806e9 100644 --- a/awx/main/tests/unit/api/test_generics.py +++ b/awx/main/tests/unit/api/test_generics.py @@ -31,7 +31,9 @@ def mock_organization(): @pytest.fixture def parent_relationship_factory(mocker): def rf(serializer_class, relationship_name, relationship_value=mocker.Mock()): - mock_parent_relationship = mocker.MagicMock(**{'%s.add.return_value' % relationship_name: relationship_value}) + mock_parent_relationship = mocker.MagicMock( + **{'%s.add.return_value' % relationship_name: relationship_value, '%s.contains.return_value' % relationship_name: False} + ) mocker.patch('awx.api.generics.ParentMixin.get_parent_object', return_value=mock_parent_relationship) serializer = serializer_class()