Repository: Materials-Consortia/optimade-python-tools

Issue:
Misleading error message from validator on failure from '/versions'
It seems from Materials-Consortia/providers#58 that the versioned `/versions`-endpoint's headers is checked, while it's not even present, and is afterwards correctly determined to return a `404 Not Found` response.

Essentially, if a `404 Not Found` response is the expected response, the validator shouldn't try to check headers and other stuff about the response (unless required by the specification).

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/670?src=pr&el=h1) Report
> Merging [#670](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/670?src=pr&el=desc) (2cccd7a) into [master](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/commit/1160b880994795492d792cd4ab667a0dcc9215cc?el=desc) (1160b88) will **decrease** coverage by `0.05%`.
> The diff coverage is `71.42%`.

[![Impacted file tree graph](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/670/graphs/tree.svg?width=650&height=150&src=pr&token=UJAtmqkZZO)](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/670?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #670      +/-   ##
==========================================
- Coverage   93.30%   93.25%   -0.06%     
==========================================
  Files          61       61              
  Lines        3300     3305       +5     
==========================================
+ Hits         3079     3082       +3     
- Misses        221      223       +2     
```

| Flag | Coverage Δ | |
|---|---|---|
| project | `93.25% <71.42%> (-0.06%)` | :arrow_down: |
| validator | `66.05% <71.42%> (-0.01%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/670?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [optimade/validator/validator.py](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/670/diff?src=pr&el=tree#diff-b3B0aW1hZGUvdmFsaWRhdG9yL3ZhbGlkYXRvci5weQ==) | `81.99% <71.42%> (-0.24%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/670?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/670?src=pr&el=footer). Last update [1160b88...2cccd7a](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/670?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

ml-evs: Any objection to me using this PR to disable the codecov status checks appearing in GH actions? We'd still get the coverage report as comment, which is more useful.

It's annoying seeing the ❌ on the main repo page for 0.005% coverage drops, I don't think they ever fixed the problems with the `threshold` parameter in codecov (namely that it doesn't work).
ml-evs: > Looks alright. Any way to test it?

Well it now passes for the netlify preview (showing the optional failure) and still passes on all implementations that already had the headers, so think we're okay.


CasperWA: > > Looks alright. Any way to test it?
> 
> Well it now passes for the netlify preview (showing the optional failure) and still passes on all implementations that already had the headers, so think we're okay.

Ah you ran it locally for the providers PR? Great!
CasperWA: You can merge from the CLI (using `--ff-only`) or rebase+merge, but I think you might lose the commit verification? No need to create a merge commit.
ml-evs: Panic reverted this to a draft, following the discussion:

https://github.com/Materials-Consortia/optimade-python-tools/issues/669#issuecomment-758090356
ml-evs: Right, reworked it slightly to allow for this mixed requirement... hopefully no major issues!
ml-evs: > Any objection to me using this PR to disable the codecov status checks appearing in GH actions? We'd still get the coverage report as comment, which is more useful.
> 
> It's annoying seeing the x on the main repo page for 0.005% coverage drops, I don't think they ever fixed the problems with the `threshold` parameter in codecov (namely that it doesn't work).

Removing the codecov changes here as #672 fixes the threshold for GH status checks
ml-evs: > Still missing a test sweat_smile (can be merged without, no worries).

I was actually writing one before I was rudely interrupted by a meeting... you'll see how convoluted it is, even for this simple endpoint, though perhaps this is the most important one to test due to the lack of pydantic models.
ml-evs: Resolved and rebased back into one commit, should be good to go! Probably worth a release after this too.

Patch:
diff --git a/optimade/validator/validator.py b/optimade/validator/validator.py
index f67dd00c..4403af64 100644
--- a/optimade/validator/validator.py
+++ b/optimade/validator/validator.py
@@ -926,8 +926,10 @@ class ImplementationValidator:
         # First, check that there is a versions endpoint in the appropriate place:
         # If passed a versioned URL, then strip that version from
         # the URL before looking for `/versions`.
+        _old_base_url = self.base_url
         if re.match(VERSIONS_REGEXP, self.base_url_parsed.path) is not None:
             self.client.base_url = "/".join(self.client.base_url.split("/")[:-1])
+            self.base_url = self.client.base_url
 
         response, _ = self._get_endpoint(
             CONF.versions_endpoint, expected_status_code=200
@@ -936,28 +938,31 @@ class ImplementationValidator:
         self._test_versions_endpoint_content(response, request=CONF.versions_endpoint)
 
         # If passed a versioned URL, first reset the URL of the client to the
-        # versioned one, then that this versioned URL does NOT host a versions endpoint
+        # versioned one, then test that this versioned URL does NOT host a versions endpoint
         if re.match(VERSIONS_REGEXP, self.base_url_parsed.path) is not None:
-            self.client.base_url = self.base_url
+            self.client.base_url = _old_base_url
+            self.base_url = _old_base_url
             self._get_endpoint(CONF.versions_endpoint, expected_status_code=404)
 
     @test_case
     def _test_versions_endpoint_content(
         self, response: requests.Response
-    ) -> Tuple[Optional[requests.Response], str]:
+    ) -> Tuple[requests.Response, str]:
         """Checks that the response from the versions endpoint complies
-        with the specification.
+        with the specification and that its 'Content-Type' header complies with
+        [RFC 4180](https://tools.ietf.org/html/rfc4180.html).
 
         Parameters:
             response: The HTTP response from the versions endpoint.
 
+        Raises:
+            ResponseError: If any content checks fail.
+
         Returns:
             The successful HTTP response or `None`, and a summary string.
 
         """
         text_content = response.text.strip().split("\n")
-        headers = response.headers
-
         if text_content[0] != "version":
             raise ResponseError(
                 f"First line of `/{CONF.versions_endpoint}` response must be 'version', not {text_content[0]}"
@@ -976,23 +981,57 @@ class ImplementationValidator:
                     f"Version numbers reported by `/{CONF.versions_endpoint}` must be integers specifying the major version, not {text_content}."
                 )
 
-        content_type = headers.get("content-type")
-        if content_type is not None:
-            content_type = [_.replace(" ", "") for _ in content_type.split(";")]
-        if not content_type or content_type[0].strip() != "text/csv":
+        content_type = response.headers.get("content-type")
+        if not content_type:
             raise ResponseError(
-                f"Incorrect content-type header {content_type} instead of 'text/csv'"
+                "Missing 'Content-Type' in response header from `/versions`."
             )
 
-        for type_parameter in content_type:
-            if type_parameter == "header=present":
-                break
-        else:
+        content_type = [_.replace(" ", "") for _ in content_type.split(";")]
+
+        self._test_versions_headers(
+            content_type, ("text/csv", "text/plain"), optional=True
+        )
+        self._test_versions_headers(content_type, "header=present", optional=True)
+
+        return response, "`/versions` endpoint responded correctly."
+
+    @test_case
+    def _test_versions_headers(
+        self,
+        content_type: Dict[str, Any],
+        expected_parameter: Union[str, List[str]],
+    ) -> Tuple[Dict[str, Any], str]:
+        """Tests that the `Content-Type` field of the `/versions` header contains
+        the passed parameter.
+
+        Arguments:
+            content_type: The 'Content-Type' field from the response of the `/versions` endpoint.
+            expected_paramter: A substring or list of substrings that are expected in
+                the Content-Type of the response. If multiple strings are passed, they will
+                be treated as possible alternatives to one another.
+
+        Raises:
+            ResponseError: If the expected 'Content-Type' parameter is missing.
+
+        Returns:
+            The HTTP response headers and a summary string.
+
+        """
+
+        if isinstance(expected_parameter, str):
+            expected_parameter = [expected_parameter]
+
+        if not any(param in content_type for param in expected_parameter):
             raise ResponseError(
-                f"Missing 'header=present' parameter in content-type {content_type}"
+                f"Incorrect 'Content-Type' header {';'.join(content_type)!r}.\n"
+                f"Missing at least one expected parameter(s): {expected_parameter!r}"
             )
 
-        return response, "`/versions` endpoint responded correctly."
+        return (
+            content_type,
+            f"`/versions` response had one of the expected Content-Type parameters {expected_parameter}.",
+        )
 
     def _test_as_type(self) -> None:
         """Tests that the base URL of the validator (i.e. with no


Test patch:
diff --git a/tests/validator/test_utils.py b/tests/validator/test_utils.py
index 5c9e983e..4e34b2db 100644
--- a/tests/validator/test_utils.py
+++ b/tests/validator/test_utils.py
@@ -367,3 +367,92 @@ def test_that_system_exit_is_fatal_in_test_case():
     assert validator.results.failure_count == 0
     assert validator.results.optional_failure_count == 0
     assert validator.results.internal_failure_count == 0
+
+
+def test_versions_test_cases():
+    """Check the `/versions` test cases."""
+    from requests import Response
+    from functools import partial
+
+    unversioned_base_url = "https://example.org"
+    versioned_base_url = unversioned_base_url + "/v1"
+
+    def monkey_patched_get_endpoint(
+        url,
+        expected_status_code=200,
+        content=b"version\n1",
+        content_type="text/csv;header=present",
+        **kwargs
+    ):
+        r = Response()
+        if expected_status_code == 200:
+            r._content = content
+            r.headers["content-type"] = content_type
+            r.status_code = 200
+        else:
+            r.status_code = 404
+
+        gets.append(url)
+
+        return r, None
+
+    # Test that the versioned base URL correctly tests the unversioned and then resets
+    validator = ImplementationValidator(base_url=versioned_base_url, verbosity=5)
+    gets = []
+    validator._get_endpoint = monkey_patched_get_endpoint
+    validator._test_versions_endpoint()
+    assert len(gets) == 2
+    assert validator.client.base_url == versioned_base_url
+    assert validator.results.success_count == 1
+    assert validator.results.optional_success_count == 2
+    assert validator.results.failure_count == 0
+
+    # Test that the unversioned base URL correctly tests just the unversioned URL
+    validator = ImplementationValidator(base_url=unversioned_base_url, verbosity=0)
+    gets = []
+    validator._get_endpoint = monkey_patched_get_endpoint
+    validator._test_versions_endpoint()
+    assert len(gets) == 1
+    assert validator.client.base_url == unversioned_base_url
+    assert validator.results.success_count == 1
+    assert validator.results.optional_success_count == 2
+    assert validator.results.failure_count == 0
+
+    # Test that bad content successfully triggers failures
+    bad_content = (b"versions\n1", b"version\n", b"version\n1.1")
+    for content in bad_content:
+        validator = ImplementationValidator(base_url=versioned_base_url, verbosity=0)
+        validator._get_endpoint = partial(monkey_patched_get_endpoint, content=content)
+        validator._test_versions_endpoint()
+        assert validator.client.base_url == versioned_base_url
+        assert validator.results.success_count == 0
+        assert validator.results.failure_count == 1
+        assert validator.results.optional_failure_count == 0
+
+    # Test that missing content-type header triggers a hard failure
+    validator = ImplementationValidator(base_url=versioned_base_url, verbosity=0)
+    validator._get_endpoint = partial(monkey_patched_get_endpoint, content_type="")
+    validator._test_versions_endpoint()
+    assert validator.client.base_url == versioned_base_url
+    assert validator.results.success_count == 0
+    assert validator.results.failure_count == 1
+    assert validator.results.optional_failure_count == 0
+
+    # Test that bad content-type headers successfully trigger *optional* failures
+    bad_content_type = (
+        ("text/csv", 1),
+        ("text/plain", 1),
+        ("header=present", 1),
+        ("x;y", 2),
+        ("application/json;charset=utf-8", 2),
+    )
+    for content_type, num_errors in bad_content_type:
+        validator = ImplementationValidator(base_url=versioned_base_url, verbosity=0)
+        validator._get_endpoint = partial(
+            monkey_patched_get_endpoint, content_type=content_type
+        )
+        validator._test_versions_endpoint()
+        assert validator.client.base_url == versioned_base_url
+        assert validator.results.success_count == 1
+        assert validator.results.failure_count == 0
+        assert validator.results.optional_failure_count == num_errors