Repository: Materials-Consortia/optimade-python-tools (python)

Issue:
Test for AvailableApiVersion is correct for the wrong reasons
Since the `root_validator` `crosscheck_url_and_version` has been added to the model `AvailableApiVersion`, the test `test_available_api_versions()` in `tests/models/test_models.py` succeeds for the wrong reasons.

This may be fixed by also checking the error messages.

Pull request description:
Update tests for available_api_versions
Fixes #204

Moved some of the validation for the fields "url" and "version" to the Field.
For "url", a `regex` validator cannot be added in Field, since the type "AnyHttpUrl" takes precedence, see https://pydantic-docs.helpmanual.io/usage/schema/#unenforced-field-constraints for more info.
Instead the validator is kept explicitly and the regex is added to the OpenAPI schema as `pattern`.

Note the extra part of the version validator has been removed, where it was checking whether or not each segment can be parsed as an integer.
This is now considered true always as a result of the regex validation.

**Important**: The regex for "url" currently _only_ allows the MAJOR versions `0` and `1`. I think this makes sense? It was to avoid invalid cases like `v09924`.

Patch:
diff --git a/openapi/index_openapi.json b/openapi/index_openapi.json
index 1c16c3ad..b7876f6e 100644
--- a/openapi/index_openapi.json
+++ b/openapi/index_openapi.json
@@ -250,12 +250,14 @@
             "title": "Url",
             "maxLength": 65536,
             "minLength": 1,
+            "pattern": ".+/v[0-1](\\.[0-9]+)*/?$",
             "type": "string",
             "description": "a string specifying a versioned base URL that MUST adhere to the rules in section Base URL",
             "format": "uri"
           },
           "version": {
             "title": "Version",
+            "pattern": "^[0-9]+(\\.[0-9]+){,2}$",
             "type": "string",
             "description": "a string containing the full version number of the API served at that versioned base URL. The version number string MUST NOT be prefixed by, e.g., 'v'."
           }
diff --git a/openapi/openapi.json b/openapi/openapi.json
index 3475d2b7..87988afd 100644
--- a/openapi/openapi.json
+++ b/openapi/openapi.json
@@ -918,12 +918,14 @@
             "title": "Url",
             "maxLength": 65536,
             "minLength": 1,
+            "pattern": ".+/v[0-1](\\.[0-9]+)*/?$",
             "type": "string",
             "description": "a string specifying a versioned base URL that MUST adhere to the rules in section Base URL",
             "format": "uri"
           },
           "version": {
             "title": "Version",
+            "pattern": "^[0-9]+(\\.[0-9]+){,2}$",
             "type": "string",
             "description": "a string containing the full version number of the API served at that versioned base URL. The version number string MUST NOT be prefixed by, e.g., 'v'."
           }
diff --git a/optimade/models/baseinfo.py b/optimade/models/baseinfo.py
index d2d44276..fa8f417e 100644
--- a/optimade/models/baseinfo.py
+++ b/optimade/models/baseinfo.py
@@ -16,33 +16,23 @@ class AvailableApiVersion(BaseModel):
     url: AnyHttpUrl = Field(
         ...,
         description="a string specifying a versioned base URL that MUST adhere to the rules in section Base URL",
+        pattern=r".+/v[0-1](\.[0-9]+)*/?$",
     )
 
     version: str = Field(
         ...,
         description="a string containing the full version number of the API served at that versioned base URL. "
         "The version number string MUST NOT be prefixed by, e.g., 'v'.",
+        regex=r"^[0-9]+(\.[0-9]+){,2}$",
     )
 
     @validator("url")
     def url_must_be_versioned_base_url(cls, v):
-        """The URL must be a versioned Base URL"""
-        if not re.match(r".*/v[0-9]+(\.[0-9]+)*/?", v):
+        """The URL must be a valid versioned Base URL"""
+        if not re.match(r".+/v[0-1](\.[0-9]+)*/?$", v):
             raise ValueError(f"url MUST be a versioned base URL. It is: {v}")
         return v
 
-    @validator("version")
-    def version_must_be_valid(cls, v):
-        """The version number string MUST NOT be prefixed by, e.g., 'v'"""
-        if not re.match(r"[0-9]+\.[0-9]+(\.[0-9]+)?", v):
-            raise ValueError(f"version MUST NOT be prefixed by, e.g., 'v'. It is: {v}")
-        try:
-            tuple(int(val) for val in v.split("."))
-        except ValueError:
-            raise ValueError(f"failed to parse version {v} sections as integers.")
-
-        return v
-
     @root_validator(pre=False, skip_on_failure=True)
     def crosscheck_url_and_version(cls, values):
         """ Check that URL version and API version are compatible. """
@@ -57,7 +47,6 @@ class AvailableApiVersion(BaseModel):
             raise ValueError(
                 f"API version {api_version} is not compatible with url version {url_version}."
             )
-
         return values
 
 


Test patch:
diff --git a/tests/models/test_models.py b/tests/models/test_models.py
index b159b40c..8cf973d6 100644
--- a/tests/models/test_models.py
+++ b/tests/models/test_models.py
@@ -133,36 +133,44 @@ class TestPydanticValidation(unittest.TestCase):
             with self.assertRaises(ValidationError):
                 ReferenceResource(**ReferenceMapper.map_back(ref))
 
+    def test_available_api_versions(self):
+        bad_urls = [
+            {"url": "asfdsafhttps://example.com/v0.0", "version": "0.0.0"},
+            {"url": "https://example.com/optimade", "version": "1.0.0"},
+            {"url": "https://example.com/v0999", "version": "0999.0.0"},
+            {"url": "http://example.com/v2.3", "version": "2.3.0"},
+        ]
+        good_urls = [
+            {"url": "https://example.com/v0", "version": "0.1.9"},
+            {"url": "https://example.com/v1.0.2", "version": "1.0.2"},
+            {"url": "https://example.com/optimade/v1.2", "version": "1.2.3"},
+        ]
+        bad_combos = [
+            {"url": "https://example.com/v0", "version": "1.0.0"},
+            {"url": "https://example.com/v1.0.2", "version": "1.0.3"},
+            {"url": "https://example.com/optimade/v1.2", "version": "1.3.2"},
+        ]
+
+        for data in bad_urls:
+            with pytest.raises(ValueError) as exc:
+                AvailableApiVersion(**data)
+                pytest.fail(f"Url {data['url']} should have failed")
+            self.assertTrue(
+                "url MUST be a versioned base URL" in exc.exconly()
+                or "URL scheme not permitted" in exc.exconly(),
+                msg=f"Validator 'url_must_be_versioned_base_url' not triggered as it should.\nException message: {exc.exconly()}.\nInputs: {data}",
+            )
 
-def test_available_api_versions():
-    bad_urls = [
-        "asfdsafhttps://example.com/v0.0",
-        "https://example.com/optimade",
-        "https://example.com/v0",
-        "https://example.com/v0999",
-    ]
-    good_urls = [
-        {"url": "https://example.com/v0", "version": "0.1.9"},
-        {"url": "https://example.com/v1.0.2", "version": "1.0.2"},
-        {"url": "http://example.com/v2.3", "version": "2.3.1"},
-    ]
-    bad_combos = [
-        {"url": "https://example.com/v0", "version": "1.0.0"},
-        {"url": "https://example.com/v1.0.2", "version": "1.0.3"},
-        {"url": "http://example.com/v2.3", "version": "2.0.1"},
-    ]
-
-    for url in bad_urls:
-        with pytest.raises(ValueError):
-            AvailableApiVersion(url=url, version="1.0")
-            pytest.fail(f"Url {url} should have failed")
-
-    for data in bad_combos:
-        with pytest.raises(ValueError):
-            AvailableApiVersion(**data)
-            pytest.fail(
-                f"{data['url']} should have failed with version {data['version']}"
+        for data in bad_combos:
+            with pytest.raises(ValueError) as exc:
+                AvailableApiVersion(**data)
+                pytest.fail(
+                    f"{data['url']} should have failed with version {data['version']}"
+                )
+            self.assertTrue(
+                "is not compatible with url version" in exc.exconly(),
+                msg=f"Validator 'crosscheck_url_and_version' not triggered as it should.\nException message: {exc.exconly()}.\nInputs: {data}",
             )
 
-    for data in good_urls:
-        AvailableApiVersion(**data)
+        for data in good_urls:
+            self.assertIsInstance(AvailableApiVersion(**data), AvailableApiVersion)