Fix collection redirects for filter and test plugins (#77210)

* Fix collection redirects for jinja2 filters/tests

* Handle recursive redirects

Co-authored-by: Matt Martz <matt@sivel.net>

changelogs/fragments/77210-fix-collection-filter-test-redirects.yml

@@ -0,0 +1,2 @@
+bugfixes:
+  - Fix collection filter/test plugin redirects (https://github.com/ansible/ansible/issues/77192).

lib/ansible/template/__init__.py

@@ -455,6 +455,7 @@ class JinjaPluginIntercept(MutableMapping):
     # FUTURE: we can cache FQ filter/test calls for the entire duration of a run, since a given collection's impl's
     # aren't supposed to change during a run
     def __getitem__(self, key):
+        original_key = key
         self._load_ansible_plugins()
 
         try:
@@ -469,56 +470,61 @@ class JinjaPluginIntercept(MutableMapping):
                 if func:
                     return func
 
-                # didn't find it in the pre-built Jinja env, assume it's a former builtin and follow the normal routing path
-                leaf_key = key
-                key = 'ansible.builtin.' + key
-            else:
-                leaf_key = key.split('.')[-1]
+            key, leaf_key = get_fqcr_and_name(key)
+            seen = set()
+
+            while True:
+                if key in seen:
+                    raise TemplateSyntaxError(
+                        'recursive collection redirect found for %r' % original_key,
+                        0
+                    )
+                seen.add(key)
 
-            acr = AnsibleCollectionRef.try_parse_fqcr(key, self._dirname)
+                acr = AnsibleCollectionRef.try_parse_fqcr(key, self._dirname)
 
-            if not acr:
-                raise KeyError('invalid plugin name: {0}'.format(key))
+                if not acr:
+                    raise KeyError('invalid plugin name: {0}'.format(key))
 
-            ts = _get_collection_metadata(acr.collection)
+                ts = _get_collection_metadata(acr.collection)
 
-            # TODO: implement support for collection-backed redirect (currently only builtin)
-            # TODO: implement cycle detection (unified across collection redir as well)
+                # TODO: implement cycle detection (unified across collection redir as well)
 
-            routing_entry = ts.get('plugin_routing', {}).get(self._dirname, {}).get(leaf_key, {})
+                routing_entry = ts.get('plugin_routing', {}).get(self._dirname, {}).get(leaf_key, {})
 
-            deprecation_entry = routing_entry.get('deprecation')
-            if deprecation_entry:
-                warning_text = deprecation_entry.get('warning_text')
-                removal_date = deprecation_entry.get('removal_date')
-                removal_version = deprecation_entry.get('removal_version')
+                deprecation_entry = routing_entry.get('deprecation')
+                if deprecation_entry:
+                    warning_text = deprecation_entry.get('warning_text')
+                    removal_date = deprecation_entry.get('removal_date')
+                    removal_version = deprecation_entry.get('removal_version')
 
-                if not warning_text:
-                    warning_text = '{0} "{1}" is deprecated'.format(self._dirname, key)
+                    if not warning_text:
+                        warning_text = '{0} "{1}" is deprecated'.format(self._dirname, key)
 
-                display.deprecated(warning_text, version=removal_version, date=removal_date, collection_name=acr.collection)
+                    display.deprecated(warning_text, version=removal_version, date=removal_date, collection_name=acr.collection)
 
-            tombstone_entry = routing_entry.get('tombstone')
+                tombstone_entry = routing_entry.get('tombstone')
 
-            if tombstone_entry:
-                warning_text = tombstone_entry.get('warning_text')
-                removal_date = tombstone_entry.get('removal_date')
-                removal_version = tombstone_entry.get('removal_version')
+                if tombstone_entry:
+                    warning_text = tombstone_entry.get('warning_text')
+                    removal_date = tombstone_entry.get('removal_date')
+                    removal_version = tombstone_entry.get('removal_version')
 
-                if not warning_text:
-                    warning_text = '{0} "{1}" has been removed'.format(self._dirname, key)
+                    if not warning_text:
+                        warning_text = '{0} "{1}" has been removed'.format(self._dirname, key)
 
-                exc_msg = display.get_deprecation_message(warning_text, version=removal_version, date=removal_date,
-                                                          collection_name=acr.collection, removed=True)
+                    exc_msg = display.get_deprecation_message(warning_text, version=removal_version, date=removal_date,
+                                                              collection_name=acr.collection, removed=True)
 
-                raise AnsiblePluginRemovedError(exc_msg)
+                    raise AnsiblePluginRemovedError(exc_msg)
 
-            redirect_fqcr = routing_entry.get('redirect', None)
-            if redirect_fqcr:
-                acr = AnsibleCollectionRef.from_fqcr(ref=redirect_fqcr, ref_type=self._dirname)
-                display.vvv('redirecting {0} {1} to {2}.{3}'.format(self._dirname, key, acr.collection, acr.resource))
-                key = redirect_fqcr
-            # TODO: handle recursive forwarding (not necessary for builtin, but definitely for further collection redirs)
+                redirect = routing_entry.get('redirect', None)
+                if redirect:
+                    next_key, leaf_key = get_fqcr_and_name(redirect, collection=acr.collection)
+                    display.vvv('redirecting (type: {0}) {1}.{2} to {3}'.format(self._dirname, acr.collection, acr.resource, next_key))
+                    key = next_key
+                else:
+                    break
 
             func = self._collection_jinja_func_cache.get(key)
 
@@ -593,6 +599,17 @@ class JinjaPluginIntercept(MutableMapping):
         return len(self._delegatee)
 
 
+def get_fqcr_and_name(resource, collection='ansible.builtin'):
+    if '.' not in resource:
+        name = resource
+        fqcr = collection + '.' + resource
+    else:
+        name = resource.split('.')[-1]
+        fqcr = resource
+
+    return fqcr, name
+
+
 @_unroll_iterator
 def _ansible_finalize(thing):
     """A custom finalize function for jinja2, which prevents None from being

test/integration/targets/collections/collection_root_user/ansible_collections/testns/testredirect/meta/runtime.yml

@@ -2,3 +2,20 @@ plugin_routing:
   modules:
     ping:
       redirect: testns.testcoll.ping
+  filter:
+    multi_redirect_filter:
+      redirect: testns.testredirect.redirect_filter1
+      deprecation:
+        warning_text: deprecation1
+    redirect_filter1:
+      redirect: redirect_filter2
+      deprecation:
+        warning_text: deprecation2
+    redirect_filter2:
+      redirect: testns.testcoll.testfilter
+      deprecation:
+        warning_text: deprecation3
+    dead_end:
+      redirect: bad_redirect
+    recursive_redirect:
+      redirect: recursive_redirect

test/integration/targets/collections/runme.sh

@@ -65,6 +65,16 @@ else
   ansible-playbook -i "${INVENTORY_PATH}" collection_root_user/ansible_collections/testns/testcoll/playbooks/default_collection_playbook.yml "$@"
 fi
 
+# test redirects and warnings for filter redirects
+echo "testing redirect and deprecation display"
+ANSIBLE_DEPRECATION_WARNINGS=yes ansible localhost -m debug -a msg='{{ "data" | testns.testredirect.multi_redirect_filter }}' -vvvvv 2>&1 | tee out.txt
+cat out.txt
+
+test "$(grep out.txt -ce 'deprecation1' -ce 'deprecation2' -ce 'deprecation3')" == 3
+grep out.txt -e 'redirecting (type: filter) testns.testredirect.multi_redirect_filter to testns.testredirect.redirect_filter1'
+grep out.txt -e 'redirecting (type: filter) testns.testredirect.redirect_filter1 to testns.testredirect.redirect_filter2'
+grep out.txt -e 'redirecting (type: filter) testns.testredirect.redirect_filter2 to testns.testcoll.testfilter'
+
 echo "--- validating collections support in playbooks/roles"
 # run test playbooks
 ansible-playbook -i "${INVENTORY_PATH}" -v "${TEST_PLAYBOOK}" "$@"

test/integration/targets/collections/test_collection_meta.yml

@@ -21,6 +21,25 @@
   # redirect filter
   - assert:
       that: ('yes' | formerly_core_filter) == True
+  # redirect filter (multiple levels)
+  - assert:
+      that: ('data' | testns.testredirect.multi_redirect_filter) == 'data_via_testfilter_from_userdir'
+  # invalid filter redirect
+  - debug: msg="{{ 'data' | testns.testredirect.dead_end }}"
+    ignore_errors: yes
+    register: redirect_failure
+  - assert:
+      that:
+        - redirect_failure is failed
+        - '"No filter named ''testns.testredirect.dead_end''" in redirect_failure.msg'
+  # recursive filter redirect
+  - debug: msg="{{ 'data' | testns.testredirect.recursive_redirect }}"
+    ignore_errors: yes
+    register: redirect_failure
+  - assert:
+      that:
+        - redirect_failure is failed
+        - '"recursive collection redirect found for ''testns.testredirect.recursive_redirect''" in redirect_failure.msg'
   # legacy filter should mask redirected
   - assert:
       that: ('' | formerly_core_masked_filter) == 'hello from overridden formerly_core_masked_filter'