galaxy - Fix collection install dep resolver for bad versions (#67405)

* Also make sure version is a string and not an int/float

changelogs/fragments/galaxy-collection-install-version.yaml

@@ -0,0 +1,2 @@
+bugfixes:
+- ansible-galaxy - Fix issue when compared installed dependencies with a collection having no ``MANIFEST.json`` or an empty version string in the json

lib/ansible/galaxy/collection.py

@@ -219,12 +219,15 @@ class CollectionRequirement:
                 requirement = req
                 op = operator.eq
 
-                # In the case we are checking a new requirement on a base requirement (parent != None) we can't accept
-                # version as '*' (unknown version) unless the requirement is also '*'.
-                if parent and version == '*' and requirement != '*':
-                    break
-                elif requirement == '*' or version == '*':
-                    continue
+            # In the case we are checking a new requirement on a base requirement (parent != None) we can't accept
+            # version as '*' (unknown version) unless the requirement is also '*'.
+            if parent and version == '*' and requirement != '*':
+                display.warning("Failed to validate the collection requirement '%s:%s' for %s when the existing "
+                                "install does not have a version set, the collection may not work."
+                                % (to_text(self), req, parent))
+                continue
+            elif requirement == '*' or version == '*':
+                continue
 
             if not op(LooseVersion(version), LooseVersion(requirement)):
                 break
@@ -286,7 +289,13 @@ class CollectionRequirement:
             manifest = info['manifest_file']['collection_info']
             namespace = manifest['namespace']
             name = manifest['name']
-            version = manifest['version']
+            version = to_text(manifest['version'], errors='surrogate_or_strict')
+
+            if not hasattr(LooseVersion(version), 'version'):
+                display.warning("Collection at '%s' does not have a valid version set, falling back to '*'. Found "
+                                "version: '%s'" % (to_text(b_path), version))
+                version = '*'
+
             dependencies = manifest['dependencies']
         else:
             display.warning("Collection at '%s' does not have a MANIFEST.json file, cannot detect version."
@@ -877,7 +886,7 @@ def _get_collection_info(dep_map, existing_collections, collection, requirement,
     existing = [c for c in existing_collections if to_text(c) == to_text(collection_info)]
     if existing and not collection_info.force:
         # Test that the installed collection fits the requirement
-        existing[0].add_requirement(to_text(collection_info), requirement)
+        existing[0].add_requirement(parent, requirement)
         collection_info = existing[0]
 
     dep_map[to_text(collection_info)] = collection_info

test/units/galaxy/test_collection_install.py

@@ -165,13 +165,14 @@ def test_build_requirement_from_path(collection_artifact):
     assert actual.dependencies == {}
 
 
-def test_build_requirement_from_path_with_manifest(collection_artifact):
+@pytest.mark.parametrize('version', ['1.1.1', 1.1, 1])
+def test_build_requirement_from_path_with_manifest(version, collection_artifact):
     manifest_path = os.path.join(collection_artifact[0], b'MANIFEST.json')
     manifest_value = json.dumps({
         'collection_info': {
             'namespace': 'namespace',
             'name': 'name',
-            'version': '1.1.1',
+            'version': version,
             'dependencies': {
                 'ansible_namespace.collection': '*'
             }
@@ -188,8 +189,8 @@ def test_build_requirement_from_path_with_manifest(collection_artifact):
     assert actual.b_path == collection_artifact[0]
     assert actual.api is None
     assert actual.skip is True
-    assert actual.versions == set([u'1.1.1'])
-    assert actual.latest_version == u'1.1.1'
+    assert actual.versions == set([to_text(version)])
+    assert actual.latest_version == to_text(version)
     assert actual.dependencies == {'ansible_namespace.collection': '*'}
 
 
@@ -203,6 +204,42 @@ def test_build_requirement_from_path_invalid_manifest(collection_artifact):
         collection.CollectionRequirement.from_path(collection_artifact[0], True)
 
 
+def test_build_requirement_from_path_no_version(collection_artifact, monkeypatch):
+    manifest_path = os.path.join(collection_artifact[0], b'MANIFEST.json')
+    manifest_value = json.dumps({
+        'collection_info': {
+            'namespace': 'namespace',
+            'name': 'name',
+            'version': '',
+            'dependencies': {}
+        }
+    })
+    with open(manifest_path, 'wb') as manifest_obj:
+        manifest_obj.write(to_bytes(manifest_value))
+
+    mock_display = MagicMock()
+    monkeypatch.setattr(Display, 'display', mock_display)
+
+    actual = collection.CollectionRequirement.from_path(collection_artifact[0], True)
+
+    # While the folder name suggests a different collection, we treat MANIFEST.json as the source of truth.
+    assert actual.namespace == u'namespace'
+    assert actual.name == u'name'
+    assert actual.b_path == collection_artifact[0]
+    assert actual.api is None
+    assert actual.skip is True
+    assert actual.versions == set(['*'])
+    assert actual.latest_version == u'*'
+    assert actual.dependencies == {}
+
+    assert mock_display.call_count == 1
+
+    actual_warn = ' '.join(mock_display.mock_calls[0][1][0].split('\n'))
+    expected_warn = "Collection at '%s' does not have a valid version set, falling back to '*'. Found version: ''" \
+        % to_text(collection_artifact[0])
+    assert expected_warn in actual_warn
+
+
 def test_build_requirement_from_tar(collection_artifact):
     actual = collection.CollectionRequirement.from_tar(collection_artifact[1], True, True)
 
@@ -497,13 +534,20 @@ def test_add_collection_requirements(versions, requirement, expected_filter, exp
     assert req.latest_version == expected_latest
 
 
-def test_add_collection_requirement_to_unknown_installed_version():
+def test_add_collection_requirement_to_unknown_installed_version(monkeypatch):
+    mock_display = MagicMock()
+    monkeypatch.setattr(Display, 'display', mock_display)
+
     req = collection.CollectionRequirement('namespace', 'name', None, 'https://galaxy.com', ['*'], '*', False,
                                            skip=True)
 
-    expected = "Cannot meet requirement namespace.name:1.0.0 as it is already installed at version 'unknown'."
-    with pytest.raises(AnsibleError, match=expected):
-        req.add_requirement(str(req), '1.0.0')
+    req.add_requirement('parent.collection', '1.0.0')
+    assert req.latest_version == '*'
+
+    assert mock_display.call_count == 1
+
+    actual_warn = ' '.join(mock_display.mock_calls[0][1][0].split('\n'))
+    assert "Failed to validate the collection requirement 'namespace.name:1.0.0' for parent.collection" in actual_warn
 
 
 def test_add_collection_wildcard_requirement_to_unknown_installed_version():