Repository: Materials-Consortia/optimade-python-tools

Issue:
Links under top-level 'links' may be objects
I am trying to validate COD implementation (in development) using `optimade-python-tools` v0.9.8. The validator seems to require the values under the top-level `links` being strings:
```
$ optimade_validator https://www.crystallography.net/cod-test/optimade -v 5
Testing entire implementation at https://www.crystallography.net/cod-test/optimade...

Mandatory tests:
[snip]
2020-07-10 14:51:48,137 - optimade.validator.validator |    DEBUG: Testing multiple entry endpoint of references
✔: https://www.crystallography.net/cod-test/optimade/references?page_limit=5 - request successful.
✖: https://www.crystallography.net/cod-test/optimade/references?page_limit=5 - deserialize_response - failed with error
[snip]
	links -> first
	  str type expected (type=type_error.str)
	links -> next
	  str type expected (type=type_error.str)
Traceback (most recent call last):
[snip]
  File "/home/andrius/.local/lib/python3.6/site-packages/optimade/validator/validator.py", line 619, in get_endpoint
    request_str = request_str.replace("\n", "")
AttributeError: 'dict' object has no attribute 'replace'
```
In the COD implementation these values are "link objects", as per OPTIMADE specification:
```json
   "links" : {
      "first" : {
         "href" : "http://www.crystallography.net/cod-test/optimade/v1.0.0/references?page_limit=5"
      },
      "next" : {
         "href" : "http://www.crystallography.net/cod-test/optimade/v1.0.0/references?page_limit=5&page_offset=5"
      },
      "base_url" : "http://www.crystallography.net/cod-test/optimade/v1.0.0/"
   }
```

Discussion:
codecov[bot]: # [Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/484?src=pr&el=h1) Report
> Merging [#484](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/484?src=pr&el=desc) into [master](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/commit/f14c67f80a837e0f4ed53937029a2c769ceab2e8?el=desc) will **not change** coverage.
> The diff coverage is `100.00%`.

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

```diff
@@           Coverage Diff           @@
##           master     #484   +/-   ##
=======================================
  Coverage   91.64%   91.64%           
=======================================
  Files          60       60           
  Lines        2848     2848           
=======================================
  Hits         2610     2610           
  Misses        238      238           
```

| Flag | Coverage Δ | |
|---|---|---|
| #unittests | `91.64% <100.00%> (ø)` | |

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/484?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [optimade/models/jsonapi.py](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/484/diff?src=pr&el=tree#diff-b3B0aW1hZGUvbW9kZWxzL2pzb25hcGkucHk=) | `91.96% <100.00%> (ø)` | |
| [optimade/models/links.py](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/484/diff?src=pr&el=tree#diff-b3B0aW1hZGUvbW9kZWxzL2xpbmtzLnB5) | `100.00% <100.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/484?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/484?src=pr&el=footer). Last update [f14c67f...f29f747](https://codecov.io/gh/Materials-Consortia/optimade-python-tools/pull/484?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

CasperWA: Should we perhaps add some tests for this - having the test try to validate a mock-response with various kinds of link types?
ml-evs: > Looks good, thanks @ml-evs! We also should remember to downstream the static schemas to OPTIMADE specification project.

I opened a PR this morning to do just that, will also add these changes to it.
ml-evs: > I originally didn't use Optional, since `None` was explicitly allowed as a value here. But fine by me to change it and make it align with all other link types.

I don't think pydantic treats `Optional` and `Union[None, ...]` differently, right? Certaintly the standard library indicates that it is just an alias as far as it is concerned, and it doesn't seem to have had an effect on the generated schema for `base_url`? If I'm wrong, and it does have an effect, that might be a way we could handle `SHOULD`, but having required keys that are allowed to be `None`...

> Should we perhaps add some tests for this - having the test try to validate a mock-response with various kinds of link types?

Yeah, fair, I'll do that.
ml-evs: > > Should we perhaps add some tests for this - having the test try to validate a mock-response with various kinds of link types?
> 
> Yeah, fair, I'll do that.

So I realised that top-level links should allow arbitrary keys (see e.g. [this response](https://www.crystallography.net/cod-test/optimade/structures?page_limit=5) from cod-test which includes `base_url` in the links response; this is actually an example in the spec). Before this PR, we were just discarding extra keys passed to this model (the pydantic default), but I've made it so that any extra key gets validated as `AnyUrl` or `jsonapi.Links` too.

Patch:
diff --git a/openapi/index_openapi.json b/openapi/index_openapi.json
index f5bc5837..6a011bc7 100644
--- a/openapi/index_openapi.json
+++ b/openapi/index_openapi.json
@@ -1665,35 +1665,63 @@
           },
           "first": {
             "title": "First",
-            "maxLength": 65536,
-            "minLength": 1,
-            "type": "string",
-            "description": "The first page of data",
-            "format": "uri"
+            "anyOf": [
+              {
+                "type": "string",
+                "minLength": 1,
+                "maxLength": 65536,
+                "format": "uri"
+              },
+              {
+                "$ref": "#/components/schemas/Link"
+              }
+            ],
+            "description": "The first page of data"
           },
           "last": {
             "title": "Last",
-            "maxLength": 65536,
-            "minLength": 1,
-            "type": "string",
-            "description": "The last page of data",
-            "format": "uri"
+            "anyOf": [
+              {
+                "type": "string",
+                "minLength": 1,
+                "maxLength": 65536,
+                "format": "uri"
+              },
+              {
+                "$ref": "#/components/schemas/Link"
+              }
+            ],
+            "description": "The last page of data"
           },
           "prev": {
             "title": "Prev",
-            "maxLength": 65536,
-            "minLength": 1,
-            "type": "string",
-            "description": "The previous page of data",
-            "format": "uri"
+            "anyOf": [
+              {
+                "type": "string",
+                "minLength": 1,
+                "maxLength": 65536,
+                "format": "uri"
+              },
+              {
+                "$ref": "#/components/schemas/Link"
+              }
+            ],
+            "description": "The previous page of data"
           },
           "next": {
             "title": "Next",
-            "maxLength": 65536,
-            "minLength": 1,
-            "type": "string",
-            "description": "The next page of data",
-            "format": "uri"
+            "anyOf": [
+              {
+                "type": "string",
+                "minLength": 1,
+                "maxLength": 65536,
+                "format": "uri"
+              },
+              {
+                "$ref": "#/components/schemas/Link"
+              }
+            ],
+            "description": "The next page of data"
           }
         },
         "description": "A set of Links objects, possibly including pagination"
diff --git a/openapi/openapi.json b/openapi/openapi.json
index dd7d43b2..cb4f49c6 100644
--- a/openapi/openapi.json
+++ b/openapi/openapi.json
@@ -3340,35 +3340,63 @@
           },
           "first": {
             "title": "First",
-            "maxLength": 65536,
-            "minLength": 1,
-            "type": "string",
-            "description": "The first page of data",
-            "format": "uri"
+            "anyOf": [
+              {
+                "type": "string",
+                "minLength": 1,
+                "maxLength": 65536,
+                "format": "uri"
+              },
+              {
+                "$ref": "#/components/schemas/Link"
+              }
+            ],
+            "description": "The first page of data"
           },
           "last": {
             "title": "Last",
-            "maxLength": 65536,
-            "minLength": 1,
-            "type": "string",
-            "description": "The last page of data",
-            "format": "uri"
+            "anyOf": [
+              {
+                "type": "string",
+                "minLength": 1,
+                "maxLength": 65536,
+                "format": "uri"
+              },
+              {
+                "$ref": "#/components/schemas/Link"
+              }
+            ],
+            "description": "The last page of data"
           },
           "prev": {
             "title": "Prev",
-            "maxLength": 65536,
-            "minLength": 1,
-            "type": "string",
-            "description": "The previous page of data",
-            "format": "uri"
+            "anyOf": [
+              {
+                "type": "string",
+                "minLength": 1,
+                "maxLength": 65536,
+                "format": "uri"
+              },
+              {
+                "$ref": "#/components/schemas/Link"
+              }
+            ],
+            "description": "The previous page of data"
           },
           "next": {
             "title": "Next",
-            "maxLength": 65536,
-            "minLength": 1,
-            "type": "string",
-            "description": "The next page of data",
-            "format": "uri"
+            "anyOf": [
+              {
+                "type": "string",
+                "minLength": 1,
+                "maxLength": 65536,
+                "format": "uri"
+              },
+              {
+                "$ref": "#/components/schemas/Link"
+              }
+            ],
+            "description": "The next page of data"
           }
         },
         "description": "A set of Links objects, possibly including pagination"
diff --git a/optimade/models/jsonapi.py b/optimade/models/jsonapi.py
index 27881bdb..34bf1e4e 100644
--- a/optimade/models/jsonapi.py
+++ b/optimade/models/jsonapi.py
@@ -6,6 +6,7 @@ from pydantic import (  # pylint: disable=no-name-in-module
     BaseModel,
     AnyUrl,
     Field,
+    parse_obj_as,
     root_validator,
 )
 
@@ -61,10 +62,33 @@ class ToplevelLinks(BaseModel):
     )
 
     # Pagination
-    first: Optional[AnyUrl] = Field(None, description="The first page of data")
-    last: Optional[AnyUrl] = Field(None, description="The last page of data")
-    prev: Optional[AnyUrl] = Field(None, description="The previous page of data")
-    next: Optional[AnyUrl] = Field(None, description="The next page of data")
+    first: Optional[Union[AnyUrl, Link]] = Field(
+        None, description="The first page of data"
+    )
+    last: Optional[Union[AnyUrl, Link]] = Field(
+        None, description="The last page of data"
+    )
+    prev: Optional[Union[AnyUrl, Link]] = Field(
+        None, description="The previous page of data"
+    )
+    next: Optional[Union[AnyUrl, Link]] = Field(
+        None, description="The next page of data"
+    )
+
+    @root_validator(pre=False)
+    def check_additional_keys_are_links(cls, values):
+        """The `ToplevelLinks` class allows any additional keys, as long as
+        they are also Links or Urls themselves.
+
+        """
+        for key, value in values.items():
+            if key not in cls.schema()["properties"]:
+                values[key] = parse_obj_as(Optional[Union[AnyUrl, Link]], value)
+
+        return values
+
+    class Config:
+        extra = "allow"
 
 
 class ErrorLinks(BaseModel):
diff --git a/optimade/models/links.py b/optimade/models/links.py
index d1e440af..c6e6c57a 100644
--- a/optimade/models/links.py
+++ b/optimade/models/links.py
@@ -47,12 +47,12 @@ class LinksResourceAttributes(Attributes):
         ...,
         description="Human-readable description for the OPTIMADE API implementation, e.g., for use in clients to show a description to the end-user.",
     )
-    base_url: Union[AnyUrl, Link, None] = Field(
+    base_url: Optional[Union[AnyUrl, Link]] = Field(
         ...,
         description="JSON API links object, pointing to the base URL for this implementation",
     )
 
-    homepage: Union[AnyUrl, Link, None] = Field(
+    homepage: Optional[Union[AnyUrl, Link]] = Field(
         ...,
         description="JSON API links object, pointing to a homepage URL for this implementation",
     )


Test patch:
diff --git a/tests/models/test_jsonapi.py b/tests/models/test_jsonapi.py
index a63f5dc3..3d684666 100644
--- a/tests/models/test_jsonapi.py
+++ b/tests/models/test_jsonapi.py
@@ -1,6 +1,57 @@
-from optimade.models.jsonapi import Error
+from pydantic import ValidationError
+import pytest
+from optimade.models.jsonapi import Error, ToplevelLinks
 
 
 def test_hashability():
     error = Error(id="test")
     assert set([error])
+
+
+def test_toplevel_links():
+    """Test top-level links responses as both URLs and Links objects,
+    and check that any arbitrary key is allowed as long as the value
+    can be validated as a URL or a Links object too.
+
+    """
+    test_links = {
+        "first": {"href": "http://example.org/structures?page_limit=3&page_offset=0"},
+        "last": {"href": "http://example.org/structures?page_limit=3&page_offset=10"},
+        "prev": {
+            "href": "http://example.org/structures?page_limit=3&page_offset=2",
+            "meta": {"description": "the previous link"},
+        },
+        "next": {
+            "href": "http://example.org/structures?page_limit=3&page_offset=3",
+            "meta": {"description": "the next link"},
+        },
+    }
+
+    # Test all defined fields as URLs and Links objects
+    for link in test_links:
+        assert ToplevelLinks(**{link: test_links[link]})
+        assert ToplevelLinks(**{link: test_links[link]["href"]})
+
+    # Allow arbitrary keys are as long as they are links
+    assert ToplevelLinks(
+        **{
+            "base_url": "https://example.org/structures",
+            "other_url": {"href": "https://example.org"},
+        }
+    )
+    assert ToplevelLinks(
+        **{
+            "base_url5": {
+                "href": "https://example.org/structures",
+                "meta": {"description": "the base URL"},
+            },
+            "none_url": None,
+        }
+    )
+
+    # Check that non-URL and non-Links objects will fail to validate
+    with pytest.raises(ValidationError):
+        ToplevelLinks(**{"base_url": {"this object": "is not a URL or a Links object"}})
+
+    with pytest.raises(ValidationError):
+        ToplevelLinks(**{"base_url": {"href": "not a link"}})