Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add database page limit #139

Merged
merged 3 commits into from
Jan 20, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions openapi/index_openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@
"title": "Page Limit",
"minimum": 0.0,
"type": "integer",
"default": 500
"default": 20
},
"name": "page_limit",
"in": "query"
Expand Down Expand Up @@ -291,7 +291,7 @@
"title": "Page Limit",
"minimum": 0.0,
"type": "integer",
"default": 500
"default": 20
},
"name": "page_limit",
"in": "query"
Expand Down Expand Up @@ -480,7 +480,7 @@
"title": "Page Limit",
"minimum": 0.0,
"type": "integer",
"default": 500
"default": 20
},
"name": "page_limit",
"in": "query"
Expand Down
18 changes: 9 additions & 9 deletions openapi/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@
"title": "Page Limit",
"minimum": 0.0,
"type": "integer",
"default": 500
"default": 20
},
"name": "page_limit",
"in": "query"
Expand Down Expand Up @@ -312,7 +312,7 @@
"title": "Page Limit",
"minimum": 0.0,
"type": "integer",
"default": 500
"default": 20
},
"name": "page_limit",
"in": "query"
Expand Down Expand Up @@ -553,7 +553,7 @@
"title": "Page Limit",
"minimum": 0.0,
"type": "integer",
"default": 500
"default": 20
},
"name": "page_limit",
"in": "query"
Expand Down Expand Up @@ -873,7 +873,7 @@
"title": "Page Limit",
"minimum": 0.0,
"type": "integer",
"default": 500
"default": 20
},
"name": "page_limit",
"in": "query"
Expand Down Expand Up @@ -1033,7 +1033,7 @@
"title": "Page Limit",
"minimum": 0.0,
"type": "integer",
"default": 500
"default": 20
},
"name": "page_limit",
"in": "query"
Expand Down Expand Up @@ -1274,7 +1274,7 @@
"title": "Page Limit",
"minimum": 0.0,
"type": "integer",
"default": 500
"default": 20
},
"name": "page_limit",
"in": "query"
Expand Down Expand Up @@ -1594,7 +1594,7 @@
"title": "Page Limit",
"minimum": 0.0,
"type": "integer",
"default": 500
"default": 20
},
"name": "page_limit",
"in": "query"
Expand Down Expand Up @@ -1754,7 +1754,7 @@
"title": "Page Limit",
"minimum": 0.0,
"type": "integer",
"default": 500
"default": 20
},
"name": "page_limit",
"in": "query"
Expand Down Expand Up @@ -1995,7 +1995,7 @@
"title": "Page Limit",
"minimum": 0.0,
"type": "integer",
"default": 500
"default": 20
},
"name": "page_limit",
"in": "query"
Expand Down
3 changes: 2 additions & 1 deletion optimade/server/config.ini
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ REFERENCES_COLLECTION = references
STRUCTURES_COLLECTION = structures

[SERVER]
PAGE_LIMIT = 500
PAGE_LIMIT = 20
DB_PAGE_LIMIT = 500
DEFAULT_DB = test_server

[IMPLEMENTATION]
Expand Down
15 changes: 11 additions & 4 deletions optimade/server/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,7 @@ def _DEFAULTS(field: str) -> Any:
"references_collection": "references",
"structures_collection": "structures",
"page_limit": 20,
"db_page_limit": 500,
CasperWA marked this conversation as resolved.
Show resolved Hide resolved
"default_db": "test_server",
"implementation": {
"name": "Example implementation",
Expand Down Expand Up @@ -138,6 +139,9 @@ def load_from_ini(self):
self.page_limit = config.getint(
"SERVER", "PAGE_LIMIT", fallback=self._DEFAULTS("page_limit")
)
self.db_page_limit = config.getint(
"SERVER", "DB_PAGE_LIMIT", fallback=self._DEFAULTS("db_page_limit")
)
self.default_db = config.get(
"SERVER", "DEFAULT_DB", fallback=self._DEFAULTS("default_db")
)
Expand All @@ -160,9 +164,9 @@ def load_from_ini(self):
self.provider_fields = {}
for endpoint in {"links", "references", "structures"}:
self.provider_fields[endpoint] = (
{field for field, _ in config[endpoint].items() if _ == ""}
list({field for field, _ in config[endpoint].items() if _ == ""})
if endpoint in config
else set()
else []
)

# MONGO collections
Expand Down Expand Up @@ -197,6 +201,9 @@ def load_from_json(self):
)

self.page_limit = int(config.get("page_limit", self._DEFAULTS("page_limit")))
self.db_page_limit = int(
config.get("db_page_limit", self._DEFAULTS("db_page_limit"))
)
self.default_db = config.get("default_db", self._DEFAULTS("default_db"))

# This is done in this way, since each field is OPTIONAL
Expand All @@ -212,8 +219,8 @@ def load_from_json(self):
self.provider = config.get("provider", self._DEFAULTS("provider"))
self.provider_fields = {}
for endpoint in {"structures", "references"}:
self.provider_fields[endpoint] = set(
config.get("provider_fields", {}).get(endpoint, [])
self.provider_fields[endpoint] = list(
set(config.get("provider_fields", {}).get(endpoint, []))
)


Expand Down
14 changes: 7 additions & 7 deletions optimade/server/entry_collections.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,10 +90,9 @@ def __init__(

self.provider = CONFIG.provider["prefix"]
self.provider_fields = CONFIG.provider_fields.get(resource_mapper.ENDPOINT, [])
self.page_limit = CONFIG.page_limit
self.parser = LarkParser(
version=(0, 10, 1), variant="default"
) # The MongoTransformer only supports v0.10.0 as the latest grammar
) # The MongoTransformer only supports v0.10.1 as the latest grammar

def __len__(self):
return self.collection.estimated_document_count()
Expand Down Expand Up @@ -176,15 +175,16 @@ def _parse_params(
)

if getattr(params, "page_limit", False):
limit = self.page_limit
if params.page_limit != self.page_limit:
limit = CONFIG.page_limit
if params.page_limit != CONFIG.page_limit:
CasperWA marked this conversation as resolved.
Show resolved Hide resolved
limit = params.page_limit
if limit > self.page_limit:
if limit > CONFIG.db_page_limit:
raise HTTPException(
status_code=400, detail=f"Max page_limit is {self.page_limit}"
status_code=403, # Forbidden
detail=f"Max allowed page_limit is {CONFIG.db_page_limit}, you requested {limit}",
)
if limit == 0:
limit = self.page_limit
limit = CONFIG.page_limit
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this necessary?
what if limit < 0?

If people explicitly request 0 entries, I'd say they should get 0 entries

Copy link
Member Author

@CasperWA CasperWA Jan 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if limit < 0?

This is handled by the QueryParams, i.e., all query parameter of page_* are NonNegativeInt types.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I guess we could put in another check, although it shouldn't be necessary?

cursor_kwargs["limit"] = limit

# All OPTiMaDe fields
Expand Down
2 changes: 1 addition & 1 deletion optimade/server/mappers/entries.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def all_aliases(cls) -> Tuple[Tuple[str, str]]:
return (
tuple(
(CONFIG.provider["prefix"] + field, field)
for field in CONFIG.provider_fields.get(cls.ENDPOINT, {})
for field in CONFIG.provider_fields.get(cls.ENDPOINT, [])
)
+ cls.ALIASES
)
Expand Down
64 changes: 51 additions & 13 deletions tests/server/test_server.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# pylint: disable=no-member,wrong-import-position
# pylint: disable=no-member,wrong-import-position,import-outside-toplevel
import unittest
import abc

Expand Down Expand Up @@ -336,20 +336,37 @@ def test_page_limit(self):
expected_return = 6
self._check_response(request, expected_ids, expected_return)

def test_db_page_limit(self):
from optimade.server.config import CONFIG

request = f"/structures?page_limit={CONFIG.db_page_limit + 1}"
self._check_error_response(
request,
expected_status=403,
expected_title="HTTPException",
expected_detail=f"Max allowed page_limit is {CONFIG.db_page_limit}, you requested {CONFIG.db_page_limit + 1}",
)

def test_list_has_all(self):
request = '/structures?filter=elements HAS ALL "Ba","F","H","Mn","O","Re","Si"'
self._check_not_implemented(request)
self._check_error_response(
request, expected_status=501, expected_title="NotImplementedError"
)
# expected_ids = ["mpf_3819"]
# self._check_response(request, expected_ids, len(expected_ids))

request = '/structures?filter=elements HAS ALL "Re","Ti"'
self._check_not_implemented(request)
self._check_error_response(
request, expected_status=501, expected_title="NotImplementedError"
)
# expected_ids = ["mpf_3819"]
# self._check_response(request, expected_ids, len(expected_ids))

def test_list_has_any(self):
request = '/structures?filter=elements HAS ANY "Re","Ti"'
self._check_not_implemented(request)
self._check_error_response(
request, expected_status=501, expected_title="NotImplementedError"
)
# expected_ids = ["mpf_3819"]
# self._check_response(request, expected_ids, len(expected_ids))

Expand All @@ -364,25 +381,39 @@ def test_list_length_basic(self):
def test_list_length(self):
request = "/structures?filter=elements LENGTH >= 9"
error_detail = "Operator >= not implemented for LENGTH filter."
self._check_not_implemented(request, expected_detail=error_detail)
self._check_error_response(
request,
expected_status=501,
expected_title="NotImplementedError",
expected_detail=error_detail,
)
# expected_ids = ["mpf_3819"]
# self._check_response(request, expected_ids, len(expected_ids))

request = "/structures?filter=structure_features LENGTH > 0"
error_detail = "Operator > not implemented for LENGTH filter."
self._check_not_implemented(request, expected_detail=error_detail)
self._check_error_response(
request,
expected_status=501,
expected_title="NotImplementedError",
expected_detail=error_detail,
)
# expected_ids = []
# self._check_response(request, expected_ids, len(expected_ids))

def test_list_has_only(self):
request = '/structures?filter=elements HAS ONLY "Ac"'
self._check_not_implemented(request)
self._check_error_response(
request, expected_status=501, expected_title="NotImplementedError"
)
# expected_ids = ["mpf_1"]
# self._check_response(request, expected_ids, len(expected_ids))

def test_list_correlated(self):
request = '/structures?filter=elements:elements_ratios HAS "Ag":"0.2"'
self._check_not_implemented(request)
self._check_error_response(
request, expected_status=501, expected_title="NotImplementedError"
)
# expected_ids = ["mpf_259"]
# self._check_response(request, expected_ids, len(expected_ids))

Expand Down Expand Up @@ -482,21 +513,28 @@ def _check_response(self, request, expected_ids, expected_return):
print(f"{self.client.base_url}{request}")
raise exc

def _check_not_implemented(self, request, expected_detail: str = None):
def _check_error_response(
self,
request,
expected_status: int = 500,
expected_title: str = None,
expected_detail: str = None,
):
try:
response = self.client.get(request)
self.assertEqual(
response.status_code,
501,
msg=f"Request should have failed, but did not: {response.json()}",
expected_status,
msg=f"Request should have been an error with status code {expected_status}, "
f"but instead {response.status_code} was received.\nResponse:\n{response.json()}",
)
response = response.json()
self.assertEqual(len(response["errors"]), 1)
self.assertEqual(response["meta"]["data_returned"], 0)

error = response["errors"][0]
self.assertEqual("501", error["status"])
self.assertEqual("NotImplementedError", error["title"])
self.assertEqual(str(expected_status), error["status"])
self.assertEqual(expected_title, error["title"])

if expected_detail is None:
expected_detail = "Error trying to process rule "
Expand Down