From c9b547d6d950618650106377e40d3a29fed5bb90 Mon Sep 17 00:00:00 2001 From: Peter Braun Date: Thu, 22 Oct 2026 15:46:11 +0200 Subject: [PATCH] fix: do not call .filter() on list before converting to queryset (#16752) * fix: do not call .filter() on list before converting to queryset * fix: optimize slicing of queryset * fix: preserve existing filters and ordering on the queryset --- awx/main/models/inventory.py | 20 +++----- .../tests/functional/tasks/test_fact_cache.py | 51 +++++++++++++++++++ 2 files changed, 59 insertions(+), 12 deletions(-) diff --git a/awx/main/models/inventory.py b/awx/main/models/inventory.py index 4157d96921..a66724fe35 100644 --- a/awx/main/models/inventory.py +++ b/awx/main/models/inventory.py @@ -282,21 +282,17 @@ class Inventory(CommonModelNameNotUnique, RelatedJobsMixin, OpaQueryPathMixin): def get_sliced_hosts(self, host_queryset, slice_number, slice_count): """ - Returns a slice of Hosts given a slice number and total slice count, or - the original queryset if slicing is not requested. - - NOTE: If slicing is performed, this will return a List[Host] with the - resulting slice. If slicing is not performed it will return the - original queryset (not evaluating it or forcing it to a list). This - puts the burden on the caller to check the resulting type. This is - non-ideal because it's easy to get wrong, but I think the only way - around it is to force the queryset which has memory implications for - large inventories. + Returns a queryset of Hosts for a given slice number and total slice + count, or the original queryset if slicing is not requested. """ - if slice_count > 1 and slice_number > 0: offset = slice_number - 1 - host_queryset = host_queryset[offset::slice_count] + # Fetch only PKs to avoid loading the full host records into memory + # before slicing the query set. Unfortunately it seems that there is + # no good way to slice a queryset without converting it to a list first + all_pks = list(host_queryset.values_list('pk', flat=True)) + sliced_pks = all_pks[offset::slice_count] + host_queryset = host_queryset.filter(pk__in=sliced_pks) return host_queryset def get_script_data(self, hostvars=False, towervars=False, show_all=False, slice_number=1, slice_count=1): diff --git a/awx/main/tests/functional/tasks/test_fact_cache.py b/awx/main/tests/functional/tasks/test_fact_cache.py index 616bf67d65..fb6566389e 100644 --- a/awx/main/tests/functional/tasks/test_fact_cache.py +++ b/awx/main/tests/functional/tasks/test_fact_cache.py @@ -221,3 +221,54 @@ class TestConstructedInventoryFactCache: assert source_host.ansible_facts == {'role': 'web', 'deployed': True} assert not constructed_host.ansible_facts, f'Facts were stored on the constructed host: {constructed_host.ansible_facts!r}' + + +@pytest.mark.django_db +class TestSlicedJobFactCache: + """finish_fact_cache must work when get_hosts_for_fact_cache returns a sliced result.""" + + def test_sliced_hosts_persist_facts(self, organization, artifacts_dir): + """Sliced jobs must persist facts via get_sliced_hosts queryset. + + Regression test for AAP-97902: get_sliced_hosts() used to return a list + when slice_count > 1, causing finish_fact_cache() to crash with + AttributeError when calling .filter(). get_sliced_hosts() now converts + the step-sliced list back to a QuerySet. + """ + inv = Inventory.objects.create(organization=organization, name='slice-inv') + hosts = [inv.hosts.create(name=f'host{i}') for i in range(4)] + + for h in hosts: + h.ansible_facts = {'old': True} + h.ansible_facts_modified = now() + h.save(update_fields=['ansible_facts', 'ansible_facts_modified']) + + host_qs = inv.hosts.all() + sliced_qs = inv.get_sliced_hosts(host_qs, slice_number=1, slice_count=2) + + # get_sliced_hosts must return a QuerySet, not a list + assert hasattr(sliced_qs, 'filter'), 'get_sliced_hosts must return a QuerySet' + + sliced_hosts = list(sliced_qs) + start_fact_cache(sliced_qs, artifacts_dir=artifacts_dir, timeout=0, inventory_id=inv.id) + + # Simulate ansible writing new facts for each host in the slice + for h in sliced_hosts: + fact_file = os.path.join(artifacts_dir, 'fact_cache', h.name) + future = time.time() + 60 + with open(fact_file, 'w') as f: + json.dump({'updated': True, 'host': h.name}, f) + os.utime(fact_file, (future, future)) + + # This would crash with AttributeError before the fix + finish_fact_cache(sliced_qs, artifacts_dir=artifacts_dir, inventory_id=inv.id) + + for h in hosts: + h.refresh_from_db() + + sliced_names = {h.name for h in sliced_hosts} + for h in hosts: + if h.name in sliced_names: + assert h.ansible_facts.get('updated') is True, f'{h.name} should have updated facts' + else: + assert h.ansible_facts == {'old': True}, f'{h.name} should be unchanged'