From 8c596978aa2e1245f1070dffce0a07eee1b7995b Mon Sep 17 00:00:00 2001 From: Pontus Freyhult Date: Fri, 12 Feb 2021 15:07:15 +0100 Subject: [PATCH 01/47] Change sessions to support multiple simultaneous users --- metadata_backend/api/auth.py | 50 +++++++++++------- metadata_backend/api/handlers.py | 32 ++++++----- metadata_backend/api/middlewares.py | 19 ++++++- tests/test_auth.py | 82 ++++++++++++++++++----------- tests/test_handlers.py | 9 ++++ 5 files changed, 124 insertions(+), 68 deletions(-) diff --git a/metadata_backend/api/auth.py b/metadata_backend/api/auth.py index ff1cdb3f6..8ab8fe079 100644 --- a/metadata_backend/api/auth.py +++ b/metadata_backend/api/auth.py @@ -7,7 +7,7 @@ from aiohttp import web, BasicAuth, ClientSession from aiohttp.web import Request, Response -from .middlewares import generate_cookie +from .middlewares import decrypt_cookie, generate_cookie, get_session from authlib.jose import jwt from authlib.oidc.core import CodeIDToken from authlib.jose.errors import MissingClaimError, InvalidClaimError, ExpiredTokenError, InvalidTokenError, DecodeError @@ -49,7 +49,8 @@ async def login(self, req: Request) -> Response: """ # Generate a state for callback and save it to session storage state = secrets.token_hex() - await self._save_to_session(req, key="oidc_state", value=state) + req.app["OIDC_State"].add(state) + LOG.debug("Start login") # Parameters for authorisation request params = { @@ -87,9 +88,8 @@ async def callback(self, req: Request) -> Response: LOG.error(reason) raise web.HTTPBadRequest(reason=reason) - # Verify, that states match - state = await self._get_from_session(req, "oidc_state") - if not secrets.compare_digest(str(state), str(params["state"])): + # Verify, that state is pending + if not params["state"] in req.app["OIDC_State"]: raise web.HTTPForbidden(reason="Bad user session.") auth = BasicAuth(login=self.client_id, password=self.client_secret) @@ -117,9 +117,6 @@ async def callback(self, req: Request) -> Response: LOG.error(reason) raise web.HTTPBadRequest(reason=reason) - await self._save_to_session(req, key="access_token", value=access_token) - await self._set_user(req, access_token) - response = web.HTTPSeeOther(f"{self.redirect}/home") cookie, _ = generate_cookie(req) @@ -129,8 +126,6 @@ async def callback(self, req: Request) -> Response: hashlib.sha256((cookie["id"] + cookie["referer"] + req.app["Salt"]).encode("utf-8")) ).hexdigest() - session = cookie["id"] - cookie_crypted = req.app["Crypt"].encrypt(json.dumps(cookie).encode("utf-8")).decode("utf-8") response.headers["Cache-Control"] = "no-cache, no-store, must-revalidate" @@ -147,7 +142,15 @@ async def callback(self, req: Request) -> Response: httponly=trust, # type: ignore ) - req.app["Cookies"].add(session) + # Inject cookie to this request to let _set_user work as expected. + + session_id = cookie["id"] + + req.app["Session"][session_id] = {"oidc_state": params["state"], "access_token": access_token} + req.app["Cookies"].add(session_id) + req.app["OIDC_State"].remove(params["state"]) + + await self._set_user(req, session_id, access_token) # done like this otherwise it will not redirect properly response.headers["Location"] = "/home" if self.redirect == self.domain else f"{self.redirect}/home" @@ -163,11 +166,18 @@ async def logout(self, req: Request) -> Response: :returns: HTTPSeeOther redirect to login page """ # Revoke token at AAI - req.app["Session"]["access_token"] = None - req.app["Session"]["user_info"] = None - req.app["Session"]["oidc_state"] = None - req.app["Session"] = {} - req.app["Cookies"] = set({}) + + try: + cookie = decrypt_cookie(req) + req.app["OIDC_State"].remove(req.app["Session"][cookie["id"]]["oidc_state"]) + except KeyError: + pass + + try: + cookie = decrypt_cookie(req) + req.app["Session"].pop(cookie["id"]) + except KeyError: + pass response = web.HTTPSeeOther(f"{self.redirect}/") response.headers["Location"] = "/" if self.redirect == self.domain else f"{self.redirect}/" @@ -175,7 +185,7 @@ async def logout(self, req: Request) -> Response: raise response - async def _set_user(self, req: Request, token: str) -> None: + async def _set_user(self, req: Request, session_id: str, token: str) -> None: """Set user in current session and return user id based on result of create_user. :raises: HTTPBadRequest in could not get user info from AAI OIDC @@ -202,7 +212,7 @@ async def _set_user(self, req: Request, token: str) -> None: db_client = req.app["db_client"] operator = UserOperator(db_client) user_id = await operator.create_user(user_data) - await self._save_to_session(req, key="user_info", value=user_id) + req.app["Session"][session_id]["user_info"] = user_id async def _get_key(self) -> dict: """Get OAuth2 public key and transform it to usable pem key. @@ -275,7 +285,7 @@ async def _save_to_session( """ LOG.debug(f"Save a value for {key} to session.") - session = request.app["Session"] + session = get_session(request) session[key] = value async def _get_from_session(self, req: Request, key: str) -> str: @@ -288,7 +298,7 @@ async def _get_from_session(self, req: Request, key: str) -> str: :returns: Specific value from session storage """ try: - session = req.app["Session"] + session = get_session(req) return session[key] except KeyError as e: reason = f"Session has no key {key}, error: {e}." diff --git a/metadata_backend/api/handlers.py b/metadata_backend/api/handlers.py index bd47dbef7..b5f78795a 100644 --- a/metadata_backend/api/handlers.py +++ b/metadata_backend/api/handlers.py @@ -14,6 +14,8 @@ from multidict import MultiDict, MultiDictProxy from xmlschema import XMLSchemaException +from .middlewares import decrypt_cookie, get_session + from ..conf.conf import schema_types from ..helpers.logger import LOG from ..helpers.parser import XMLToJSONParser @@ -72,7 +74,7 @@ async def _handle_check_ownedby_user(self, req: Request, collection: str, access :returns: bool """ db_client = req.app["db_client"] - current_user = req.app["Session"]["user_info"] + current_user = get_session(req)["user_info"] user_op = UserOperator(db_client) if collection != "folders": @@ -119,7 +121,7 @@ async def _handle_user_objects_collection(self, req: Request, collection: str) - :returns: List """ db_client = req.app["db_client"] - current_user = req.app["Session"]["user_info"] + current_user = get_session(req)["user_info"] user_op = UserOperator(db_client) folder_op = FolderOperator(db_client) @@ -364,7 +366,7 @@ async def delete_object(self, req: Request) -> Response: await folder_op.remove_object(folder_id, collection, accession_id) else: user_op = UserOperator(db_client) - current_user = req.app["Session"]["user_info"] + current_user = get_session(req)["user_info"] check_user = await user_op.check_user_has_doc(collection, current_user, accession_id) if check_user: await user_op.remove_objects(current_user, "drafts", [accession_id]) @@ -469,7 +471,7 @@ async def get_folders(self, req: Request) -> Response: user_operator = UserOperator(db_client) - current_user = req.app["Session"]["user_info"] + current_user = get_session(req)["user_info"] user = await user_operator.read_user(current_user) operator = FolderOperator(db_client) @@ -495,7 +497,7 @@ async def post_folder(self, req: Request) -> Response: folder = await operator.create_folder(content) user_op = UserOperator(db_client) - current_user = req.app["Session"]["user_info"] + current_user = get_session(req)["user_info"] await user_op.assign_objects(current_user, "folders", [folder]) body = json.dumps({"folderId": folder}) @@ -646,7 +648,7 @@ async def delete_folder(self, req: Request) -> Response: _folder_id = await operator.delete_folder(folder_id) user_op = UserOperator(db_client) - current_user = req.app["Session"]["user_info"] + current_user = get_session(req)["user_info"] await user_op.remove_objects(current_user, "folders", [folder_id]) LOG.info(f"DELETE folder with ID {_folder_id} was successful.") @@ -666,7 +668,7 @@ async def get_user(self, req: Request) -> Response: db_client = req.app["db_client"] operator = UserOperator(db_client) - current_user = req.app["Session"]["user_info"] + current_user = get_session(req)["user_info"] user = await operator.read_user(current_user) LOG.info(f"GET user with ID {user_id} was successful.") @@ -700,7 +702,7 @@ async def patch_user(self, req: Request) -> Response: operator = UserOperator(db_client) - current_user = req.app["Session"]["user_info"] + current_user = get_session(req)["user_info"] user = await operator.update_user(current_user, patch_ops if isinstance(patch_ops, list) else [patch_ops]) body = json.dumps({"userId": user}) @@ -723,7 +725,7 @@ async def delete_user(self, req: Request) -> Response: fold_ops = FolderOperator(db_client) obj_ops = Operator(db_client) - current_user = req.app["Session"]["user_info"] + current_user = get_session(req)["user_info"] user = await operator.read_user(current_user) for folder_id in user["folders"]: @@ -739,11 +741,13 @@ async def delete_user(self, req: Request) -> Response: await operator.delete_user(current_user) LOG.info(f"DELETE user with ID {current_user} was successful.") - req.app["Session"]["access_token"] = None - req.app["Session"]["user_info"] = None - req.app["Session"]["oidc_state"] = None - req.app["Session"] = {} - req.app["Cookies"] = set({}) + cookie = decrypt_cookie(req) + + try: + req.app["Session"].pop(cookie["id"]) + req.app["Cookies"].pop(cookie["id"]) + except KeyError: + pass response = web.HTTPSeeOther(f"{aai_config['redirect']}/") response.headers["Location"] = ( diff --git a/metadata_backend/api/middlewares.py b/metadata_backend/api/middlewares.py index 1626ffc86..f66ad0a71 100644 --- a/metadata_backend/api/middlewares.py +++ b/metadata_backend/api/middlewares.py @@ -30,6 +30,7 @@ async def http_error_handler(req: Request, handler: Callable) -> Response: except web.HTTPError as error: details = _json_exception(error.status, error, req.url) LOG.error(details) + # LOG.error(error.__traceback__.format_exc()) c_type = "application/problem+json" if error.status == 400: raise web.HTTPBadRequest(text=details, content_type=c_type) @@ -80,13 +81,15 @@ async def check_login(request: Request, handler: Callable) -> StreamResponse: ): return await handler(request) if request.path.startswith(tuple(controlled_paths)) and "OIDC_URL" in os.environ and bool(os.getenv("OIDC_URL")): - session = request.app["Session"] + cookie = decrypt_cookie(request) + session = request.app["Session"].setdefault(cookie["id"], {}) if not all(x in ["access_token", "user_info", "oidc_state"] for x in session): LOG.debug("checked session parameter") response = web.HTTPSeeOther(f"{aai_config['domain']}/aai") response.headers["Location"] = "/aai" raise response - if decrypt_cookie(request)["id"] in request.app["Cookies"]: + + if cookie["id"] in request.app["Cookies"]: LOG.debug("checked cookie session") _check_csrf(request) else: @@ -101,6 +104,18 @@ async def check_login(request: Request, handler: Callable) -> StreamResponse: return await handler(request) +def get_session(request: Request) -> dict: + """ + Return the current session for the user (derived from the cookie). + + :param request: A HTTP request instance + :returns: a dict for the session. + """ + cookie = decrypt_cookie(request) + session = request.app["Session"].setdefault(cookie["id"], {}) + return session + + def generate_cookie(request: Request) -> Tuple[dict, str]: """ Generate an encrypted and unencrypted cookie. diff --git a/tests/test_auth.py b/tests/test_auth.py index ba03e5388..198941c7d 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -3,14 +3,20 @@ from metadata_backend.api.auth import AccessHandler from unittest.mock import MagicMock, patch from aiohttp.test_utils import AioHTTPTestCase, unittest_run_loop +from metadata_backend.api.middlewares import generate_cookie from metadata_backend.server import init -from .mockups import Mock_Request, MockResponse, jwt_data, jwk_data, jwt_data_claim_miss, jwt_data_bad_nonce +from .mockups import ( + Mock_Request, + MockResponse, + get_request_with_fernet, + jwt_data, + jwk_data, + jwt_data_claim_miss, + jwt_data_bad_nonce, +) from aiounittest import AsyncTestCase, futurized import json -import cryptography.fernet -import hashlib -from os import urandom class AccessHandlerFailTestCase(AioHTTPTestCase): @@ -37,8 +43,8 @@ async def test_login_with_default_config_values(self): self.assertEqual(response.status, 404) resp_json = await response.json() self.assertEqual(resp_json["instance"], "/authorize") - # Also check oidc_state is saved to session storage - self.assertIn("oidc_state", self.client.app["Session"]) + # Also check that we have regisitered oidc state + self.assertEqual(1, len(self.client.app["OIDC_State"])) @unittest_run_loop async def test_callback_fails_without_query_params(self): @@ -60,15 +66,19 @@ async def test_callback_fails_with_wrong_oidc_state(self): @unittest_run_loop async def test_callback_(self): """Test that callback.""" - self.client.app["Session"] = {"oidc_state": "mock_oidc_state_value"} - response = await self.client.get("/callback?state=mock_oidc_state_value&code=code") - self.assertEqual(response.status, 500) + self.client.app["OIDC_State"] = set("mo_state_value") + response = await self.client.get("/callback?state=mo_state_value&code=code") + self.assertIn(response.status, (403, 500)) @unittest_run_loop async def test_logout_works(self): """Test that logout revokes all tokens.""" - self.client.app["Session"] = {"access_token": "mock_token_value"} - response = await self.client.get("/logout") + + request = get_request_with_fernet() + request.app["Crypt"] = self.client.app["Crypt"] + cookie, cookiestring = generate_cookie(request) + self.client.app["Session"] = {cookie["id"]: {"access_token": "mock_token_value"}} + response = await self.client.get("/logout", cookies={"MTD_SESSION": cookiestring}) self.assertEqual(response.status, 404) self.assertEqual(self.client.app["Session"], {}) self.assertEqual(self.client.app["Cookies"], set()) @@ -103,7 +113,10 @@ def tearDown(self): async def test_get_key_value_from_session_fail(self): """Test retrieving key value pair from session exceptions.""" - request = Mock_Request() + request = get_request_with_fernet() + _, cookiestring = generate_cookie(request) + request.cookies["MTD_SESSION"] = cookiestring + with self.assertRaises(HTTPUnauthorized): await self.AccessHandler._get_from_session(request, "mock_value") @@ -133,15 +146,19 @@ async def test_jwk_key(self): async def test_set_user_fail(self): """Test set user raises exception.""" request = Mock_Request() - tk = "something" + tk = ("something",) + session_id = "session_id" with self.assertRaises(HTTPBadRequest): - await self.AccessHandler._set_user(request, tk) + await self.AccessHandler._set_user(request, session_id, tk) async def test_set_user(self): """Test set user success.""" - request = Mock_Request() + request = get_request_with_fernet() + session_id = "session_id" + new_user_id = "USR12345" + request.app["db_client"] = MagicMock() - request.app["Session"] = {} + request.app["Session"] = {session_id: {}} tk = "something" data = { "eppn": "eppn@test.fi", @@ -151,15 +168,19 @@ async def test_set_user(self): resp = MockResponse(data, 200) with patch("aiohttp.ClientSession.get", return_value=resp): - with patch("metadata_backend.api.operators.UserOperator.create_user", return_value=futurized("USR12345")): - await self.AccessHandler._set_user(request, tk) + with patch("metadata_backend.api.operators.UserOperator.create_user", return_value=futurized(new_user_id)): + await self.AccessHandler._set_user(request, session_id, tk) + + self.assertIn("user_info", request.app["Session"][session_id]) + self.assertEqual(new_user_id, request.app["Session"][session_id]["user_info"]) async def test_callback_fail(self): """Test callback fails.""" - request = Mock_Request() + request = get_request_with_fernet() request.query["state"] = "state" request.query["code"] = "code" - request.app["Session"] = {"oidc_state": "state"} + request.app["Session"] = {} + request.app["OIDC_State"] = set(("state",)) resp_no_token = MockResponse({}, 200) resp_400 = MockResponse({}, 400) @@ -173,13 +194,12 @@ async def test_callback_fail(self): async def test_callback_pass(self): """Test callback correct validation.""" - request = Mock_Request() + request = get_request_with_fernet() request.query["state"] = "state" request.query["code"] = "code" - request.app["Session"] = {"oidc_state": "state"} + request.app["Session"] = {} request.app["Cookies"] = set({}) - request.app["Crypt"] = cryptography.fernet.Fernet(cryptography.fernet.Fernet.generate_key()) - request.app["Salt"] = hashlib.sha256(urandom(512)).hexdigest() + request.app["OIDC_State"] = set(("state",)) resp_token = MockResponse(jwt_data, 200) resp_jwk = MockResponse(jwk_data, 200) @@ -191,13 +211,12 @@ async def test_callback_pass(self): async def test_callback_missing_claim(self): """Test callback missing claim validation.""" - request = Mock_Request() + request = get_request_with_fernet() request.query["state"] = "state" request.query["code"] = "code" - request.app["Session"] = {"oidc_state": "state"} + request.app["Session"] = {} request.app["Cookies"] = set({}) - request.app["Crypt"] = cryptography.fernet.Fernet(cryptography.fernet.Fernet.generate_key()) - request.app["Salt"] = hashlib.sha256(urandom(512)).hexdigest() + request.app["OIDC_State"] = set(("state",)) resp_token = MockResponse(jwt_data_claim_miss, 200) resp_jwk = MockResponse(jwk_data, 200) @@ -210,13 +229,12 @@ async def test_callback_missing_claim(self): async def test_callback_bad_claim(self): """Test callback bad nonce validation.""" - request = Mock_Request() + request = get_request_with_fernet() request.query["state"] = "state" request.query["code"] = "code" - request.app["Session"] = {"oidc_state": "state"} + request.app["OIDC_State"] = set() + request.app["Session"] = {} request.app["Cookies"] = set({}) - request.app["Crypt"] = cryptography.fernet.Fernet(cryptography.fernet.Fernet.generate_key()) - request.app["Salt"] = hashlib.sha256(urandom(512)).hexdigest() resp_token = MockResponse(jwt_data_bad_nonce, 200) resp_jwk = MockResponse(jwk_data, 200) diff --git a/tests/test_handlers.py b/tests/test_handlers.py index b50548c14..134690317 100644 --- a/tests/test_handlers.py +++ b/tests/test_handlers.py @@ -7,6 +7,8 @@ from aiohttp.test_utils import AioHTTPTestCase, unittest_run_loop from aiounittest import futurized +from metadata_backend.api.middlewares import generate_cookie +from .mockups import get_request_with_fernet from metadata_backend.server import init @@ -101,6 +103,13 @@ async def setUpAsync(self): self.MockedFolderOperator = self.patch_folderoperator.start() self.MockedUserOperator = self.patch_useroperator.start() + # Set up authentication + request = get_request_with_fernet() + request.app["Crypt"] = self.client.app["Crypt"] + cookie, cookiestring = generate_cookie(request) + self.client.app["Session"] = {cookie["id"]: {"access_token": "mock_token_value", "user_info": {}}} + self.client._session.cookie_jar.update_cookies({"MTD_SESSION": cookiestring}) + async def tearDownAsync(self): """Cleanup mocked stuff.""" self.patch_parser.stop() From 63eb13fe619a0fe33130e156fc1b7b095710ff1c Mon Sep 17 00:00:00 2001 From: Pontus Freyhult Date: Fri, 12 Feb 2021 15:28:23 +0100 Subject: [PATCH 02/47] Fix tests that are skipped in some cases --- tests/test_auth.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index 198941c7d..065f043c8 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -57,7 +57,8 @@ async def test_callback_fails_without_query_params(self): @unittest_run_loop async def test_callback_fails_with_wrong_oidc_state(self): """Test that callback endpoint raises 403 when state in the query is not the same as specified in session.""" - self.client.app["Session"] = {"oidc_state": "mock_oidc_state_value"} + self.client.app["Session"] = {} + self.client.app["OIDC_State"] = set() response = await self.client.get("/callback?state=wrong_value&code=code") self.assertEqual(response.status, 403) resp_json = await response.json() From 96333e751c0513702e6e3054c5570a10c8525286 Mon Sep 17 00:00:00 2001 From: Pontus Freyhult Date: Fri, 12 Feb 2021 15:36:26 +0100 Subject: [PATCH 03/47] Fix more tests that are skipped in some cases --- tests/test_auth.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/test_auth.py b/tests/test_auth.py index 065f043c8..87b5072b4 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -39,6 +39,7 @@ async def tearDownAsync(self): @unittest_run_loop async def test_login_with_default_config_values(self): """Test that login raises 404 when the AUTH_URL env variable is not a proper endpoint.""" + self.client.app["OIDC_State"] = set() response = await self.client.get("/aai") self.assertEqual(response.status, 404) resp_json = await response.json() @@ -67,7 +68,7 @@ async def test_callback_fails_with_wrong_oidc_state(self): @unittest_run_loop async def test_callback_(self): """Test that callback.""" - self.client.app["OIDC_State"] = set("mo_state_value") + self.client.app["OIDC_State"] = set(("mo_state_value",)) response = await self.client.get("/callback?state=mo_state_value&code=code") self.assertIn(response.status, (403, 500)) From dd566aa05906742b80866cb92305669824fafaf0 Mon Sep 17 00:00:00 2001 From: Pontus Freyhult Date: Mon, 15 Feb 2021 09:01:50 +0100 Subject: [PATCH 04/47] Add new store OIDC_State for ongoing authentication requests --- metadata_backend/server.py | 1 + 1 file changed, 1 insertion(+) diff --git a/metadata_backend/server.py b/metadata_backend/server.py index ce32ac003..8066ef63c 100644 --- a/metadata_backend/server.py +++ b/metadata_backend/server.py @@ -53,6 +53,7 @@ async def init() -> web.Application: server["Salt"] = secrets.token_hex(64) server["Session"] = {} server["Cookies"] = set({}) + server["OIDC_State"] = set({}) server.middlewares.append(http_error_handler) server.middlewares.append(check_login) From 1ad598f73fd59f7a0fc99fc899e703d117708c1f Mon Sep 17 00:00:00 2001 From: Pontus Freyhult Date: Mon, 15 Feb 2021 11:24:03 +0100 Subject: [PATCH 05/47] Use correct operator to remove item from set --- metadata_backend/api/handlers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/metadata_backend/api/handlers.py b/metadata_backend/api/handlers.py index b5f78795a..00a11f757 100644 --- a/metadata_backend/api/handlers.py +++ b/metadata_backend/api/handlers.py @@ -745,7 +745,7 @@ async def delete_user(self, req: Request) -> Response: try: req.app["Session"].pop(cookie["id"]) - req.app["Cookies"].pop(cookie["id"]) + req.app["Cookies"].remove(cookie["id"]) except KeyError: pass From 45a01129c6420852c882fe51ac7cfbb94fa66700 Mon Sep 17 00:00:00 2001 From: Pontus Freyhult Date: Mon, 15 Feb 2021 18:55:11 +0100 Subject: [PATCH 06/47] Remove obsoleted functions to save to/get from session --- metadata_backend/api/auth.py | 39 ++---------------------------------- tests/test_auth.py | 12 ----------- 2 files changed, 2 insertions(+), 49 deletions(-) diff --git a/metadata_backend/api/auth.py b/metadata_backend/api/auth.py index 8ab8fe079..f47a50de0 100644 --- a/metadata_backend/api/auth.py +++ b/metadata_backend/api/auth.py @@ -7,14 +7,14 @@ from aiohttp import web, BasicAuth, ClientSession from aiohttp.web import Request, Response -from .middlewares import decrypt_cookie, generate_cookie, get_session +from .middlewares import decrypt_cookie, generate_cookie from authlib.jose import jwt from authlib.oidc.core import CodeIDToken from authlib.jose.errors import MissingClaimError, InvalidClaimError, ExpiredTokenError, InvalidTokenError, DecodeError from multidict import CIMultiDict from .operators import UserOperator -from typing import Dict, Tuple, Union +from typing import Dict, Tuple from ..helpers.logger import LOG @@ -273,38 +273,3 @@ async def _validate_jwt(self, token: str) -> None: raise web.HTTPUnauthorized(reason=f"Invalid JWT format: {e}") except Exception: raise web.HTTPForbidden(reason="No access") - - async def _save_to_session( - self, request: Request, key: str = "key", value: Union[Tuple[str, str], str] = "value" - ) -> None: - """Save a given value to a session key. - - :param request: HTTP request - :param key: key to identify in the session storage - :param value: value for the key stored the session storage - """ - LOG.debug(f"Save a value for {key} to session.") - - session = get_session(request) - session[key] = value - - async def _get_from_session(self, req: Request, key: str) -> str: - """Get a value from session storage. - - :param req: HTTP request - :param key: name of the key to be returned from session storage - :raises: HTTPUnauthorized in case session does not have value for key - :raises: HTTPForbidden in case session does not have key - :returns: Specific value from session storage - """ - try: - session = get_session(req) - return session[key] - except KeyError as e: - reason = f"Session has no key {key}, error: {e}." - LOG.error(reason) - raise web.HTTPUnauthorized(reason=reason) - except Exception as e: - reason = f"Failed to retrieve {key} from session: {e}" - LOG.error(reason) - raise web.HTTPForbidden(reason=reason) diff --git a/tests/test_auth.py b/tests/test_auth.py index 87b5072b4..f6cff5a99 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -113,18 +113,6 @@ def tearDown(self): """Cleanup mocked stuff.""" pass - async def test_get_key_value_from_session_fail(self): - """Test retrieving key value pair from session exceptions.""" - request = get_request_with_fernet() - _, cookiestring = generate_cookie(request) - request.cookies["MTD_SESSION"] = cookiestring - - with self.assertRaises(HTTPUnauthorized): - await self.AccessHandler._get_from_session(request, "mock_value") - - with self.assertRaises(HTTPForbidden): - await self.AccessHandler._get_from_session("request", "mock_value") - async def test_get_jwk_fail(self): """Test retrieving JWK exception.""" with self.assertRaises(HTTPUnauthorized): From 9daa78b3c75645d4210481626ed36e0527c3f1f6 Mon Sep 17 00:00:00 2001 From: Pontus Freyhult Date: Tue, 16 Feb 2021 10:59:08 +0100 Subject: [PATCH 07/47] Remove obsolete debug code --- metadata_backend/api/middlewares.py | 1 - 1 file changed, 1 deletion(-) diff --git a/metadata_backend/api/middlewares.py b/metadata_backend/api/middlewares.py index f66ad0a71..16d2b57d7 100644 --- a/metadata_backend/api/middlewares.py +++ b/metadata_backend/api/middlewares.py @@ -30,7 +30,6 @@ async def http_error_handler(req: Request, handler: Callable) -> Response: except web.HTTPError as error: details = _json_exception(error.status, error, req.url) LOG.error(details) - # LOG.error(error.__traceback__.format_exc()) c_type = "application/problem+json" if error.status == 400: raise web.HTTPBadRequest(text=details, content_type=c_type) From b7994c3457447ec2da5429b65862ac67db83c929 Mon Sep 17 00:00:00 2001 From: Joakim Bygdell Date: Fri, 5 Feb 2021 14:46:05 +0100 Subject: [PATCH 08/47] Use proper ENVs for the mongodb credentials --- README.md | 4 ++-- metadata_backend/conf/conf.py | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/README.md b/README.md index e16c9d21e..a79468977 100644 --- a/README.md +++ b/README.md @@ -22,8 +22,8 @@ For more detailed setup, do following: - Install project by running: `pip install .` in project root - Setup mongodb and env variables via desired way, details: - Server expects to find mongodb instance running, specified with following environment variables: - - `MONGO_INITDB_ROOT_USERNAME`, username for admin user to mondogdb instance - - `MONGO_INITDB_ROOT_PASSWORD`, password for admin user to mondogdb instance + - `MONGO_USERNAME`, username for connecting to mondogdb instance + - `MONGO_PASSWORD`, password for connecting to mondogdb instance - `MONGO_HOST`, host and port for mongodb instance (e.g. `localhost:27017`) - Out of the box, metadata submitter is configured with default values from MongoDB Docker image - Suitable mongodb instance can be launched with Docker by running `docker-compose up database` diff --git a/metadata_backend/conf/conf.py b/metadata_backend/conf/conf.py index 8718ebe9a..7609acf66 100644 --- a/metadata_backend/conf/conf.py +++ b/metadata_backend/conf/conf.py @@ -5,8 +5,8 @@ MongoDB. Currently in use: -- ``MONGO_INITDB_ROOT_USERNAME`` - Admin username for mongodb -- ``MONGO_INITDB_ROOT_PASSWORD`` - Admin password for mongodb +- ``MONGO_USERNAME`` - Username for mongodb +- ``MONGO_PASSWORD`` - Password for mongodb - ``MONGO_HOST`` - Mongodb server hostname, with port specified Admin access is needed in order to create new databases during runtime. @@ -43,8 +43,8 @@ # Set custom timeouts and other parameters here so they can be imported to # other modules if needed. -mongo_user = os.getenv("MONGO_INITDB_ROOT_USERNAME", "admin") -mongo_password = os.getenv("MONGO_INITDB_ROOT_PASSWORD", "admin") +mongo_user = os.getenv("MONGO_USERNAME", "admin") +mongo_password = os.getenv("MONGO_PASSWORD", "admin") mongo_host = os.getenv("MONGO_HOST", "localhost:27017") mongo_authdb = os.getenv("MONGO_AUTHDB", "") _base = f"mongodb://{mongo_user}:{mongo_password}@{mongo_host}/{mongo_authdb}" From 4e48f0822b5d6c221bd65b5ce117369fb039f87f Mon Sep 17 00:00:00 2001 From: Joakim Bygdell Date: Fri, 5 Feb 2021 15:13:25 +0100 Subject: [PATCH 09/47] Use a dedicated database for all metadata --- README.md | 2 ++ metadata_backend/api/operators.py | 10 +++++----- metadata_backend/conf/conf.py | 19 +++++++++++++++++-- 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/README.md b/README.md index a79468977..d97437d26 100644 --- a/README.md +++ b/README.md @@ -25,6 +25,8 @@ For more detailed setup, do following: - `MONGO_USERNAME`, username for connecting to mondogdb instance - `MONGO_PASSWORD`, password for connecting to mondogdb instance - `MONGO_HOST`, host and port for mongodb instance (e.g. `localhost:27017`) + - `MONGO_DATABASE`, If a specific database is to be used, set the name here. + - `MONGO_AUTHDB`, if `MONGO_DATABASE` is set and the user doesn't exists in the database, set this to the database where the user exists (e.g. `admin`) - Out of the box, metadata submitter is configured with default values from MongoDB Docker image - Suitable mongodb instance can be launched with Docker by running `docker-compose up database` - After installing and setting up database, server can be launched with `metadata_submitter` diff --git a/metadata_backend/api/operators.py b/metadata_backend/api/operators.py index fd10ada8d..242b264b9 100644 --- a/metadata_backend/api/operators.py +++ b/metadata_backend/api/operators.py @@ -11,7 +11,7 @@ from multidict import MultiDictProxy from pymongo.errors import ConnectionFailure, OperationFailure -from ..conf.conf import query_map +from ..conf.conf import query_map, mongo_database from ..database.db_service import DBService, auto_reconnect from ..helpers.logger import LOG from ..helpers.parser import XMLToJSONParser @@ -297,7 +297,7 @@ def __init__(self, db_client: AsyncIOMotorClient) -> None: running on same loop with aiohttp, so needs to be passed from aiohttp Application. """ - super().__init__("objects", "application/json", db_client) + super().__init__(mongo_database, "application/json", db_client) async def query_metadata_database( self, schema_type: str, que: MultiDictProxy, page_num: int, page_size: int, filter_objects: List @@ -517,7 +517,7 @@ def __init__(self, db_client: AsyncIOMotorClient) -> None: running on same loop with aiohttp, so needs to be passed from aiohttp Application. """ - super().__init__("backups", "text/xml", db_client) + super().__init__(mongo_database, "text/xml", db_client) async def _format_data_to_create_and_add_to_db(self, schema_type: str, data: str) -> str: """Format XML metadata object and add it to db. @@ -596,7 +596,7 @@ def __init__(self, db_client: AsyncIOMotorClient) -> None: running on same loop with aiohttp, so needs to be passed from aiohttp Application. """ - self.db_service = DBService("folders", db_client) + self.db_service = DBService(mongo_database, db_client) async def check_object_in_folder(self, collection: str, accession_id: str) -> Tuple[bool, str, bool]: """Check a object/draft is in a folder. @@ -824,7 +824,7 @@ def __init__(self, db_client: AsyncIOMotorClient) -> None: running on same loop with aiohttp, so needs to be passed from aiohttp Application. """ - self.db_service = DBService("users", db_client) + self.db_service = DBService(mongo_database, db_client) async def check_user_has_doc(self, collection: str, user_id: str, accession_id: str) -> bool: """Check a folder/draft belongs to user. diff --git a/metadata_backend/conf/conf.py b/metadata_backend/conf/conf.py index 7609acf66..7583bb390 100644 --- a/metadata_backend/conf/conf.py +++ b/metadata_backend/conf/conf.py @@ -46,17 +46,32 @@ mongo_user = os.getenv("MONGO_USERNAME", "admin") mongo_password = os.getenv("MONGO_PASSWORD", "admin") mongo_host = os.getenv("MONGO_HOST", "localhost:27017") -mongo_authdb = os.getenv("MONGO_AUTHDB", "") -_base = f"mongodb://{mongo_user}:{mongo_password}@{mongo_host}/{mongo_authdb}" +mongo_database = os.getenv("MONGO_DATABASE", "") +_base = f"mongodb://{mongo_user}:{mongo_password}@{mongo_host}/{mongo_database}" if bool(os.getenv("MONGO_SSL", None)): _ca = os.getenv("MONGO_SSL_CA", None) _key = os.getenv("MONGO_SSL_CLIENT_KEY", None) _cert = os.getenv("MONGO_SSL_CLIENT_CERT", None) tls = f"?tls=true&tlsCAFile={_ca}&ssl_keyfile={_key}&ssl_certfile={_cert}" url = f"{_base}{tls}" +elif bool(os.getenv("MONGO_SSL", None)) and os.getenv("MONGO_AUTHDB", "") != "": + _ca = os.getenv("MONGO_SSL_CA", None) + _key = os.getenv("MONGO_SSL_CLIENT_KEY", None) + _cert = os.getenv("MONGO_SSL_CLIENT_CERT", None) + tls = f"?tls=true&tlsCAFile={_ca}&ssl_keyfile={_key}&ssl_certfile={_cert}" + _authdb = os.getenv("MONGO_AUTHDB", "") + auth = f"&authSource={_authdb}" + url = f"{_base}{tls}{auth}" +elif os.getenv("MONGO_AUTHDB", "") != "": + _authdb = os.getenv("MONGO_AUTHDB", "") + auth = f"?authSource={_authdb}" + url = f"{_base}{auth}" else: url = _base +if os.getenv("MONGO_DATABASE", "") == "": + mongo_database = "test" + LOG.debug(f"mongodb connection string is {url}") serverTimeout = 15000 From 9fffb493828535f7701c6491660cb4647221f3ee Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Wed, 10 Feb 2021 18:46:37 +0200 Subject: [PATCH 10/47] fix reading xml from backup-collection --- metadata_backend/api/handlers.py | 3 ++- metadata_backend/api/operators.py | 10 ++++++---- tests/test_operators.py | 6 ++++-- 3 files changed, 12 insertions(+), 7 deletions(-) diff --git a/metadata_backend/api/handlers.py b/metadata_backend/api/handlers.py index 00a11f757..673c2d029 100644 --- a/metadata_backend/api/handlers.py +++ b/metadata_backend/api/handlers.py @@ -270,6 +270,7 @@ async def get_object(self, req: Request) -> Response: req_format = req.query.get("format", "json").lower() db_client = req.app["db_client"] operator = XMLOperator(db_client) if req_format == "xml" else Operator(db_client) + type_collection = f"backup-{collection}" if req_format == "xml" else collection await operator.check_exists(collection, accession_id) @@ -279,7 +280,7 @@ async def get_object(self, req: Request) -> Response: LOG.error(reason) raise web.HTTPUnauthorized(reason=reason) - data, content_type = await operator.read_metadata_object(collection, accession_id) + data, content_type = await operator.read_metadata_object(type_collection, accession_id) data = data if req_format == "xml" else json.dumps(data) LOG.info(f"GET object with accesssion ID {accession_id} from schema {collection}.") diff --git a/metadata_backend/api/operators.py b/metadata_backend/api/operators.py index 242b264b9..c2dcdcf06 100644 --- a/metadata_backend/api/operators.py +++ b/metadata_backend/api/operators.py @@ -534,8 +534,10 @@ async def _format_data_to_create_and_add_to_db(self, schema_type: str, data: str schema = schema_type[6:] if schema_type.startswith("draft") else schema_type data_as_json = XMLToJSONParser().parse(schema, data) accession_id = await Operator(db_client)._format_data_to_create_and_add_to_db(schema_type, data_as_json) - LOG.debug(f"XMLOperator formatted data for {schema_type} to add to DB") - return await self._insert_formatted_object_to_db(schema_type, {"accessionId": accession_id, "content": data}) + LOG.debug(f"XMLOperator formatted data for backup-{schema_type} to add to DB") + return await self._insert_formatted_object_to_db( + f"backup-{schema_type}", {"accessionId": accession_id, "content": data} + ) async def _format_data_to_replace_and_add_to_db(self, schema_type: str, accession_id: str, data: str) -> str: """Format XML metadata object and add it to db. @@ -555,9 +557,9 @@ async def _format_data_to_replace_and_add_to_db(self, schema_type: str, accessio accession_id = await Operator(db_client)._format_data_to_replace_and_add_to_db( schema_type, accession_id, data_as_json ) - LOG.debug(f"XMLOperator formatted data for {schema_type} to add to DB") + LOG.debug(f"XMLOperator formatted data for backup-{schema_type} to add to DB") return await self._replace_object_from_db( - schema_type, accession_id, {"accessionId": accession_id, "content": data} + f"backup-{schema_type}", accession_id, {"accessionId": accession_id, "content": data} ) async def _format_data_to_update_and_add_to_db(self, schema_type: str, accession_id: str, data: str) -> str: diff --git a/tests/test_operators.py b/tests/test_operators.py index bc1047eee..5796a20a4 100644 --- a/tests/test_operators.py +++ b/tests/test_operators.py @@ -375,7 +375,9 @@ async def test_correct_data_is_set_to_xml_when_creating(self): ) as m_insert: with patch("metadata_backend.api.operators.XMLToJSONParser"): acc = await (operator._format_data_to_create_and_add_to_db("study", xml_data)) - m_insert.assert_called_once_with("study", {"accessionId": self.accession_id, "content": xml_data}) + m_insert.assert_called_once_with( + "backup-study", {"accessionId": self.accession_id, "content": xml_data} + ) self.assertEqual(acc, self.accession_id) async def test_correct_data_is_set_to_xml_when_replacing(self): @@ -394,7 +396,7 @@ async def test_correct_data_is_set_to_xml_when_replacing(self): with patch("metadata_backend.api.operators.XMLToJSONParser"): acc = await (operator._format_data_to_replace_and_add_to_db("study", self.accession_id, xml_data)) m_insert.assert_called_once_with( - "study", + "backup-study", self.accession_id, {"accessionId": self.accession_id, "content": xml_data}, ) From 8f981cfe8671336bb96e3a101bfa849ae7b32290 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Wed, 3 Mar 2021 10:56:12 +0200 Subject: [PATCH 11/47] rename xml-colletion to `xml-` instead of backup --- metadata_backend/api/handlers.py | 2 +- metadata_backend/api/operators.py | 8 ++++---- tests/test_operators.py | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/metadata_backend/api/handlers.py b/metadata_backend/api/handlers.py index 673c2d029..8ea339443 100644 --- a/metadata_backend/api/handlers.py +++ b/metadata_backend/api/handlers.py @@ -270,7 +270,7 @@ async def get_object(self, req: Request) -> Response: req_format = req.query.get("format", "json").lower() db_client = req.app["db_client"] operator = XMLOperator(db_client) if req_format == "xml" else Operator(db_client) - type_collection = f"backup-{collection}" if req_format == "xml" else collection + type_collection = f"xml-{collection}" if req_format == "xml" else collection await operator.check_exists(collection, accession_id) diff --git a/metadata_backend/api/operators.py b/metadata_backend/api/operators.py index c2dcdcf06..303d5fc01 100644 --- a/metadata_backend/api/operators.py +++ b/metadata_backend/api/operators.py @@ -534,9 +534,9 @@ async def _format_data_to_create_and_add_to_db(self, schema_type: str, data: str schema = schema_type[6:] if schema_type.startswith("draft") else schema_type data_as_json = XMLToJSONParser().parse(schema, data) accession_id = await Operator(db_client)._format_data_to_create_and_add_to_db(schema_type, data_as_json) - LOG.debug(f"XMLOperator formatted data for backup-{schema_type} to add to DB") + LOG.debug(f"XMLOperator formatted data for xml-{schema_type} to add to DB") return await self._insert_formatted_object_to_db( - f"backup-{schema_type}", {"accessionId": accession_id, "content": data} + f"xml-{schema_type}", {"accessionId": accession_id, "content": data} ) async def _format_data_to_replace_and_add_to_db(self, schema_type: str, accession_id: str, data: str) -> str: @@ -557,9 +557,9 @@ async def _format_data_to_replace_and_add_to_db(self, schema_type: str, accessio accession_id = await Operator(db_client)._format_data_to_replace_and_add_to_db( schema_type, accession_id, data_as_json ) - LOG.debug(f"XMLOperator formatted data for backup-{schema_type} to add to DB") + LOG.debug(f"XMLOperator formatted data for xml-{schema_type} to add to DB") return await self._replace_object_from_db( - f"backup-{schema_type}", accession_id, {"accessionId": accession_id, "content": data} + f"xml-{schema_type}", accession_id, {"accessionId": accession_id, "content": data} ) async def _format_data_to_update_and_add_to_db(self, schema_type: str, accession_id: str, data: str) -> str: diff --git a/tests/test_operators.py b/tests/test_operators.py index 5796a20a4..c4788c557 100644 --- a/tests/test_operators.py +++ b/tests/test_operators.py @@ -376,7 +376,7 @@ async def test_correct_data_is_set_to_xml_when_creating(self): with patch("metadata_backend.api.operators.XMLToJSONParser"): acc = await (operator._format_data_to_create_and_add_to_db("study", xml_data)) m_insert.assert_called_once_with( - "backup-study", {"accessionId": self.accession_id, "content": xml_data} + "xml-study", {"accessionId": self.accession_id, "content": xml_data} ) self.assertEqual(acc, self.accession_id) @@ -396,7 +396,7 @@ async def test_correct_data_is_set_to_xml_when_replacing(self): with patch("metadata_backend.api.operators.XMLToJSONParser"): acc = await (operator._format_data_to_replace_and_add_to_db("study", self.accession_id, xml_data)) m_insert.assert_called_once_with( - "backup-study", + "xml-study", self.accession_id, {"accessionId": self.accession_id, "content": xml_data}, ) From 1abe3b1be5d23b3de35569104ae64e2eb66df2f1 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Wed, 3 Mar 2021 12:46:59 +0200 Subject: [PATCH 12/47] add checks to paramters for mongodb --- docker-compose.yml | 2 ++ metadata_backend/conf/conf.py | 26 ++++++++++++++++++-------- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index d73929011..77678bc3b 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -25,6 +25,8 @@ services: - "AUTH_REFERER=http://mockauth:8000" - "JWK_URL=http://mockauth:8000/keyset" - "LOG_LEVEL=DEBUG" + - "MONGO_DATABASE=default" + - "MONGO_AUTHDB=admin" database: image: "mongo" container_name: "metadata_submitter_database_dev" diff --git a/metadata_backend/conf/conf.py b/metadata_backend/conf/conf.py index 7583bb390..5b8d41b9b 100644 --- a/metadata_backend/conf/conf.py +++ b/metadata_backend/conf/conf.py @@ -43,6 +43,13 @@ # Set custom timeouts and other parameters here so they can be imported to # other modules if needed. +# If just MONGO_DATABASE is specified it will autenticate the user against it. +# If just MONGO_AUTHDB is specified it will autenticate the user against it. +# If both MONGO_DATABASE and MONGO_AUTHDB are specified, +# the client will attempt to authenticate the specified user to the MONGO_AUTHDB database. +# If both MONGO_DATABASE and MONGO_AUTHDB are unspecified, +# the client will attempt to authenticate the specified user to the admin database. + mongo_user = os.getenv("MONGO_USERNAME", "admin") mongo_password = os.getenv("MONGO_PASSWORD", "admin") mongo_host = os.getenv("MONGO_HOST", "localhost:27017") @@ -54,23 +61,26 @@ _cert = os.getenv("MONGO_SSL_CLIENT_CERT", None) tls = f"?tls=true&tlsCAFile={_ca}&ssl_keyfile={_key}&ssl_certfile={_cert}" url = f"{_base}{tls}" -elif bool(os.getenv("MONGO_SSL", None)) and os.getenv("MONGO_AUTHDB", "") != "": +elif bool(os.getenv("MONGO_SSL", None)) and bool(os.getenv("MONGO_AUTHDB")): _ca = os.getenv("MONGO_SSL_CA", None) _key = os.getenv("MONGO_SSL_CLIENT_KEY", None) _cert = os.getenv("MONGO_SSL_CLIENT_CERT", None) - tls = f"?tls=true&tlsCAFile={_ca}&ssl_keyfile={_key}&ssl_certfile={_cert}" - _authdb = os.getenv("MONGO_AUTHDB", "") - auth = f"&authSource={_authdb}" - url = f"{_base}{tls}{auth}" -elif os.getenv("MONGO_AUTHDB", "") != "": - _authdb = os.getenv("MONGO_AUTHDB", "") + if _ca and _key and _cert: + tls = f"?tls=true&tlsCAFile={_ca}&ssl_keyfile={_key}&ssl_certfile={_cert}" + _authdb = str(os.getenv("MONGO_AUTHDB")) + auth = f"&authSource={_authdb}" + url = f"{_base}{tls}{auth}" + else: + LOG.error("Missing CA, key or certificate.") +elif bool(os.getenv("MONGO_AUTHDB")): + _authdb = str(os.getenv("MONGO_AUTHDB")) auth = f"?authSource={_authdb}" url = f"{_base}{auth}" else: url = _base if os.getenv("MONGO_DATABASE", "") == "": - mongo_database = "test" + mongo_database = "default" LOG.debug(f"mongodb connection string is {url}") From a5a6b80443553491b8ca2e9c284c849fbc44463d Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Wed, 3 Mar 2021 13:26:18 +0200 Subject: [PATCH 13/47] update env var documentation --- docs/submitter.rst | 23 ++++++++++++++++------- 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/docs/submitter.rst b/docs/submitter.rst index 3058cdfdc..e5c95a86f 100644 --- a/docs/submitter.rst +++ b/docs/submitter.rst @@ -18,13 +18,16 @@ the table below. +--------------------------------+-------------------------------+-----------------------------------------------------------------------------------+ | ENV | Default | Description | +--------------------------------+-------------------------------+-----------------------------------------------------------------------------------+ -| ``MONGO_HOST`` | ``localhost:27017`` | Mongodb server hostname, with port specified if needed. | +| ``MONGO_HOST`` | ``localhost:27017`` | MongoDB server hostname, with port specified if needed. | +--------------------------------+-------------------------------+-----------------------------------------------------------------------------------+ -| ``MONGO_AUTHDB`` | ``-`` | Mongodb authentication database. | +| ``MONGO_AUTHDB`` | ``-`` | MongoDB authentication database. | +--------------------------------+-------------------------------+-----------------------------------------------------------------------------------+ -| ``MONGO_INITDB_ROOT_USERNAME`` | ``admin`` | Admin username for mongodb. | +| ``MONGO_DATABASE`` | ``default`` | MongoDB default database, will be used as authentication database if | +| | | ``MONGO_AUTHDB`` is not set. | +--------------------------------+-------------------------------+-----------------------------------------------------------------------------------+ -| ``MONGO_INITDB_ROOT_PASSWORD`` | ``admin`` | Admin password for mongodb. | +| ``MONGO_USERNAME`` | ``admin`` | Admin username for MongoDB. | ++--------------------------------+-------------------------------+-----------------------------------------------------------------------------------+ +| ``MONGO_PASSWORD`` | ``admin`` | Admin password for MongoDB. | +--------------------------------+-------------------------------+-----------------------------------------------------------------------------------+ | ``MONGO_SSL`` | ``-`` | Set to True to enable MONGO TLS connection url. | +--------------------------------+-------------------------------+-----------------------------------------------------------------------------------+ @@ -75,6 +78,12 @@ the table below. | | | for more information. | +--------------------------------+-------------------------------+-----------------------------------------------------------------------------------+ + +.. note:: If just ``MONGO_DATABASE`` is specified it will autenticate the user against it. + If just ``MONGO_AUTHDB`` is specified it will autenticate the user against it. + If both ``MONGO_DATABASE`` and ``MONGO_AUTHDB`` are specified, the client will attempt to authenticate the specified user to the MONGO_AUTHDB database. + If both ``MONGO_DATABASE`` and ``MONGO_AUTHDB`` are unspecified, the client will attempt to authenticate the specified user to the admin database. + Install and run --------------- @@ -87,11 +96,11 @@ For installing ``metadata-submitter`` backend do the following: .. hint:: Before running the application have MongoDB running. - MongoDB Server expects to find mongodb instance running, spesified with following environmental variables: + MongoDB Server expects to find MongoDB instance running, spesified with following environmental variables: - ``MONGO_INITDB_ROOT_USERNAME`` (username for admin user to mondogdb instance) - ``MONGO_INITDB_ROOT_PASSWORD`` (password for admin user to mondogdb instance) - - ``MONGO_HOST`` (host and port for mongodb instancem, e.g. `localhost:27017`) + - ``MONGO_HOST`` (host and port for MongoDB instance, e.g. `localhost:27017`) To run the backend from command line use: @@ -119,4 +128,4 @@ The REST API is structured as follows: any additional operations on other users are rejected. -.. include:: code.rst \ No newline at end of file +.. include:: code.rst From bca34810cd03bdae6bd19db78deaae5301277d8c Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Wed, 3 Mar 2021 14:22:48 +0200 Subject: [PATCH 14/47] more refined check for mongo_ssl bool value MONGO_SSL=False will not work as intended @teemukataja pointed out this --- metadata_backend/conf/conf.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/metadata_backend/conf/conf.py b/metadata_backend/conf/conf.py index 5b8d41b9b..3ff21c015 100644 --- a/metadata_backend/conf/conf.py +++ b/metadata_backend/conf/conf.py @@ -34,6 +34,7 @@ import json import os from pathlib import Path +from distutils.util import strtobool from motor.motor_asyncio import AsyncIOMotorClient @@ -55,13 +56,13 @@ mongo_host = os.getenv("MONGO_HOST", "localhost:27017") mongo_database = os.getenv("MONGO_DATABASE", "") _base = f"mongodb://{mongo_user}:{mongo_password}@{mongo_host}/{mongo_database}" -if bool(os.getenv("MONGO_SSL", None)): +if strtobool(os.getenv("MONGO_SSL", "False")): _ca = os.getenv("MONGO_SSL_CA", None) _key = os.getenv("MONGO_SSL_CLIENT_KEY", None) _cert = os.getenv("MONGO_SSL_CLIENT_CERT", None) tls = f"?tls=true&tlsCAFile={_ca}&ssl_keyfile={_key}&ssl_certfile={_cert}" url = f"{_base}{tls}" -elif bool(os.getenv("MONGO_SSL", None)) and bool(os.getenv("MONGO_AUTHDB")): +elif strtobool(os.getenv("MONGO_SSL", "False")) and bool(os.getenv("MONGO_AUTHDB")): _ca = os.getenv("MONGO_SSL_CA", None) _key = os.getenv("MONGO_SSL_CLIENT_KEY", None) _cert = os.getenv("MONGO_SSL_CLIENT_CERT", None) From e9501d4a4282bb956cb0d28e01dfbc462b4aa6d2 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Mon, 8 Mar 2021 11:44:17 +0200 Subject: [PATCH 15/47] require titles for objects --- metadata_backend/helpers/schemas/ena_analysis.json | 5 +++-- metadata_backend/helpers/schemas/ena_dac.json | 3 ++- metadata_backend/helpers/schemas/ena_experiment.json | 3 ++- metadata_backend/helpers/schemas/ena_policy.json | 5 +++-- metadata_backend/helpers/schemas/ena_run.json | 5 +++-- metadata_backend/helpers/schemas/ena_sample.json | 12 +++++++++--- 6 files changed, 22 insertions(+), 11 deletions(-) diff --git a/metadata_backend/helpers/schemas/ena_analysis.json b/metadata_backend/helpers/schemas/ena_analysis.json index 1e33cff49..0f51dbc93 100644 --- a/metadata_backend/helpers/schemas/ena_analysis.json +++ b/metadata_backend/helpers/schemas/ena_analysis.json @@ -265,7 +265,8 @@ "type": "object", "description": "A SRA analysis object captures sequence analysis results including sequence alignments, sequence variations and sequence annotations.", "required": [ - "analysisType" + "analysisType", + "title" ], "properties": { "title": { @@ -410,4 +411,4 @@ } } } -} \ No newline at end of file +} diff --git a/metadata_backend/helpers/schemas/ena_dac.json b/metadata_backend/helpers/schemas/ena_dac.json index e4512fbc7..977288bc0 100644 --- a/metadata_backend/helpers/schemas/ena_dac.json +++ b/metadata_backend/helpers/schemas/ena_dac.json @@ -111,7 +111,8 @@ }, "type": "object", "required": [ - "contacts" + "contacts", + "title" ], "properties": { "contacts": { diff --git a/metadata_backend/helpers/schemas/ena_experiment.json b/metadata_backend/helpers/schemas/ena_experiment.json index baf06cd12..10555466d 100644 --- a/metadata_backend/helpers/schemas/ena_experiment.json +++ b/metadata_backend/helpers/schemas/ena_experiment.json @@ -547,6 +547,7 @@ "type": "object", "description": "An Experiment specifies of what will be sequenced and how the sequencing will be performed. It does not contain results. An Experiment is composed of a design, a platform selection, and processing parameters.", "required": [ + "title", "design", "studyRef", "platform" @@ -672,4 +673,4 @@ } } } -} \ No newline at end of file +} diff --git a/metadata_backend/helpers/schemas/ena_policy.json b/metadata_backend/helpers/schemas/ena_policy.json index 35e0d0027..b43b66792 100644 --- a/metadata_backend/helpers/schemas/ena_policy.json +++ b/metadata_backend/helpers/schemas/ena_policy.json @@ -199,7 +199,8 @@ "description": "Describes an object that contains data access policy information.", "required": [ "dacRef", - "policy" + "policy", + "title" ], "properties": { "title": { @@ -273,4 +274,4 @@ } } } -} \ No newline at end of file +} diff --git a/metadata_backend/helpers/schemas/ena_run.json b/metadata_backend/helpers/schemas/ena_run.json index 51cf72b3b..41f25001b 100644 --- a/metadata_backend/helpers/schemas/ena_run.json +++ b/metadata_backend/helpers/schemas/ena_run.json @@ -293,7 +293,8 @@ "type": "object", "description": "A run contains a group of reads generated for a particular experiment.", "required": [ - "experimentRef" + "experimentRef", + "title" ], "properties": { "title": { @@ -567,4 +568,4 @@ } } } -} \ No newline at end of file +} diff --git a/metadata_backend/helpers/schemas/ena_sample.json b/metadata_backend/helpers/schemas/ena_sample.json index 91d28906f..09cc6d1fd 100644 --- a/metadata_backend/helpers/schemas/ena_sample.json +++ b/metadata_backend/helpers/schemas/ena_sample.json @@ -94,11 +94,17 @@ }, "type": "object", "required": [ - "sampleName" + "sampleName", + "title" ], "properties": { - "sampleName": { + "title": { "title": "Sample Title", + "description": "Short text that can be used to define submissions in searches or in displays.", + "type": "string" + }, + "sampleName": { + "title": "Sample Names", "description": "Short text that can be used to call out sample records in search results or in displays.", "type": "object", "properties": { @@ -147,4 +153,4 @@ } } } -} \ No newline at end of file +} From a327bb64c9a4fced7024c524869691c62fa7d2ba Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Mon, 8 Mar 2021 11:56:51 +0200 Subject: [PATCH 16/47] adjust fixtures --- tests/test_files/experiment/ERX000119.json | 5 +++-- tests/test_files/experiment/ERX000119.xml | 3 ++- tests/test_files/run/ERR000076.json | 3 ++- tests/test_files/run/ERR000076.xml | 1 + 4 files changed, 8 insertions(+), 4 deletions(-) diff --git a/tests/test_files/experiment/ERX000119.json b/tests/test_files/experiment/ERX000119.json index 2bbad38cb..ba4a1d80b 100644 --- a/tests/test_files/experiment/ERX000119.json +++ b/tests/test_files/experiment/ERX000119.json @@ -1,4 +1,5 @@ -{ +{ + "title": "Experiment", "alias": "NA18504.3", "centerName": "MPIMG", "identifiers": { @@ -65,4 +66,4 @@ "units": "MB" } ] -} \ No newline at end of file +} diff --git a/tests/test_files/experiment/ERX000119.xml b/tests/test_files/experiment/ERX000119.xml index 4736d446b..56a4aa457 100644 --- a/tests/test_files/experiment/ERX000119.xml +++ b/tests/test_files/experiment/ERX000119.xml @@ -1,10 +1,11 @@ - + ERX000119 NA18504.3 + Experiment SRP000031 diff --git a/tests/test_files/run/ERR000076.json b/tests/test_files/run/ERR000076.json index 605e971f9..aedfc4ede 100644 --- a/tests/test_files/run/ERR000076.json +++ b/tests/test_files/run/ERR000076.json @@ -1,5 +1,6 @@ { "alias": "BGI-FC304RWAAXX", + "title": "Run", "runDate": "2008-07-08T00:00:00.000+01:00", "runCenter": "BGI", "centerName": "BGI", @@ -45,4 +46,4 @@ "units": "bp" } ] -} \ No newline at end of file +} diff --git a/tests/test_files/run/ERR000076.xml b/tests/test_files/run/ERR000076.xml index e603a5947..d408d3d25 100644 --- a/tests/test_files/run/ERR000076.xml +++ b/tests/test_files/run/ERR000076.xml @@ -5,6 +5,7 @@ ERR000076 BGI-FC304RWAAXX + Run ERX000037 From c5c51e805a5e73d826105d63480f5cd9a8bf03a4 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Mon, 8 Mar 2021 22:50:21 +0200 Subject: [PATCH 17/47] new links structure for objects This requires oneOf processing from array in UI --- metadata_backend/helpers/parser.py | 9 +- .../helpers/schemas/ena_analysis.json | 143 +++++++------- metadata_backend/helpers/schemas/ena_dac.json | 143 +++++++------- .../helpers/schemas/ena_dataset.json | 145 +++++++------- .../helpers/schemas/ena_experiment.json | 143 +++++++------- .../helpers/schemas/ena_policy.json | 143 +++++++------- metadata_backend/helpers/schemas/ena_run.json | 143 +++++++------- .../helpers/schemas/ena_sample.json | 178 +++++++++++------- .../helpers/schemas/ena_study.json | 145 +++++++------- tests/test_files/study/SRP000539.json | 8 +- tests/test_parser.py | 2 +- 11 files changed, 685 insertions(+), 517 deletions(-) diff --git a/metadata_backend/helpers/parser.py b/metadata_backend/helpers/parser.py index e6d6091ed..ce0e723ad 100644 --- a/metadata_backend/helpers/parser.py +++ b/metadata_backend/helpers/parser.py @@ -1,7 +1,6 @@ """Tool to parse XML files to JSON.""" import re -from collections import defaultdict from typing import Any, Dict, List, Union from aiohttp import web @@ -98,14 +97,14 @@ def _flatten(self, data: Any) -> Union[Dict, List, str, None]: continue if key in links and len(value) == 1: - grp = defaultdict(list) + grp = list() if isinstance(value[key[:-1]], dict): - k = list(value[key[:-1]].keys())[0] - grp[f"{k}s"] = [it for it in value[key[:-1]].values()] + grp = [it for it in value[key[:-1]].values()] + children[key] = grp else: for item in value[key[:-1]]: for k, v in item.items(): - grp[f"{k}s"].append(v) + grp.append(v) children[key] = grp continue diff --git a/metadata_backend/helpers/schemas/ena_analysis.json b/metadata_backend/helpers/schemas/ena_analysis.json index 0f51dbc93..5cac12de5 100644 --- a/metadata_backend/helpers/schemas/ena_analysis.json +++ b/metadata_backend/helpers/schemas/ena_analysis.json @@ -1,73 +1,98 @@ { "title": "Analysis", "definitions": { + "Links": { + "$id": "#/definitions/Links", + "title": "Link Type", + "oneOf": [ + { + "$ref": "#/definitions/xrefLink" + }, + { + "$ref": "#/definitions/urlLink" + }, + { + "$ref": "#/definitions/entrezLink" + } + ] + }, "xrefLink": { "$id": "#/definitions/xrefLink", - "type": "array", "title": "XRef Link", - "items": { - "type": "object", - "required": [ - "db", - "id" - ], - "properties": { - "db": { - "type": "string", - "title": "Database" - }, - "id": { - "type": "string", - "title": "Associated accession Id" - } + "type": "object", + "required": [ + "db", + "id" + ], + "properties": { + "db": { + "type": "string", + "title": "Database" + }, + "id": { + "type": "string", + "title": "ID", + "description": "Accession in the referenced database." + }, + "label": { + "type": "string", + "title": "Label", + "description": "Text label to display for the link." } } }, "urlLink": { "$id": "#/definitions/urlLink", - "type": "array", "title": "URL Link", - "items": { - "type": "object", - "required": [ - "label", - "url" - ], - "properties": { - "label": { - "type": "string", - "title": "Label", - "description": "Text label to display for the link." - }, - "url": { - "type": "string", - "title": "URL", - "description": "The internet service link (http(s), ftp) etc.", - "pattern": "^(https?|ftp)://" - } + "type": "object", + "required": [ + "label", + "url" + ], + "properties": { + "label": { + "type": "string", + "title": "Label", + "description": "Text label to display for the link." + }, + "url": { + "type": "string", + "title": "URL", + "description": "The internet service link (http(s), ftp) etc.", + "pattern": "^(https?|ftp)://" } } }, "entrezLink": { "$id": "#/definitions/entrezLink", - "type": "array", "title": "Entrez Link", - "items": { - "type": "object", - "required": [ - "db" - ], - "properties": { - "db": { - "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", - "title": "Database", - "type": "string" - }, - "label": { - "description": "How to label the link.", - "title": "Label", - "type": "string" - } + "type": "object", + "required": [ + "db", + "id", + "query" + ], + "properties": { + "db": { + "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", + "title": "Database", + "type": "string" + }, + "id": { + "type": "number", + "minimum": 0, + "title": "ID", + "description": "Numeric record id meaningful to the NCBI Entrez system." + }, + "query": { + "type": "string", + "title": "Query", + "description": "Accession string meaningful to the NCBI Entrez system." + }, + "label": { + "description": "Text label to display for the link.", + "title": "Label", + "type": "string" } } }, @@ -389,18 +414,10 @@ } }, "analysisLinks": { - "type": "object", + "type": "array", "title": "Analysis Links", - "properties": { - "xrefLinks": { - "$ref": "#/definitions/xrefLink" - }, - "entrezLinks": { - "$ref": "#/definitions/entrezLink" - }, - "urlLinks": { - "$ref": "#/definitions/urlLink" - } + "items": { + "$ref": "#/definitions/Links" } }, "analysisAttributes": { diff --git a/metadata_backend/helpers/schemas/ena_dac.json b/metadata_backend/helpers/schemas/ena_dac.json index 977288bc0..d08c81b34 100644 --- a/metadata_backend/helpers/schemas/ena_dac.json +++ b/metadata_backend/helpers/schemas/ena_dac.json @@ -1,73 +1,98 @@ { "title": "DAC - Data Access Committee", "definitions": { + "Links": { + "$id": "#/definitions/Links", + "title": "Link Type", + "oneOf": [ + { + "$ref": "#/definitions/xrefLink" + }, + { + "$ref": "#/definitions/urlLink" + }, + { + "$ref": "#/definitions/entrezLink" + } + ] + }, "xrefLink": { "$id": "#/definitions/xrefLink", - "type": "array", "title": "XRef Link", - "items": { - "type": "object", - "required": [ - "db", - "id" - ], - "properties": { - "db": { - "type": "string", - "title": "Database" - }, - "id": { - "type": "string", - "title": "Associated accession Id" - } + "type": "object", + "required": [ + "db", + "id" + ], + "properties": { + "db": { + "type": "string", + "title": "Database" + }, + "id": { + "type": "string", + "title": "ID", + "description": "Accession in the referenced database." + }, + "label": { + "type": "string", + "title": "Label", + "description": "Text label to display for the link." } } }, "urlLink": { "$id": "#/definitions/urlLink", - "type": "array", "title": "URL Link", - "items": { - "type": "object", - "required": [ - "label", - "url" - ], - "properties": { - "label": { - "type": "string", - "title": "Label", - "description": "Text label to display for the link." - }, - "url": { - "type": "string", - "title": "URL", - "description": "The internet service link (http(s), ftp) etc.", - "pattern": "^(https?|ftp)://" - } + "type": "object", + "required": [ + "label", + "url" + ], + "properties": { + "label": { + "type": "string", + "title": "Label", + "description": "Text label to display for the link." + }, + "url": { + "type": "string", + "title": "URL", + "description": "The internet service link (http(s), ftp) etc.", + "pattern": "^(https?|ftp)://" } } }, "entrezLink": { "$id": "#/definitions/entrezLink", - "type": "array", "title": "Entrez Link", - "items": { - "type": "object", - "required": [ - "db" - ], - "properties": { - "db": { - "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", - "title": "Database", - "type": "string" - }, - "label": { - "description": "How to label the link.", - "title": "Label", - "type": "string" - } + "type": "object", + "required": [ + "db", + "id", + "query" + ], + "properties": { + "db": { + "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", + "title": "Database", + "type": "string" + }, + "id": { + "type": "number", + "minimum": 0, + "title": "ID", + "description": "Numeric record id meaningful to the NCBI Entrez system." + }, + "query": { + "type": "string", + "title": "Query", + "description": "Accession string meaningful to the NCBI Entrez system." + }, + "label": { + "description": "Text label to display for the link.", + "title": "Label", + "type": "string" } } }, @@ -145,18 +170,10 @@ "type": "string" }, "dacLinks": { - "type": "object", + "type": "array", "title": "DAC Links", - "properties": { - "xrefLinks": { - "$ref": "#/definitions/xrefLink" - }, - "entrezLinks": { - "$ref": "#/definitions/entrezLink" - }, - "urlLinks": { - "$ref": "#/definitions/urlLink" - } + "items": { + "$ref": "#/definitions/Links" } } } diff --git a/metadata_backend/helpers/schemas/ena_dataset.json b/metadata_backend/helpers/schemas/ena_dataset.json index 95a5185ab..5dbc729f0 100644 --- a/metadata_backend/helpers/schemas/ena_dataset.json +++ b/metadata_backend/helpers/schemas/ena_dataset.json @@ -1,73 +1,98 @@ { "title": "Dataset", "definitions": { + "Links": { + "$id": "#/definitions/Links", + "title": "Link Type", + "oneOf": [ + { + "$ref": "#/definitions/xrefLink" + }, + { + "$ref": "#/definitions/urlLink" + }, + { + "$ref": "#/definitions/entrezLink" + } + ] + }, "xrefLink": { "$id": "#/definitions/xrefLink", - "type": "array", "title": "XRef Link", - "items": { - "type": "object", - "required": [ - "db", - "id" - ], - "properties": { - "db": { - "type": "string", - "title": "Database" - }, - "id": { - "type": "string", - "title": "Associated accession Id" - } + "type": "object", + "required": [ + "db", + "id" + ], + "properties": { + "db": { + "type": "string", + "title": "Database" + }, + "id": { + "type": "string", + "title": "ID", + "description": "Accession in the referenced database." + }, + "label": { + "type": "string", + "title": "Label", + "description": "Text label to display for the link." } } }, "urlLink": { "$id": "#/definitions/urlLink", - "type": "array", "title": "URL Link", - "items": { - "type": "object", - "required": [ - "label", - "url" - ], - "properties": { - "label": { - "type": "string", - "title": "Label", - "description": "Text label to display for the link." - }, - "url": { - "type": "string", - "title": "URL", - "description": "The internet service link (http(s), ftp) etc.", - "pattern": "^(https?|ftp)://" - } + "type": "object", + "required": [ + "label", + "url" + ], + "properties": { + "label": { + "type": "string", + "title": "Label", + "description": "Text label to display for the link." + }, + "url": { + "type": "string", + "title": "URL", + "description": "The internet service link (http(s), ftp) etc.", + "pattern": "^(https?|ftp)://" } } }, "entrezLink": { "$id": "#/definitions/entrezLink", - "type": "array", "title": "Entrez Link", - "items": { - "type": "object", - "required": [ - "db" - ], - "properties": { - "db": { - "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", - "title": "Database", - "type": "string" - }, - "label": { - "description": "How to label the link.", - "title": "Label", - "type": "string" - } + "type": "object", + "required": [ + "db", + "id", + "query" + ], + "properties": { + "db": { + "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", + "title": "Database", + "type": "string" + }, + "id": { + "type": "number", + "minimum": 0, + "title": "ID", + "description": "Numeric record id meaningful to the NCBI Entrez system." + }, + "query": { + "type": "string", + "title": "Query", + "description": "Accession string meaningful to the NCBI Entrez system." + }, + "label": { + "description": "Text label to display for the link.", + "title": "Label", + "type": "string" } } }, @@ -203,18 +228,10 @@ "$ref": "#/definitions/reference" }, "datasetLinks": { - "type": "object", + "type": "array", "title": "Dataset Links", - "properties": { - "xrefLinks": { - "$ref": "#/definitions/xrefLink" - }, - "entrezLinks": { - "$ref": "#/definitions/entrezLink" - }, - "urlLinks": { - "$ref": "#/definitions/urlLink" - } + "items": { + "$ref": "#/definitions/Links" } }, "datasetAttributes": { @@ -225,4 +242,4 @@ } } } -} \ No newline at end of file +} diff --git a/metadata_backend/helpers/schemas/ena_experiment.json b/metadata_backend/helpers/schemas/ena_experiment.json index 10555466d..857e4fb02 100644 --- a/metadata_backend/helpers/schemas/ena_experiment.json +++ b/metadata_backend/helpers/schemas/ena_experiment.json @@ -1,73 +1,98 @@ { "title": "Experiment", "definitions": { + "Links": { + "$id": "#/definitions/Links", + "title": "Link Type", + "oneOf": [ + { + "$ref": "#/definitions/xrefLink" + }, + { + "$ref": "#/definitions/urlLink" + }, + { + "$ref": "#/definitions/entrezLink" + } + ] + }, "xrefLink": { "$id": "#/definitions/xrefLink", - "type": "array", "title": "XRef Link", - "items": { - "type": "object", - "required": [ - "db", - "id" - ], - "properties": { - "db": { - "type": "string", - "title": "Database" - }, - "id": { - "type": "string", - "title": "Associated accession Id" - } + "type": "object", + "required": [ + "db", + "id" + ], + "properties": { + "db": { + "type": "string", + "title": "Database" + }, + "id": { + "type": "string", + "title": "ID", + "description": "Accession in the referenced database." + }, + "label": { + "type": "string", + "title": "Label", + "description": "Text label to display for the link." } } }, "urlLink": { "$id": "#/definitions/urlLink", - "type": "array", "title": "URL Link", - "items": { - "type": "object", - "required": [ - "label", - "url" - ], - "properties": { - "label": { - "type": "string", - "title": "Label", - "description": "Text label to display for the link." - }, - "url": { - "type": "string", - "title": "URL", - "description": "The internet service link (http(s), ftp) etc.", - "pattern": "^(https?|ftp)://" - } + "type": "object", + "required": [ + "label", + "url" + ], + "properties": { + "label": { + "type": "string", + "title": "Label", + "description": "Text label to display for the link." + }, + "url": { + "type": "string", + "title": "URL", + "description": "The internet service link (http(s), ftp) etc.", + "pattern": "^(https?|ftp)://" } } }, "entrezLink": { "$id": "#/definitions/entrezLink", - "type": "array", "title": "Entrez Link", - "items": { - "type": "object", - "required": [ - "db" - ], - "properties": { - "db": { - "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", - "title": "Database", - "type": "string" - }, - "label": { - "description": "How to label the link.", - "title": "Label", - "type": "string" - } + "type": "object", + "required": [ + "db", + "id", + "query" + ], + "properties": { + "db": { + "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", + "title": "Database", + "type": "string" + }, + "id": { + "type": "number", + "minimum": 0, + "title": "ID", + "description": "Numeric record id meaningful to the NCBI Entrez system." + }, + "query": { + "type": "string", + "title": "Query", + "description": "Accession string meaningful to the NCBI Entrez system." + }, + "label": { + "description": "Text label to display for the link.", + "title": "Label", + "type": "string" } } }, @@ -650,19 +675,11 @@ ] }, "experimentLinks": { - "type": "object", + "type": "array", "title": "Experiment Links", "description": " Links to resources related to this experiment or experiment set (publication, datasets, online databases).", - "properties": { - "xrefLinks": { - "$ref": "#/definitions/xrefLink" - }, - "entrezLinks": { - "$ref": "#/definitions/entrezLink" - }, - "urlLinks": { - "$ref": "#/definitions/urlLink" - } + "items": { + "$ref": "#/definitions/Links" } }, "experimentAttributes": { diff --git a/metadata_backend/helpers/schemas/ena_policy.json b/metadata_backend/helpers/schemas/ena_policy.json index b43b66792..bb5e0df50 100644 --- a/metadata_backend/helpers/schemas/ena_policy.json +++ b/metadata_backend/helpers/schemas/ena_policy.json @@ -1,73 +1,98 @@ { "title": "Policy", "definitions": { + "Links": { + "$id": "#/definitions/Links", + "title": "Link Type", + "oneOf": [ + { + "$ref": "#/definitions/xrefLink" + }, + { + "$ref": "#/definitions/urlLink" + }, + { + "$ref": "#/definitions/entrezLink" + } + ] + }, "xrefLink": { "$id": "#/definitions/xrefLink", - "type": "array", "title": "XRef Link", - "items": { - "type": "object", - "required": [ - "db", - "id" - ], - "properties": { - "db": { - "type": "string", - "title": "Database" - }, - "id": { - "type": "string", - "title": "Associated accession Id" - } + "type": "object", + "required": [ + "db", + "id" + ], + "properties": { + "db": { + "type": "string", + "title": "Database" + }, + "id": { + "type": "string", + "title": "ID", + "description": "Accession in the referenced database." + }, + "label": { + "type": "string", + "title": "Label", + "description": "Text label to display for the link." } } }, "urlLink": { "$id": "#/definitions/urlLink", - "type": "array", "title": "URL Link", - "items": { - "type": "object", - "required": [ - "label", - "url" - ], - "properties": { - "label": { - "type": "string", - "title": "Label", - "description": "Text label to display for the link." - }, - "url": { - "type": "string", - "title": "URL", - "description": "The internet service link (http(s), ftp) etc.", - "pattern": "^(https?|ftp)://" - } + "type": "object", + "required": [ + "label", + "url" + ], + "properties": { + "label": { + "type": "string", + "title": "Label", + "description": "Text label to display for the link." + }, + "url": { + "type": "string", + "title": "URL", + "description": "The internet service link (http(s), ftp) etc.", + "pattern": "^(https?|ftp)://" } } }, "entrezLink": { "$id": "#/definitions/entrezLink", - "type": "array", "title": "Entrez Link", - "items": { - "type": "object", - "required": [ - "db" - ], - "properties": { - "db": { - "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", - "title": "Database", - "type": "string" - }, - "label": { - "description": "How to label the link.", - "title": "Label", - "type": "string" - } + "type": "object", + "required": [ + "db", + "id", + "query" + ], + "properties": { + "db": { + "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", + "title": "Database", + "type": "string" + }, + "id": { + "type": "number", + "minimum": 0, + "title": "ID", + "description": "Numeric record id meaningful to the NCBI Entrez system." + }, + "query": { + "type": "string", + "title": "Query", + "description": "Accession string meaningful to the NCBI Entrez system." + }, + "label": { + "description": "Text label to display for the link.", + "title": "Label", + "type": "string" } } }, @@ -247,18 +272,10 @@ ] }, "policyLinks": { - "type": "object", + "type": "array", "title": "Policy Links", - "properties": { - "xrefLinks": { - "$ref": "#/definitions/xrefLink" - }, - "entrezLinks": { - "$ref": "#/definitions/entrezLink" - }, - "urlLinks": { - "$ref": "#/definitions/urlLink" - } + "items": { + "$ref": "#/definitions/Links" } }, "dataUses": { diff --git a/metadata_backend/helpers/schemas/ena_run.json b/metadata_backend/helpers/schemas/ena_run.json index 41f25001b..0be2a5240 100644 --- a/metadata_backend/helpers/schemas/ena_run.json +++ b/metadata_backend/helpers/schemas/ena_run.json @@ -1,73 +1,98 @@ { "title": "Run", "definitions": { + "Links": { + "$id": "#/definitions/Links", + "title": "Link Type", + "oneOf": [ + { + "$ref": "#/definitions/xrefLink" + }, + { + "$ref": "#/definitions/urlLink" + }, + { + "$ref": "#/definitions/entrezLink" + } + ] + }, "xrefLink": { "$id": "#/definitions/xrefLink", - "type": "array", "title": "XRef Link", - "items": { - "type": "object", - "required": [ - "db", - "id" - ], - "properties": { - "db": { - "type": "string", - "title": "Database" - }, - "id": { - "type": "string", - "title": "Associated accession Id" - } + "type": "object", + "required": [ + "db", + "id" + ], + "properties": { + "db": { + "type": "string", + "title": "Database" + }, + "id": { + "type": "string", + "title": "ID", + "description": "Accession in the referenced database." + }, + "label": { + "type": "string", + "title": "Label", + "description": "Text label to display for the link." } } }, "urlLink": { "$id": "#/definitions/urlLink", - "type": "array", "title": "URL Link", - "items": { - "type": "object", - "required": [ - "label", - "url" - ], - "properties": { - "label": { - "type": "string", - "title": "Label", - "description": "Text label to display for the link." - }, - "url": { - "type": "string", - "title": "URL", - "description": "The internet service link (http(s), ftp) etc.", - "pattern": "^(https?|ftp)://" - } + "type": "object", + "required": [ + "label", + "url" + ], + "properties": { + "label": { + "type": "string", + "title": "Label", + "description": "Text label to display for the link." + }, + "url": { + "type": "string", + "title": "URL", + "description": "The internet service link (http(s), ftp) etc.", + "pattern": "^(https?|ftp)://" } } }, "entrezLink": { "$id": "#/definitions/entrezLink", - "type": "array", "title": "Entrez Link", - "items": { - "type": "object", - "required": [ - "db" - ], - "properties": { - "db": { - "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", - "title": "Database", - "type": "string" - }, - "label": { - "description": "How to label the link.", - "title": "Label", - "type": "string" - } + "type": "object", + "required": [ + "db", + "id", + "query" + ], + "properties": { + "db": { + "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", + "title": "Database", + "type": "string" + }, + "id": { + "type": "number", + "minimum": 0, + "title": "ID", + "description": "Numeric record id meaningful to the NCBI Entrez system." + }, + "query": { + "type": "string", + "title": "Query", + "description": "Accession string meaningful to the NCBI Entrez system." + }, + "label": { + "description": "Text label to display for the link.", + "title": "Label", + "type": "string" } } }, @@ -546,18 +571,10 @@ } }, "runLinks": { - "type": "object", + "type": "array", "title": "Run Links", - "properties": { - "xrefLinks": { - "$ref": "#/definitions/xrefLink" - }, - "entrezLinks": { - "$ref": "#/definitions/entrezLink" - }, - "urlLinks": { - "$ref": "#/definitions/urlLink" - } + "items": { + "$ref": "#/definitions/Links" } }, "runAttributes": { diff --git a/metadata_backend/helpers/schemas/ena_sample.json b/metadata_backend/helpers/schemas/ena_sample.json index 09cc6d1fd..a9fe9ee08 100644 --- a/metadata_backend/helpers/schemas/ena_sample.json +++ b/metadata_backend/helpers/schemas/ena_sample.json @@ -1,73 +1,98 @@ { "title": "Sample", "definitions": { + "Links": { + "$id": "#/definitions/Links", + "title": "Link Type", + "oneOf": [ + { + "$ref": "#/definitions/xrefLink" + }, + { + "$ref": "#/definitions/urlLink" + }, + { + "$ref": "#/definitions/entrezLink" + } + ] + }, "xrefLink": { "$id": "#/definitions/xrefLink", - "type": "array", "title": "XRef Link", - "items": { - "type": "object", - "required": [ - "db", - "id" - ], - "properties": { - "db": { - "type": "string", - "title": "Database" - }, - "id": { - "type": "string", - "title": "Associated accession Id" - } + "type": "object", + "required": [ + "db", + "id" + ], + "properties": { + "db": { + "type": "string", + "title": "Database" + }, + "id": { + "type": "string", + "title": "ID", + "description": "Accession in the referenced database." + }, + "label": { + "type": "string", + "title": "Label", + "description": "Text label to display for the link." } } }, "urlLink": { "$id": "#/definitions/urlLink", - "type": "array", "title": "URL Link", - "items": { - "type": "object", - "required": [ - "label", - "url" - ], - "properties": { - "label": { - "type": "string", - "title": "Label", - "description": "Text label to display for the link." - }, - "url": { - "type": "string", - "title": "URL", - "description": "The internet service link (http(s), ftp) etc.", - "pattern": "^(https?|ftp)://" - } + "type": "object", + "required": [ + "label", + "url" + ], + "properties": { + "label": { + "type": "string", + "title": "Label", + "description": "Text label to display for the link." + }, + "url": { + "type": "string", + "title": "URL", + "description": "The internet service link (http(s), ftp) etc.", + "pattern": "^(https?|ftp)://" } } }, "entrezLink": { "$id": "#/definitions/entrezLink", - "type": "array", "title": "Entrez Link", - "items": { - "type": "object", - "required": [ - "db" - ], - "properties": { - "db": { - "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", - "title": "Database", - "type": "string" - }, - "label": { - "description": "How to label the link.", - "title": "Label", - "type": "string" - } + "type": "object", + "required": [ + "db", + "id", + "query" + ], + "properties": { + "db": { + "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", + "title": "Database", + "type": "string" + }, + "id": { + "type": "number", + "minimum": 0, + "title": "ID", + "description": "Numeric record id meaningful to the NCBI Entrez system." + }, + "query": { + "type": "string", + "title": "Query", + "description": "Accession string meaningful to the NCBI Entrez system." + }, + "label": { + "description": "Text label to display for the link.", + "title": "Label", + "type": "string" } } }, @@ -107,6 +132,9 @@ "title": "Sample Names", "description": "Short text that can be used to call out sample records in search results or in displays.", "type": "object", + "required": [ + "taxonId" + ], "properties": { "taxonId": { "type": "integer", @@ -130,19 +158,43 @@ "description": "More extensive free-form description of the sample.", "type": "string" }, - "sampleLinks": { - "type": "object", - "title": "Sample Links", - "properties": { - "xrefLinks": { - "$ref": "#/definitions/xrefLink" - }, - "entrezLinks": { - "$ref": "#/definitions/entrezLink" + "sampleData": { + "title": "Sample Data Type", + "oneOf": [ + { + "type": "object", + "title": "Human Sample", + "properties": { + "gender": { + "type": "string", + "title": "Gender", + "enum": ["male", "female", "unknown"] + } + }, + "required": [ + "gender" + ] }, - "urlLinks": { - "$ref": "#/definitions/urlLink" + { + "type": "object", + "title": "Non Human Sample", + "properties": { + "dataDescription": { + "type": "string", + "title": "Sample Type Description" + } + }, + "required": [ + "dataDescription" + ] } + ] + }, + "sampleLinks": { + "type": "array", + "title": "Sample Links", + "items": { + "$ref": "#/definitions/Links" } }, "sampleAttributes": { diff --git a/metadata_backend/helpers/schemas/ena_study.json b/metadata_backend/helpers/schemas/ena_study.json index 5e91003ae..88d17d38e 100644 --- a/metadata_backend/helpers/schemas/ena_study.json +++ b/metadata_backend/helpers/schemas/ena_study.json @@ -1,73 +1,98 @@ { "title": "Study", "definitions": { + "Links": { + "$id": "#/definitions/Links", + "title": "Link Type", + "oneOf": [ + { + "$ref": "#/definitions/xrefLink" + }, + { + "$ref": "#/definitions/urlLink" + }, + { + "$ref": "#/definitions/entrezLink" + } + ] + }, "xrefLink": { "$id": "#/definitions/xrefLink", - "type": "array", "title": "XRef Link", - "items": { - "type": "object", - "required": [ - "db", - "id" - ], - "properties": { - "db": { - "type": "string", - "title": "Database" - }, - "id": { - "type": "string", - "title": "Associated accession Id" - } + "type": "object", + "required": [ + "db", + "id" + ], + "properties": { + "db": { + "type": "string", + "title": "Database" + }, + "id": { + "type": "string", + "title": "ID", + "description": "Accession in the referenced database." + }, + "label": { + "type": "string", + "title": "Label", + "description": "Text label to display for the link." } } }, "urlLink": { "$id": "#/definitions/urlLink", - "type": "array", "title": "URL Link", - "items": { - "type": "object", - "required": [ - "label", - "url" - ], - "properties": { - "label": { - "type": "string", - "title": "Label", - "description": "Text label to display for the link." - }, - "url": { - "type": "string", - "title": "URL", - "description": "The internet service link (http(s), ftp) etc.", - "pattern": "^(https?|ftp)://" - } + "type": "object", + "required": [ + "label", + "url" + ], + "properties": { + "label": { + "type": "string", + "title": "Label", + "description": "Text label to display for the link." + }, + "url": { + "type": "string", + "title": "URL", + "description": "The internet service link (http(s), ftp) etc.", + "pattern": "^(https?|ftp)://" } } }, "entrezLink": { "$id": "#/definitions/entrezLink", - "type": "array", "title": "Entrez Link", - "items": { - "type": "object", - "required": [ - "db" - ], - "properties": { - "db": { - "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", - "title": "Database", - "type": "string" - }, - "label": { - "description": "How to label the link.", - "title": "Label", - "type": "string" - } + "type": "object", + "required": [ + "db", + "id", + "query" + ], + "properties": { + "db": { + "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", + "title": "Database", + "type": "string" + }, + "id": { + "type": "number", + "minimum": 0, + "title": "ID", + "description": "Numeric record id meaningful to the NCBI Entrez system." + }, + "query": { + "type": "string", + "title": "Query", + "description": "Accession string meaningful to the NCBI Entrez system." + }, + "label": { + "description": "Text label to display for the link.", + "title": "Label", + "type": "string" } } }, @@ -122,7 +147,7 @@ "properties": { "descriptor": { "type": "object", - "title": "Study Description", + "title": "Study", "required": [ "studyTitle", "studyType" @@ -156,18 +181,10 @@ "type": "string" }, "studyLinks": { - "type": "object", + "type": "array", "title": "Study Links", - "properties": { - "xrefLinks": { - "$ref": "#/definitions/xrefLink" - }, - "entrezLinks": { - "$ref": "#/definitions/entrezLink" - }, - "urlLinks": { - "$ref": "#/definitions/urlLink" - } + "items": { + "$ref": "#/definitions/Links" } }, "studyAttributes": { diff --git a/tests/test_files/study/SRP000539.json b/tests/test_files/study/SRP000539.json index 92fef58ff..407d2fcce 100644 --- a/tests/test_files/study/SRP000539.json +++ b/tests/test_files/study/SRP000539.json @@ -21,18 +21,16 @@ "studyAbstract": "Part of a set of highly integrated epigenome maps for Arabidopsis thaliana. Keywords: Illumina high-throughput bisulfite sequencing Overall design: Whole genome shotgun bisulfite sequencing of wildtype Arabidopsis plants (Columbia-0), and met1, drm1 drm2 cmt3, and ros1 dml2 dml3 null mutants using the Illumina Genetic Analyzer.", "centerProjectName": "GSE10966" }, - "studyLinks": { - "xrefLinks": [ + "studyLinks": [ { "db": "pubmed", "id": "18423832" } - ] - }, + ], "studyAttributes": [ { "tag": "parent_bioproject", "value": "PRJNA107265" } ] -} \ No newline at end of file +} diff --git a/tests/test_parser.py b/tests/test_parser.py index d113157cc..f47cefaff 100644 --- a/tests/test_parser.py +++ b/tests/test_parser.py @@ -30,7 +30,7 @@ def test_study_is_parsed(self): study_xml = self.load_xml_from_file("study", "SRP000539.xml") study_json = self.parser.parse("study", study_xml) self.assertIn("Highly integrated epigenome maps in Arabidopsis", study_json["descriptor"]["studyTitle"]) - self.assertIn("18423832", study_json["studyLinks"]["xrefLinks"][0]["id"]) + self.assertIn("18423832", study_json["studyLinks"][0]["id"]) def test_sample_is_parsed(self): """Test that sample is parsed correctly and accessionId is set. From e510966daa77ef23facca946bedb6f3239640e74 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Mon, 8 Mar 2021 22:50:36 +0200 Subject: [PATCH 18/47] small adjustments and corrections for study schema --- metadata_backend/helpers/schemas/ena_study.json | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/metadata_backend/helpers/schemas/ena_study.json b/metadata_backend/helpers/schemas/ena_study.json index 88d17d38e..047f93f53 100644 --- a/metadata_backend/helpers/schemas/ena_study.json +++ b/metadata_backend/helpers/schemas/ena_study.json @@ -147,7 +147,7 @@ "properties": { "descriptor": { "type": "object", - "title": "Study", + "title": "Study Description", "required": [ "studyTitle", "studyType" @@ -180,6 +180,12 @@ "description": "More extensive free-form description of the study.", "type": "string" }, + "pubMedID": { + "type": "string", + "title": "pubMedID identifier", + "description": "PubMed ID (8 digits) or the PMC ID (PMC + 7 digits)", + "pattern": "^[0-9]{8}|PMC[0-9]{7}" + }, "studyLinks": { "type": "array", "title": "Study Links", @@ -194,12 +200,6 @@ "$ref": "#/definitions/studyAttribute" } }, - "pubMedID": { - "type": "string", - "title": "pubMedID identifier", - "description": " PubMed ID (8 digits) or the PMC ID (PMC + 7 digits)", - "pattern": "^[0-9]{8}|PMC[0-9]{7}" - }, "center": { "title": "Description for Center", "description": "More for backwards compatibility, we might not need it.", @@ -207,7 +207,7 @@ "properties": { "centerProjectName": { "title": "Center Project Name", - "description": " Submitter defined project name. This field is intended for backward tracking of the study record to the submitter's LIMS.", + "description": " Submitter defined project name. This field is intended for backward tracking of the study record to the submitter's LIMS.", "type": "string" } } From cdaa2ad610e7dd4942c6cd63849b505407a40429 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Tue, 9 Mar 2021 12:06:37 +0200 Subject: [PATCH 19/47] fix Links validation prov for required oneof field e.g. entrezLink required field db was the same as xrefLink required field db and that confused the validator refine parser --- metadata_backend/helpers/parser.py | 14 ++- .../helpers/schemas/ena_analysis.json | 85 +++++++++++++----- metadata_backend/helpers/schemas/ena_dac.json | 85 +++++++++++++----- .../helpers/schemas/ena_dataset.json | 85 +++++++++++++----- .../helpers/schemas/ena_experiment.json | 85 +++++++++++++----- .../helpers/schemas/ena_policy.json | 85 +++++++++++++----- metadata_backend/helpers/schemas/ena_run.json | 85 +++++++++++++----- .../helpers/schemas/ena_sample.json | 85 +++++++++++++----- .../helpers/schemas/ena_study.json | 89 ++++++++++++------- tests/test_parser.py | 2 +- 10 files changed, 505 insertions(+), 195 deletions(-) diff --git a/metadata_backend/helpers/parser.py b/metadata_backend/helpers/parser.py index ce0e723ad..260641955 100644 --- a/metadata_backend/helpers/parser.py +++ b/metadata_backend/helpers/parser.py @@ -100,13 +100,21 @@ def _flatten(self, data: Any) -> Union[Dict, List, str, None]: grp = list() if isinstance(value[key[:-1]], dict): grp = [it for it in value[key[:-1]].values()] - children[key] = grp + ks = list(value[key[:-1]])[0][:-4] + if ks == "url": + children[key] = grp + else: + children[key] = [{ks + k.capitalize(): v for k, v in d.items()} for d in grp] else: for item in value[key[:-1]]: for k, v in item.items(): - grp.append(v) + ks = k[:-4] + if ks == "url": + grp.append({str(key): val for key, val in v.items()}) + else: + grp.append({ks + str(key).capitalize(): val for key, val in v.items()}) - children[key] = grp + children[key] = grp continue value = self.list() if value is None else value diff --git a/metadata_backend/helpers/schemas/ena_analysis.json b/metadata_backend/helpers/schemas/ena_analysis.json index 5cac12de5..96e93c845 100644 --- a/metadata_backend/helpers/schemas/ena_analysis.json +++ b/metadata_backend/helpers/schemas/ena_analysis.json @@ -21,20 +21,20 @@ "title": "XRef Link", "type": "object", "required": [ - "db", - "id" + "xrefDb", + "xrefId" ], "properties": { - "db": { + "xrefDb": { "type": "string", "title": "Database" }, - "id": { + "xrefId": { "type": "string", - "title": "ID", + "title": "Database ID", "description": "Accession in the referenced database." }, - "label": { + "xrefLabel": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -50,7 +50,7 @@ "url" ], "properties": { - "label": { + "urlLabel": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -68,28 +68,67 @@ "title": "Entrez Link", "type": "object", "required": [ - "db", - "id", - "query" + "entrezDb", + "entrezId" ], "properties": { - "db": { - "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", + "entrezDb": { + "description": "NCBI controlled vocabulary of permitted cross references", "title": "Database", - "type": "string" - }, - "id": { - "type": "number", - "minimum": 0, - "title": "ID", - "description": "Numeric record id meaningful to the NCBI Entrez system." + "type": "string", + "enum": [ + "pubmed", + "protein", + "nuccore", + "ipg", + "nucleotide", + "structure", + "genome", + "annotinfo", + "assembly", + "bioproject", + "biosample", + "blastdbinfo", + "books", + "cdd", + "clinvar", + "gap", + "gapplus", + "grasp", + "dbvar", + "gene", + "gds", + "geoprofiles", + "homologene", + "medgen", + "mesh", + "ncbisearch", + "nlmcatalog", + "omim", + "orgtrack", + "pmc", + "popset", + "proteinclusters", + "pcassay", + "protfam", + "biosystems", + "pccompound", + "pcsubstance", + "seqannot", + "snp", + "sra", + "taxonomy", + "biocollections", + "gtr" + ] }, - "query": { + "entrezId": { "type": "string", - "title": "Query", - "description": "Accession string meaningful to the NCBI Entrez system." + "title": "Database ID", + "description": "Numeric record id meaningful to the NCBI Entrez system.", + "pattern": "^[a-zA-Z0-9]+" }, - "label": { + "entrezLabel": { "description": "Text label to display for the link.", "title": "Label", "type": "string" diff --git a/metadata_backend/helpers/schemas/ena_dac.json b/metadata_backend/helpers/schemas/ena_dac.json index d08c81b34..4f87de220 100644 --- a/metadata_backend/helpers/schemas/ena_dac.json +++ b/metadata_backend/helpers/schemas/ena_dac.json @@ -21,20 +21,20 @@ "title": "XRef Link", "type": "object", "required": [ - "db", - "id" + "xrefDb", + "xrefId" ], "properties": { - "db": { + "xrefDb": { "type": "string", "title": "Database" }, - "id": { + "xrefId": { "type": "string", - "title": "ID", + "title": "Database ID", "description": "Accession in the referenced database." }, - "label": { + "xrefLabel": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -50,7 +50,7 @@ "url" ], "properties": { - "label": { + "urlLabel": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -68,28 +68,67 @@ "title": "Entrez Link", "type": "object", "required": [ - "db", - "id", - "query" + "entrezDb", + "entrezId" ], "properties": { - "db": { - "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", + "entrezDb": { + "description": "NCBI controlled vocabulary of permitted cross references", "title": "Database", - "type": "string" - }, - "id": { - "type": "number", - "minimum": 0, - "title": "ID", - "description": "Numeric record id meaningful to the NCBI Entrez system." + "type": "string", + "enum": [ + "pubmed", + "protein", + "nuccore", + "ipg", + "nucleotide", + "structure", + "genome", + "annotinfo", + "assembly", + "bioproject", + "biosample", + "blastdbinfo", + "books", + "cdd", + "clinvar", + "gap", + "gapplus", + "grasp", + "dbvar", + "gene", + "gds", + "geoprofiles", + "homologene", + "medgen", + "mesh", + "ncbisearch", + "nlmcatalog", + "omim", + "orgtrack", + "pmc", + "popset", + "proteinclusters", + "pcassay", + "protfam", + "biosystems", + "pccompound", + "pcsubstance", + "seqannot", + "snp", + "sra", + "taxonomy", + "biocollections", + "gtr" + ] }, - "query": { + "entrezId": { "type": "string", - "title": "Query", - "description": "Accession string meaningful to the NCBI Entrez system." + "title": "Database ID", + "description": "Numeric record id meaningful to the NCBI Entrez system.", + "pattern": "^[a-zA-Z0-9]+" }, - "label": { + "entrezLabel": { "description": "Text label to display for the link.", "title": "Label", "type": "string" diff --git a/metadata_backend/helpers/schemas/ena_dataset.json b/metadata_backend/helpers/schemas/ena_dataset.json index 5dbc729f0..4e757dad5 100644 --- a/metadata_backend/helpers/schemas/ena_dataset.json +++ b/metadata_backend/helpers/schemas/ena_dataset.json @@ -21,20 +21,20 @@ "title": "XRef Link", "type": "object", "required": [ - "db", - "id" + "xrefDb", + "xrefId" ], "properties": { - "db": { + "xrefDb": { "type": "string", "title": "Database" }, - "id": { + "xrefId": { "type": "string", - "title": "ID", + "title": "Database ID", "description": "Accession in the referenced database." }, - "label": { + "xrefLabel": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -50,7 +50,7 @@ "url" ], "properties": { - "label": { + "urlLabel": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -68,28 +68,67 @@ "title": "Entrez Link", "type": "object", "required": [ - "db", - "id", - "query" + "entrezDb", + "entrezId" ], "properties": { - "db": { - "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", + "entrezDb": { + "description": "NCBI controlled vocabulary of permitted cross references", "title": "Database", - "type": "string" - }, - "id": { - "type": "number", - "minimum": 0, - "title": "ID", - "description": "Numeric record id meaningful to the NCBI Entrez system." + "type": "string", + "enum": [ + "pubmed", + "protein", + "nuccore", + "ipg", + "nucleotide", + "structure", + "genome", + "annotinfo", + "assembly", + "bioproject", + "biosample", + "blastdbinfo", + "books", + "cdd", + "clinvar", + "gap", + "gapplus", + "grasp", + "dbvar", + "gene", + "gds", + "geoprofiles", + "homologene", + "medgen", + "mesh", + "ncbisearch", + "nlmcatalog", + "omim", + "orgtrack", + "pmc", + "popset", + "proteinclusters", + "pcassay", + "protfam", + "biosystems", + "pccompound", + "pcsubstance", + "seqannot", + "snp", + "sra", + "taxonomy", + "biocollections", + "gtr" + ] }, - "query": { + "entrezId": { "type": "string", - "title": "Query", - "description": "Accession string meaningful to the NCBI Entrez system." + "title": "Database ID", + "description": "Numeric record id meaningful to the NCBI Entrez system.", + "pattern": "^[a-zA-Z0-9]+" }, - "label": { + "entrezLabel": { "description": "Text label to display for the link.", "title": "Label", "type": "string" diff --git a/metadata_backend/helpers/schemas/ena_experiment.json b/metadata_backend/helpers/schemas/ena_experiment.json index 857e4fb02..112f756ae 100644 --- a/metadata_backend/helpers/schemas/ena_experiment.json +++ b/metadata_backend/helpers/schemas/ena_experiment.json @@ -21,20 +21,20 @@ "title": "XRef Link", "type": "object", "required": [ - "db", - "id" + "xrefDb", + "xrefId" ], "properties": { - "db": { + "xrefDb": { "type": "string", "title": "Database" }, - "id": { + "xrefId": { "type": "string", - "title": "ID", + "title": "Database ID", "description": "Accession in the referenced database." }, - "label": { + "xrefLabel": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -50,7 +50,7 @@ "url" ], "properties": { - "label": { + "urlLabel": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -68,28 +68,67 @@ "title": "Entrez Link", "type": "object", "required": [ - "db", - "id", - "query" + "entrezDb", + "entrezId" ], "properties": { - "db": { - "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", + "entrezDb": { + "description": "NCBI controlled vocabulary of permitted cross references", "title": "Database", - "type": "string" - }, - "id": { - "type": "number", - "minimum": 0, - "title": "ID", - "description": "Numeric record id meaningful to the NCBI Entrez system." + "type": "string", + "enum": [ + "pubmed", + "protein", + "nuccore", + "ipg", + "nucleotide", + "structure", + "genome", + "annotinfo", + "assembly", + "bioproject", + "biosample", + "blastdbinfo", + "books", + "cdd", + "clinvar", + "gap", + "gapplus", + "grasp", + "dbvar", + "gene", + "gds", + "geoprofiles", + "homologene", + "medgen", + "mesh", + "ncbisearch", + "nlmcatalog", + "omim", + "orgtrack", + "pmc", + "popset", + "proteinclusters", + "pcassay", + "protfam", + "biosystems", + "pccompound", + "pcsubstance", + "seqannot", + "snp", + "sra", + "taxonomy", + "biocollections", + "gtr" + ] }, - "query": { + "entrezId": { "type": "string", - "title": "Query", - "description": "Accession string meaningful to the NCBI Entrez system." + "title": "Database ID", + "description": "Numeric record id meaningful to the NCBI Entrez system.", + "pattern": "^[a-zA-Z0-9]+" }, - "label": { + "entrezLabel": { "description": "Text label to display for the link.", "title": "Label", "type": "string" diff --git a/metadata_backend/helpers/schemas/ena_policy.json b/metadata_backend/helpers/schemas/ena_policy.json index bb5e0df50..8797a996a 100644 --- a/metadata_backend/helpers/schemas/ena_policy.json +++ b/metadata_backend/helpers/schemas/ena_policy.json @@ -21,20 +21,20 @@ "title": "XRef Link", "type": "object", "required": [ - "db", - "id" + "xrefDb", + "xrefId" ], "properties": { - "db": { + "xrefDb": { "type": "string", "title": "Database" }, - "id": { + "xrefId": { "type": "string", - "title": "ID", + "title": "Database ID", "description": "Accession in the referenced database." }, - "label": { + "xrefLabel": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -50,7 +50,7 @@ "url" ], "properties": { - "label": { + "urlLabel": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -68,28 +68,67 @@ "title": "Entrez Link", "type": "object", "required": [ - "db", - "id", - "query" + "entrezDb", + "entrezId" ], "properties": { - "db": { - "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", + "entrezDb": { + "description": "NCBI controlled vocabulary of permitted cross references", "title": "Database", - "type": "string" - }, - "id": { - "type": "number", - "minimum": 0, - "title": "ID", - "description": "Numeric record id meaningful to the NCBI Entrez system." + "type": "string", + "enum": [ + "pubmed", + "protein", + "nuccore", + "ipg", + "nucleotide", + "structure", + "genome", + "annotinfo", + "assembly", + "bioproject", + "biosample", + "blastdbinfo", + "books", + "cdd", + "clinvar", + "gap", + "gapplus", + "grasp", + "dbvar", + "gene", + "gds", + "geoprofiles", + "homologene", + "medgen", + "mesh", + "ncbisearch", + "nlmcatalog", + "omim", + "orgtrack", + "pmc", + "popset", + "proteinclusters", + "pcassay", + "protfam", + "biosystems", + "pccompound", + "pcsubstance", + "seqannot", + "snp", + "sra", + "taxonomy", + "biocollections", + "gtr" + ] }, - "query": { + "entrezId": { "type": "string", - "title": "Query", - "description": "Accession string meaningful to the NCBI Entrez system." + "title": "Database ID", + "description": "Numeric record id meaningful to the NCBI Entrez system.", + "pattern": "^[a-zA-Z0-9]+" }, - "label": { + "entrezLabel": { "description": "Text label to display for the link.", "title": "Label", "type": "string" diff --git a/metadata_backend/helpers/schemas/ena_run.json b/metadata_backend/helpers/schemas/ena_run.json index 0be2a5240..83cb7c391 100644 --- a/metadata_backend/helpers/schemas/ena_run.json +++ b/metadata_backend/helpers/schemas/ena_run.json @@ -21,20 +21,20 @@ "title": "XRef Link", "type": "object", "required": [ - "db", - "id" + "xrefDb", + "xrefId" ], "properties": { - "db": { + "xrefDb": { "type": "string", "title": "Database" }, - "id": { + "xrefId": { "type": "string", - "title": "ID", + "title": "Database ID", "description": "Accession in the referenced database." }, - "label": { + "xrefLabel": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -50,7 +50,7 @@ "url" ], "properties": { - "label": { + "urlLabel": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -68,28 +68,67 @@ "title": "Entrez Link", "type": "object", "required": [ - "db", - "id", - "query" + "entrezDb", + "entrezId" ], "properties": { - "db": { - "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", + "entrezDb": { + "description": "NCBI controlled vocabulary of permitted cross references", "title": "Database", - "type": "string" - }, - "id": { - "type": "number", - "minimum": 0, - "title": "ID", - "description": "Numeric record id meaningful to the NCBI Entrez system." + "type": "string", + "enum": [ + "pubmed", + "protein", + "nuccore", + "ipg", + "nucleotide", + "structure", + "genome", + "annotinfo", + "assembly", + "bioproject", + "biosample", + "blastdbinfo", + "books", + "cdd", + "clinvar", + "gap", + "gapplus", + "grasp", + "dbvar", + "gene", + "gds", + "geoprofiles", + "homologene", + "medgen", + "mesh", + "ncbisearch", + "nlmcatalog", + "omim", + "orgtrack", + "pmc", + "popset", + "proteinclusters", + "pcassay", + "protfam", + "biosystems", + "pccompound", + "pcsubstance", + "seqannot", + "snp", + "sra", + "taxonomy", + "biocollections", + "gtr" + ] }, - "query": { + "entrezId": { "type": "string", - "title": "Query", - "description": "Accession string meaningful to the NCBI Entrez system." + "title": "Database ID", + "description": "Numeric record id meaningful to the NCBI Entrez system.", + "pattern": "^[a-zA-Z0-9]+" }, - "label": { + "entrezLabel": { "description": "Text label to display for the link.", "title": "Label", "type": "string" diff --git a/metadata_backend/helpers/schemas/ena_sample.json b/metadata_backend/helpers/schemas/ena_sample.json index a9fe9ee08..d6073a0f5 100644 --- a/metadata_backend/helpers/schemas/ena_sample.json +++ b/metadata_backend/helpers/schemas/ena_sample.json @@ -21,20 +21,20 @@ "title": "XRef Link", "type": "object", "required": [ - "db", - "id" + "xrefDb", + "xrefId" ], "properties": { - "db": { + "xrefDb": { "type": "string", "title": "Database" }, - "id": { + "xrefId": { "type": "string", - "title": "ID", + "title": "Database ID", "description": "Accession in the referenced database." }, - "label": { + "xrefLabel": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -50,7 +50,7 @@ "url" ], "properties": { - "label": { + "urlLabel": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -68,28 +68,67 @@ "title": "Entrez Link", "type": "object", "required": [ - "db", - "id", - "query" + "entrezDb", + "entrezId" ], "properties": { - "db": { - "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", + "entrezDb": { + "description": "NCBI controlled vocabulary of permitted cross references", "title": "Database", - "type": "string" - }, - "id": { - "type": "number", - "minimum": 0, - "title": "ID", - "description": "Numeric record id meaningful to the NCBI Entrez system." + "type": "string", + "enum": [ + "pubmed", + "protein", + "nuccore", + "ipg", + "nucleotide", + "structure", + "genome", + "annotinfo", + "assembly", + "bioproject", + "biosample", + "blastdbinfo", + "books", + "cdd", + "clinvar", + "gap", + "gapplus", + "grasp", + "dbvar", + "gene", + "gds", + "geoprofiles", + "homologene", + "medgen", + "mesh", + "ncbisearch", + "nlmcatalog", + "omim", + "orgtrack", + "pmc", + "popset", + "proteinclusters", + "pcassay", + "protfam", + "biosystems", + "pccompound", + "pcsubstance", + "seqannot", + "snp", + "sra", + "taxonomy", + "biocollections", + "gtr" + ] }, - "query": { + "entrezId": { "type": "string", - "title": "Query", - "description": "Accession string meaningful to the NCBI Entrez system." + "title": "Database ID", + "description": "Numeric record id meaningful to the NCBI Entrez system.", + "pattern": "^[a-zA-Z0-9]+" }, - "label": { + "entrezLabel": { "description": "Text label to display for the link.", "title": "Label", "type": "string" diff --git a/metadata_backend/helpers/schemas/ena_study.json b/metadata_backend/helpers/schemas/ena_study.json index 047f93f53..a95650c5a 100644 --- a/metadata_backend/helpers/schemas/ena_study.json +++ b/metadata_backend/helpers/schemas/ena_study.json @@ -21,23 +21,18 @@ "title": "XRef Link", "type": "object", "required": [ - "db", - "id" + "xrefDb", + "xrefId" ], "properties": { - "db": { + "xrefDb": { "type": "string", "title": "Database" }, - "id": { + "xrefId": { "type": "string", - "title": "ID", + "title": "Database ID", "description": "Accession in the referenced database." - }, - "label": { - "type": "string", - "title": "Label", - "description": "Text label to display for the link." } } }, @@ -68,31 +63,65 @@ "title": "Entrez Link", "type": "object", "required": [ - "db", - "id", - "query" + "entrezDb", + "entrezId" ], "properties": { - "db": { - "description": "NCBI controlled vocabulary of permitted cross references. Please see http://www.ncbi.nlm.nih.gov/entrez/eutils/einfo.fcgi? .", + "entrezDb": { + "description": "NCBI controlled vocabulary of permitted cross references", "title": "Database", - "type": "string" - }, - "id": { - "type": "number", - "minimum": 0, - "title": "ID", - "description": "Numeric record id meaningful to the NCBI Entrez system." - }, - "query": { "type": "string", - "title": "Query", - "description": "Accession string meaningful to the NCBI Entrez system." + "enum": [ + "pubmed", + "protein", + "nuccore", + "ipg", + "nucleotide", + "structure", + "genome", + "annotinfo", + "assembly", + "bioproject", + "biosample", + "blastdbinfo", + "books", + "cdd", + "clinvar", + "gap", + "gapplus", + "grasp", + "dbvar", + "gene", + "gds", + "geoprofiles", + "homologene", + "medgen", + "mesh", + "ncbisearch", + "nlmcatalog", + "omim", + "orgtrack", + "pmc", + "popset", + "proteinclusters", + "pcassay", + "protfam", + "biosystems", + "pccompound", + "pcsubstance", + "seqannot", + "snp", + "sra", + "taxonomy", + "biocollections", + "gtr" + ] }, - "label": { - "description": "Text label to display for the link.", - "title": "Label", - "type": "string" + "entrezId": { + "type": "string", + "title": "Database ID", + "description": "Numeric record id meaningful to the NCBI Entrez system.", + "pattern": "^[a-zA-Z0-9]+" } } }, diff --git a/tests/test_parser.py b/tests/test_parser.py index f47cefaff..95d57bfc4 100644 --- a/tests/test_parser.py +++ b/tests/test_parser.py @@ -30,7 +30,7 @@ def test_study_is_parsed(self): study_xml = self.load_xml_from_file("study", "SRP000539.xml") study_json = self.parser.parse("study", study_xml) self.assertIn("Highly integrated epigenome maps in Arabidopsis", study_json["descriptor"]["studyTitle"]) - self.assertIn("18423832", study_json["studyLinks"][0]["id"]) + self.assertIn("18423832", study_json["studyLinks"][0]["xrefId"]) def test_sample_is_parsed(self): """Test that sample is parsed correctly and accessionId is set. From e09346226fbcbf6bbc9f8c4f9e9383251dff12e9 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Tue, 9 Mar 2021 12:06:56 +0200 Subject: [PATCH 20/47] pubmed moved to xref link --- metadata_backend/helpers/schemas/ena_study.json | 6 ------ 1 file changed, 6 deletions(-) diff --git a/metadata_backend/helpers/schemas/ena_study.json b/metadata_backend/helpers/schemas/ena_study.json index a95650c5a..c93359f1d 100644 --- a/metadata_backend/helpers/schemas/ena_study.json +++ b/metadata_backend/helpers/schemas/ena_study.json @@ -209,12 +209,6 @@ "description": "More extensive free-form description of the study.", "type": "string" }, - "pubMedID": { - "type": "string", - "title": "pubMedID identifier", - "description": "PubMed ID (8 digits) or the PMC ID (PMC + 7 digits)", - "pattern": "^[0-9]{8}|PMC[0-9]{7}" - }, "studyLinks": { "type": "array", "title": "Study Links", From 4ae7d4463812a187bcb6c3a1a09981892acfc84a Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Tue, 9 Mar 2021 12:18:23 +0200 Subject: [PATCH 21/47] fix label --- metadata_backend/helpers/schemas/ena_analysis.json | 4 ++-- metadata_backend/helpers/schemas/ena_dac.json | 4 ++-- metadata_backend/helpers/schemas/ena_dataset.json | 4 ++-- metadata_backend/helpers/schemas/ena_experiment.json | 4 ++-- metadata_backend/helpers/schemas/ena_policy.json | 4 ++-- metadata_backend/helpers/schemas/ena_run.json | 4 ++-- metadata_backend/helpers/schemas/ena_sample.json | 4 ++-- metadata_backend/helpers/schemas/ena_study.json | 10 ++++++++++ 8 files changed, 24 insertions(+), 14 deletions(-) diff --git a/metadata_backend/helpers/schemas/ena_analysis.json b/metadata_backend/helpers/schemas/ena_analysis.json index 96e93c845..c7a41ffbd 100644 --- a/metadata_backend/helpers/schemas/ena_analysis.json +++ b/metadata_backend/helpers/schemas/ena_analysis.json @@ -34,7 +34,7 @@ "title": "Database ID", "description": "Accession in the referenced database." }, - "xrefLabel": { + "label": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -128,7 +128,7 @@ "description": "Numeric record id meaningful to the NCBI Entrez system.", "pattern": "^[a-zA-Z0-9]+" }, - "entrezLabel": { + "label": { "description": "Text label to display for the link.", "title": "Label", "type": "string" diff --git a/metadata_backend/helpers/schemas/ena_dac.json b/metadata_backend/helpers/schemas/ena_dac.json index 4f87de220..319d8ef8d 100644 --- a/metadata_backend/helpers/schemas/ena_dac.json +++ b/metadata_backend/helpers/schemas/ena_dac.json @@ -34,7 +34,7 @@ "title": "Database ID", "description": "Accession in the referenced database." }, - "xrefLabel": { + "label": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -128,7 +128,7 @@ "description": "Numeric record id meaningful to the NCBI Entrez system.", "pattern": "^[a-zA-Z0-9]+" }, - "entrezLabel": { + "label": { "description": "Text label to display for the link.", "title": "Label", "type": "string" diff --git a/metadata_backend/helpers/schemas/ena_dataset.json b/metadata_backend/helpers/schemas/ena_dataset.json index 4e757dad5..ebc93772e 100644 --- a/metadata_backend/helpers/schemas/ena_dataset.json +++ b/metadata_backend/helpers/schemas/ena_dataset.json @@ -34,7 +34,7 @@ "title": "Database ID", "description": "Accession in the referenced database." }, - "xrefLabel": { + "label": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -128,7 +128,7 @@ "description": "Numeric record id meaningful to the NCBI Entrez system.", "pattern": "^[a-zA-Z0-9]+" }, - "entrezLabel": { + "label": { "description": "Text label to display for the link.", "title": "Label", "type": "string" diff --git a/metadata_backend/helpers/schemas/ena_experiment.json b/metadata_backend/helpers/schemas/ena_experiment.json index 112f756ae..739c497e5 100644 --- a/metadata_backend/helpers/schemas/ena_experiment.json +++ b/metadata_backend/helpers/schemas/ena_experiment.json @@ -34,7 +34,7 @@ "title": "Database ID", "description": "Accession in the referenced database." }, - "xrefLabel": { + "label": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -128,7 +128,7 @@ "description": "Numeric record id meaningful to the NCBI Entrez system.", "pattern": "^[a-zA-Z0-9]+" }, - "entrezLabel": { + "label": { "description": "Text label to display for the link.", "title": "Label", "type": "string" diff --git a/metadata_backend/helpers/schemas/ena_policy.json b/metadata_backend/helpers/schemas/ena_policy.json index 8797a996a..a4aa96905 100644 --- a/metadata_backend/helpers/schemas/ena_policy.json +++ b/metadata_backend/helpers/schemas/ena_policy.json @@ -34,7 +34,7 @@ "title": "Database ID", "description": "Accession in the referenced database." }, - "xrefLabel": { + "label": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -128,7 +128,7 @@ "description": "Numeric record id meaningful to the NCBI Entrez system.", "pattern": "^[a-zA-Z0-9]+" }, - "entrezLabel": { + "label": { "description": "Text label to display for the link.", "title": "Label", "type": "string" diff --git a/metadata_backend/helpers/schemas/ena_run.json b/metadata_backend/helpers/schemas/ena_run.json index 83cb7c391..395b06dc4 100644 --- a/metadata_backend/helpers/schemas/ena_run.json +++ b/metadata_backend/helpers/schemas/ena_run.json @@ -34,7 +34,7 @@ "title": "Database ID", "description": "Accession in the referenced database." }, - "xrefLabel": { + "label": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -128,7 +128,7 @@ "description": "Numeric record id meaningful to the NCBI Entrez system.", "pattern": "^[a-zA-Z0-9]+" }, - "entrezLabel": { + "label": { "description": "Text label to display for the link.", "title": "Label", "type": "string" diff --git a/metadata_backend/helpers/schemas/ena_sample.json b/metadata_backend/helpers/schemas/ena_sample.json index d6073a0f5..5272a784f 100644 --- a/metadata_backend/helpers/schemas/ena_sample.json +++ b/metadata_backend/helpers/schemas/ena_sample.json @@ -34,7 +34,7 @@ "title": "Database ID", "description": "Accession in the referenced database." }, - "xrefLabel": { + "label": { "type": "string", "title": "Label", "description": "Text label to display for the link." @@ -128,7 +128,7 @@ "description": "Numeric record id meaningful to the NCBI Entrez system.", "pattern": "^[a-zA-Z0-9]+" }, - "entrezLabel": { + "label": { "description": "Text label to display for the link.", "title": "Label", "type": "string" diff --git a/metadata_backend/helpers/schemas/ena_study.json b/metadata_backend/helpers/schemas/ena_study.json index c93359f1d..1c439f32e 100644 --- a/metadata_backend/helpers/schemas/ena_study.json +++ b/metadata_backend/helpers/schemas/ena_study.json @@ -33,6 +33,11 @@ "type": "string", "title": "Database ID", "description": "Accession in the referenced database." + }, + "label": { + "description": "Text label to display for the link.", + "title": "Label", + "type": "string" } } }, @@ -122,6 +127,11 @@ "title": "Database ID", "description": "Numeric record id meaningful to the NCBI Entrez system.", "pattern": "^[a-zA-Z0-9]+" + }, + "label": { + "description": "Text label to display for the link.", + "title": "Label", + "type": "string" } } }, From 02b8a495a755d083c76b1691f5f29d69f3541e23 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Tue, 9 Mar 2021 12:18:31 +0200 Subject: [PATCH 22/47] fix integration tests --- tests/test_files/study/SRP000539.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_files/study/SRP000539.json b/tests/test_files/study/SRP000539.json index 407d2fcce..df663fa8f 100644 --- a/tests/test_files/study/SRP000539.json +++ b/tests/test_files/study/SRP000539.json @@ -23,8 +23,8 @@ }, "studyLinks": [ { - "db": "pubmed", - "id": "18423832" + "xrefDb": "pubmed", + "xrefId": "18423832" } ], "studyAttributes": [ From 0bd08ad43f52387d2a385069e93d29f9b365037a Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Tue, 9 Mar 2021 17:17:40 +0200 Subject: [PATCH 23/47] fix urllabel to lable for urlLink --- metadata_backend/helpers/schemas/ena_analysis.json | 2 +- metadata_backend/helpers/schemas/ena_dac.json | 2 +- metadata_backend/helpers/schemas/ena_dataset.json | 2 +- metadata_backend/helpers/schemas/ena_experiment.json | 2 +- metadata_backend/helpers/schemas/ena_policy.json | 2 +- metadata_backend/helpers/schemas/ena_run.json | 2 +- metadata_backend/helpers/schemas/ena_sample.json | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/metadata_backend/helpers/schemas/ena_analysis.json b/metadata_backend/helpers/schemas/ena_analysis.json index c7a41ffbd..6e96f1666 100644 --- a/metadata_backend/helpers/schemas/ena_analysis.json +++ b/metadata_backend/helpers/schemas/ena_analysis.json @@ -50,7 +50,7 @@ "url" ], "properties": { - "urlLabel": { + "label": { "type": "string", "title": "Label", "description": "Text label to display for the link." diff --git a/metadata_backend/helpers/schemas/ena_dac.json b/metadata_backend/helpers/schemas/ena_dac.json index 319d8ef8d..5d5c01bbf 100644 --- a/metadata_backend/helpers/schemas/ena_dac.json +++ b/metadata_backend/helpers/schemas/ena_dac.json @@ -50,7 +50,7 @@ "url" ], "properties": { - "urlLabel": { + "label": { "type": "string", "title": "Label", "description": "Text label to display for the link." diff --git a/metadata_backend/helpers/schemas/ena_dataset.json b/metadata_backend/helpers/schemas/ena_dataset.json index ebc93772e..ead0f810a 100644 --- a/metadata_backend/helpers/schemas/ena_dataset.json +++ b/metadata_backend/helpers/schemas/ena_dataset.json @@ -50,7 +50,7 @@ "url" ], "properties": { - "urlLabel": { + "label": { "type": "string", "title": "Label", "description": "Text label to display for the link." diff --git a/metadata_backend/helpers/schemas/ena_experiment.json b/metadata_backend/helpers/schemas/ena_experiment.json index 739c497e5..96126388e 100644 --- a/metadata_backend/helpers/schemas/ena_experiment.json +++ b/metadata_backend/helpers/schemas/ena_experiment.json @@ -50,7 +50,7 @@ "url" ], "properties": { - "urlLabel": { + "label": { "type": "string", "title": "Label", "description": "Text label to display for the link." diff --git a/metadata_backend/helpers/schemas/ena_policy.json b/metadata_backend/helpers/schemas/ena_policy.json index a4aa96905..72acc2188 100644 --- a/metadata_backend/helpers/schemas/ena_policy.json +++ b/metadata_backend/helpers/schemas/ena_policy.json @@ -50,7 +50,7 @@ "url" ], "properties": { - "urlLabel": { + "label": { "type": "string", "title": "Label", "description": "Text label to display for the link." diff --git a/metadata_backend/helpers/schemas/ena_run.json b/metadata_backend/helpers/schemas/ena_run.json index 395b06dc4..a54b395f8 100644 --- a/metadata_backend/helpers/schemas/ena_run.json +++ b/metadata_backend/helpers/schemas/ena_run.json @@ -50,7 +50,7 @@ "url" ], "properties": { - "urlLabel": { + "label": { "type": "string", "title": "Label", "description": "Text label to display for the link." diff --git a/metadata_backend/helpers/schemas/ena_sample.json b/metadata_backend/helpers/schemas/ena_sample.json index 5272a784f..0905f0f94 100644 --- a/metadata_backend/helpers/schemas/ena_sample.json +++ b/metadata_backend/helpers/schemas/ena_sample.json @@ -50,7 +50,7 @@ "url" ], "properties": { - "urlLabel": { + "label": { "type": "string", "title": "Label", "description": "Text label to display for the link." From ef2ec3e34fe35d022e09a2a5535cb14098beaf8e Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Thu, 4 Mar 2021 08:13:15 +0200 Subject: [PATCH 24/47] update required packages --- requirements.txt | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/requirements.txt b/requirements.txt index 909cc97c0..a3e1fa6a8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,11 +1,9 @@ -aiohttp -aiohttp-session -cryptography -gunicorn -jsonschema -jsonpatch -motor -python-dateutil -uvloop -xmlschema>=1.2.2 -Authlib +aiohttp==3.7.4 +cryptography==3.4.6 +gunicorn==20.0.4 +jsonschema==3.2.0 +motor==2.3.1 +python-dateutil==2.8.1 +uvloop==0.15.2 +xmlschema==1.5.1 +Authlib==0.15.3 From bb2ef6fa5f22697c00f08048ee213e7c03aaf1b7 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Thu, 4 Mar 2021 11:53:08 +0200 Subject: [PATCH 25/47] install and build frontend for production --- Dockerfile | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/Dockerfile b/Dockerfile index 7bc85b9c6..bac3be5c4 100644 --- a/Dockerfile +++ b/Dockerfile @@ -7,7 +7,10 @@ RUN apk add --update \ RUN git clone https://github.com/CSCfi/metadata-submitter-frontend.git WORKDIR /metadata-submitter-frontend -RUN npm install && npm run build --production +RUN npm install -g npm@7.6.0 \ + && npx --quiet pinst --disable \ + && npm install --production \ + && npm run build --production FROM python:3.7-alpine3.13 as BUILD-BACKEND From cdb463b3a25967cadc9ceb0ee56e9473d7d57923 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Thu, 11 Mar 2021 15:56:51 +0200 Subject: [PATCH 26/47] standardise JSON and XML strings --- docs/frontend.rst | 4 ++-- metadata_backend/api/handlers.py | 6 ++--- metadata_backend/api/operators.py | 21 +++++++++--------- metadata_backend/helpers/logger.py | 4 ++-- metadata_backend/helpers/parser.py | 8 +++---- metadata_backend/helpers/validator.py | 4 ++-- tests/integration/run_tests.py | 4 ++-- tests/test_auth.py | 1 - tests/test_handlers.py | 32 +++++++++++++-------------- tests/test_operators.py | 14 ++++++------ tests/test_parser.py | 16 +++++++------- 11 files changed, 57 insertions(+), 57 deletions(-) diff --git a/docs/frontend.rst b/docs/frontend.rst index 544524e92..075070792 100644 --- a/docs/frontend.rst +++ b/docs/frontend.rst @@ -62,8 +62,8 @@ Form components are crucial part of the application: - All submissions and folder creation are made with `react-hook-form `_. Latter uses form as a reference so submission can be triggered outside the form. -- Form for json schema based forms are created with custom json schema parser, which builds - ``react-hook-form`` based forms from given json schema. Json schema-based forms are validated against json schema with ``Ajv``. +- Form forschema based forms are created with customschema parser, which builds + ``react-hook-form`` based forms from givenschema. The forms are validated againstschema with ``Ajv``. React-hook-form is used for performance reasons: it uses uncontrolled components so adding a lot of fields to array doesn't slow rendering of the application. Redux store diff --git a/metadata_backend/api/handlers.py b/metadata_backend/api/handlers.py index 8ea339443..26e7859de 100644 --- a/metadata_backend/api/handlers.py +++ b/metadata_backend/api/handlers.py @@ -255,7 +255,7 @@ async def get_json_schema(self, req: Request) -> Response: async def get_object(self, req: Request) -> Response: """Get one metadata object by its accession id. - Returns original xml object from backup if format query parameter is + Returns original XML object from backup if format query parameter is set, otherwise json. :param req: GET request @@ -828,7 +828,7 @@ async def submit(self, req: Request) -> Response: return web.Response(body=body, status=200, content_type="application/json") async def validate(self, req: Request) -> Response: - """Handle validating an xml file sent to endpoint. + """Handle validating an XML file sent to endpoint. :param req: Multipart POST request with submission.xml and files :returns: JSON response indicating if validation was successful or not @@ -902,7 +902,7 @@ async def _execute_action(self, schema: str, content: str, db_client: AsyncIOMot return json.loads(validator.resp_body) else: - reason = f"Action {action} in xml is not supported." + reason = f"Action {action} in XML is not supported." LOG.error(reason) raise web.HTTPBadRequest(reason=reason) diff --git a/metadata_backend/api/operators.py b/metadata_backend/api/operators.py index 303d5fc01..00980c5a2 100644 --- a/metadata_backend/api/operators.py +++ b/metadata_backend/api/operators.py @@ -392,8 +392,8 @@ async def _format_data_to_create_and_add_to_db(self, schema_type: str, data: Dic Adds necessary additional information to object before adding to db. - If schema type is study, publishDate and status is added. - By default date is two months from submission date (based on ENA + If schema type is study, publishDate and status are added. + By default, date is two months from submission date (based on ENA submission model). :param schema_type: Schema type of the object to create. @@ -415,7 +415,7 @@ async def _format_data_to_replace_and_add_to_db(self, schema_type: str, accessio Replace information to object before adding to db. We will not replace accessionId, publishDate or dateCreated, - as these should are generated when created. + as these are generated when created. We will keep also publisDate and dateCreated from old object. @@ -507,6 +507,7 @@ def format_date(key: str, doc: Dict) -> Dict: class XMLOperator(BaseOperator): """Alternative operator class for handling database operations. + We store the XML data in a database ``XML-{schema}``. Operations are implemented with XML format. """ @@ -522,11 +523,11 @@ def __init__(self, db_client: AsyncIOMotorClient) -> None: async def _format_data_to_create_and_add_to_db(self, schema_type: str, data: str) -> str: """Format XML metadata object and add it to db. - XML is validated, then parsed to json and json is added to database. - After successful json insertion, xml itself is backed up to database. + XML is validated, then parsed to JSON, which is added to database. + After successful JSON insertion, XML itself is backed up to database. :param schema_type: Schema type of the object to read. - :param data: Original xml content + :param data: Original XML content :returns: Accession Id for object inserted to database """ db_client = self.db_service.db_client @@ -542,12 +543,12 @@ async def _format_data_to_create_and_add_to_db(self, schema_type: str, data: str async def _format_data_to_replace_and_add_to_db(self, schema_type: str, accession_id: str, data: str) -> str: """Format XML metadata object and add it to db. - XML is validated, then parsed to json and json is added to database. - After successful json insertion, xml itself is backed up to database. + XML is validated, then parsed to JSON, which is added to database. + After successful JSON insertion, XML itself is backed up to database. :param schema_type: Schema type of the object to replace. :param accession_id: Identifier of object to replace. - :param data: Original xml content + :param data: Original XML content :returns: Accession Id for object inserted to database """ db_client = self.db_service.db_client @@ -569,7 +570,7 @@ async def _format_data_to_update_and_add_to_db(self, schema_type: str, accession :param schema_type: Schema type of the object to replace. :param accession_id: Identifier of object to replace. - :param data: Original xml content + :param data: Original XML content :raises: HTTPUnsupportedMediaType """ reason = "XML patching is not possible." diff --git a/metadata_backend/helpers/logger.py b/metadata_backend/helpers/logger.py index 602fcbb8c..ffb86ed51 100644 --- a/metadata_backend/helpers/logger.py +++ b/metadata_backend/helpers/logger.py @@ -27,8 +27,8 @@ def get_attributes(obj: Any) -> None: def pprint_json(content: Dict) -> None: - """Print given json object to LOG. + """Print given JSON object to LOG. - :param content: json-formatted content to be printed + :param content: JSON-formatted content to be printed """ LOG.info(json.dumps(content, indent=4)) diff --git a/metadata_backend/helpers/parser.py b/metadata_backend/helpers/parser.py index 260641955..2441f4604 100644 --- a/metadata_backend/helpers/parser.py +++ b/metadata_backend/helpers/parser.py @@ -134,7 +134,7 @@ def _flatten(self, data: Any) -> Union[Dict, List, str, None]: @property def lossy(self) -> bool: - """Define that converter is lossy, xml structure can't be restored.""" + """Define that converter is lossy, XML structure can't be restored.""" return True def element_decode( @@ -154,7 +154,7 @@ def element_decode( - All "accession" keys are converted to "accesionId", key used by this program Corner cases: - - If possible, self-closing xml tag is elevated as an attribute to its + - If possible, self-closing XML tag is elevated as an attribute to its parent, otherwise "true" is added as its value. - If there is just one children and it is string, it is appended to same dictionary with its parents attributes with "value" as its key. @@ -197,10 +197,10 @@ def element_decode( class XMLToJSONParser: - """Methods to parse necessary data from different xml types.""" + """Methods to parse necessary data from different XML types.""" def parse(self, schema_type: str, content: str) -> Dict: - """Validate xml file and parse it to json. + """Validate XML file and parse it to JSON. We validate resulting JSON against a JSON schema to be sure the resulting content is consistent. diff --git a/metadata_backend/helpers/validator.py b/metadata_backend/helpers/validator.py index d1c443b84..19fba7e81 100644 --- a/metadata_backend/helpers/validator.py +++ b/metadata_backend/helpers/validator.py @@ -140,11 +140,11 @@ def validate(self) -> None: except ValidationError as e: if len(e.path) > 0: reason = f"Provided input does not seem correct for field: '{e.path[0]}'" - LOG.debug(f"Provided json input: '{e.instance}'") + LOG.debug(f"Provided JSON input: '{e.instance}'") LOG.error(reason) raise web.HTTPBadRequest(reason=reason) else: reason = f"Provided input does not seem correct because: '{e.message}'" - LOG.debug(f"Provided json input: '{e.instance}'") + LOG.debug(f"Provided JSON input: '{e.instance}'") LOG.error(reason) raise web.HTTPBadRequest(reason=reason) diff --git a/tests/integration/run_tests.py b/tests/integration/run_tests.py index 4f9cf1c06..a6777d587 100644 --- a/tests/integration/run_tests.py +++ b/tests/integration/run_tests.py @@ -415,7 +415,7 @@ async def test_put_objects(sess, folder_id): Tries to create new object, gets accession id and checks if correct resource is returned with that id. Try to use PUT with JSON and expect failure, - try to use PUT with xml and expect success. + try to use PUT with XML and expect success. :param sess: HTTP session in which request call is made :param folder_id: id of the folder used to group submission @@ -874,7 +874,7 @@ async def test_get_folders_objects(sess, folder_id: str): async def test_submissions_work(sess, folder_id): - """Test actions in submission xml files. + """Test actions in submission XML files. :param sess: HTTP session in which request call is made :param folder_id: id of the folder used to group submission objects diff --git a/tests/test_auth.py b/tests/test_auth.py index f6cff5a99..459224a41 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -75,7 +75,6 @@ async def test_callback_(self): @unittest_run_loop async def test_logout_works(self): """Test that logout revokes all tokens.""" - request = get_request_with_fernet() request.app["Crypt"] = self.client.app["Crypt"] cookie, cookiestring = generate_cookie(request) diff --git a/tests/test_handlers.py b/tests/test_handlers.py index 134690317..009169c42 100644 --- a/tests/test_handlers.py +++ b/tests/test_handlers.py @@ -130,11 +130,11 @@ def create_submission_data(self, files): return data async def fake_operator_read_metadata_object(self, schema_type, accession_id): - """Fake read operation to return mocked json.""" + """Fake read operation to return mocked JSON.""" return await futurized((self.metadata_json, "application/json")) async def fake_operator_query_metadata_object(self, schema_type, query, page_num, page_size, filtered_list): - """Fake query operation to return list containing mocked json.""" + """Fake query operation to return list containing mocked JSON.""" return await futurized( ([self.metadata_json], self.page_num, self.page_size, self.total_objects), ) @@ -289,7 +289,7 @@ async def test_submit_object_works(self): @unittest_run_loop async def test_submit_object_works_with_json(self): - """Test that json submission is handled, operator is called.""" + """Test that JSON submission is handled, operator is called.""" json_req = { "centerName": "GEO", "alias": "GSE10966", @@ -302,7 +302,7 @@ async def test_submit_object_works_with_json(self): @unittest_run_loop async def test_submit_object_missing_field_json(self): - """Test that json has missing property.""" + """Test that JSON has missing property.""" json_req = {"centerName": "GEO", "alias": "GSE10966"} response = await self.client.post("/objects/study", json=json_req) reason = "Provided input does not seem correct because: " "''descriptor' is a required property'" @@ -311,7 +311,7 @@ async def test_submit_object_missing_field_json(self): @unittest_run_loop async def test_submit_object_bad_field_json(self): - """Test that json has bad studyType.""" + """Test that JSON has bad studyType.""" json_req = { "centerName": "GEO", "alias": "GSE10966", @@ -324,7 +324,7 @@ async def test_submit_object_bad_field_json(self): @unittest_run_loop async def test_post_object_bad_json(self): - """Test that post json is badly formated.""" + """Test that post JSON is badly formated.""" json_req = { "centerName": "GEO", "alias": "GSE10966", @@ -337,7 +337,7 @@ async def test_post_object_bad_json(self): @unittest_run_loop async def test_put_object_bad_json(self): - """Test that put json is badly formated.""" + """Test that put JSON is badly formated.""" json_req = { "centerName": "GEO", "alias": "GSE10966", @@ -351,7 +351,7 @@ async def test_put_object_bad_json(self): @unittest_run_loop async def test_patch_object_bad_json(self): - """Test that patch json is badly formated.""" + """Test that patch JSON is badly formated.""" json_req = {"centerName": "GEO", "alias": "GSE10966"} call = "/drafts/study/EGA123456" response = await self.client.patch(call, data=json_req) @@ -361,7 +361,7 @@ async def test_patch_object_bad_json(self): @unittest_run_loop async def test_submit_draft_works_with_json(self): - """Test that draft json submission is handled, operator is called.""" + """Test that draft JSON submission is handled, operator is called.""" json_req = { "centerName": "GEO", "alias": "GSE10966", @@ -374,7 +374,7 @@ async def test_submit_draft_works_with_json(self): @unittest_run_loop async def test_put_draft_works_with_json(self): - """Test that draft json put method is handled, operator is called.""" + """Test that draft JSON put method is handled, operator is called.""" json_req = { "centerName": "GEO", "alias": "GSE10966", @@ -399,7 +399,7 @@ async def test_put_draft_works_with_xml(self): @unittest_run_loop async def test_patch_draft_works_with_json(self): - """Test that draft json patch method is handled, operator is called.""" + """Test that draft JSON patch method is handled, operator is called.""" json_req = {"centerName": "GEO", "alias": "GSE10966"} call = "/drafts/study/EGA123456" response = await self.client.patch(call, json=json_req) @@ -428,7 +428,7 @@ async def test_submit_object_fails_with_too_many_files(self): @unittest_run_loop async def test_get_object(self): - """Test that accessionId returns correct json object.""" + """Test that accessionId returns correct JSON object.""" url = f"/objects/study/{self.query_accessionId}" response = await self.client.get(url) self.assertEqual(response.status, 200) @@ -437,7 +437,7 @@ async def test_get_object(self): @unittest_run_loop async def test_get_draft_object(self): - """Test that draft accessionId returns correct json object.""" + """Test that draft accessionId returns correct JSON object.""" url = f"/drafts/study/{self.query_accessionId}" response = await self.client.get(url) self.assertEqual(response.status, 200) @@ -446,7 +446,7 @@ async def test_get_draft_object(self): @unittest_run_loop async def test_get_object_as_xml(self): - """Test that accessionId with xml query returns xml object.""" + """Test that accessionId with XML query returns XML object.""" url = f"/objects/study/{self.query_accessionId}" response = await self.client.get(f"{url}?format=xml") self.assertEqual(response.status, 200) @@ -455,7 +455,7 @@ async def test_get_object_as_xml(self): @unittest_run_loop async def test_query_is_called_and_returns_json_in_correct_format(self): - """Test query method calls operator and returns mocked json object.""" + """Test query method calls operator and returns mocked JSON object.""" url = f"/objects/study?studyType=foo&name=bar&page={self.page_num}" f"&per_page={self.page_size}" response = await self.client.get(url) self.assertEqual(response.status, 200) @@ -509,7 +509,7 @@ async def test_validation_fails_bad_schema(self): @unittest_run_loop async def test_validation_fails_for_invalid_xml_syntax(self): - """Test validation endpoint for xml with bad syntax.""" + """Test validation endpoint for XML with bad syntax.""" files = [("study", "SRP000539_invalid.xml")] data = self.create_submission_data(files) response = await self.client.post("/validate", data=data) diff --git a/tests/test_operators.py b/tests/test_operators.py index c4788c557..f3e1bb734 100644 --- a/tests/test_operators.py +++ b/tests/test_operators.py @@ -109,7 +109,7 @@ def tearDown(self): self.patch_user.stop() async def test_reading_metadata_works(self): - """Test json is read from db correctly.""" + """Test JSON is read from db correctly.""" operator = Operator(self.client) data = { "dateCreated": datetime.datetime(2020, 6, 14, 0, 0), @@ -132,7 +132,7 @@ async def test_reading_metadata_works(self): ) async def test_reading_metadata_works_with_xml(self): - """Test xml is read from db correctly.""" + """Test XML is read from db correctly.""" operator = XMLOperator(self.client) data = {"accessionId": "EGA123456", "content": ""} operator.db_service.read.return_value = futurized(data) @@ -171,7 +171,7 @@ def test_operator_fixes_single_document_presentation(self): result["_Id"] async def test_json_create_passes_and_returns_accessionId(self): - """Test create method for json works.""" + """Test create method for JSON works.""" operator = Operator(self.client) data = { "centerName": "GEO", @@ -184,7 +184,7 @@ async def test_json_create_passes_and_returns_accessionId(self): self.assertEqual(accession, self.accession_id) async def test_json_replace_passes_and_returns_accessionId(self): - """Test replace method for json works.""" + """Test replace method for JSON works.""" data = { "centerName": "GEO", "alias": "GSE10966", @@ -215,7 +215,7 @@ async def test_db_error_replace_raises_400_error(self): await operator.replace_metadata_object("study", self.accession_id, {}) async def test_json_update_passes_and_returns_accessionId(self): - """Test update method for json works.""" + """Test update method for JSON works.""" data = {"centerName": "GEOM", "alias": "GSE10967"} db_data = { "centerName": "GEO", @@ -248,7 +248,7 @@ async def test_db_error_update_raises_400_error(self): await operator.update_metadata_object("study", self.accession_id, {}) async def test_xml_create_passes_and_returns_accessionId(self): - """Test create method for xml works. Patch json related calls.""" + """Test create method for XML works. Patch JSON related calls.""" operator = XMLOperator(self.client) operator.db_service.db_client = self.client operator.db_service.create.return_value = futurized(True) @@ -528,7 +528,7 @@ async def test_non_working_query_params_are_not_passed_to_db_query(self): self.assertEqual(operator.db_service.aggregate.call_count, 2) async def test_query_result_is_parsed_correctly(self): - """Test json is read and correct pagination values are returned.""" + """Test JSON is read and correct pagination values are returned.""" operator = Operator(self.client) multiple_result = [ { diff --git a/tests/test_parser.py b/tests/test_parser.py index 95d57bfc4..9bfe386c5 100644 --- a/tests/test_parser.py +++ b/tests/test_parser.py @@ -18,14 +18,14 @@ def setUp(self): self.parser = XMLToJSONParser() def load_xml_from_file(self, submission, filename): - """Load xml as string from given file.""" + """Load XML as string from given file.""" path_to_xml_file = self.TESTFILES_ROOT / submission / filename return path_to_xml_file.read_text() def test_study_is_parsed(self): """Test that study is parsed correctly. - Tests for some values that converted json should have. + Tests for some values that converted JSON should have. """ study_xml = self.load_xml_from_file("study", "SRP000539.xml") study_json = self.parser.parse("study", study_xml) @@ -35,7 +35,7 @@ def test_study_is_parsed(self): def test_sample_is_parsed(self): """Test that sample is parsed correctly and accessionId is set. - Tests for some values that converted json should have. + Tests for some values that converted JSON should have. """ sample_xml = self.load_xml_from_file("sample", "SRS001433.xml") sample_json = self.parser.parse("sample", sample_xml) @@ -45,7 +45,7 @@ def test_sample_is_parsed(self): def test_experiment_is_parsed(self): """Test that experiment is parsed correctly and accessionId is set. - Tests for some values that convert json should have. + Tests for some values that convert JSON should have. """ experiment_xml = self.load_xml_from_file("experiment", "ERX000119.xml") experiment_json = self.parser.parse("experiment", experiment_xml) @@ -56,7 +56,7 @@ def test_experiment_is_parsed(self): def test_run_is_parsed(self): """Test that run is parsed correctly and accessionId is set. - Tests for some values that convert json should have. + Tests for some values that convert JSON should have. """ run_xml = self.load_xml_from_file("run", "ERR000076.xml") run_json = self.parser.parse("run", run_xml) @@ -66,7 +66,7 @@ def test_run_is_parsed(self): def test_analysis_is_parsed(self): """Test that run is parsed correctly and accessionId is set. - Tests for some values that convert json should have. + Tests for some values that convert JSON should have. """ analysis_xml = self.load_xml_from_file("analysis", "ERZ266973.xml") analysis_json = self.parser.parse("analysis", analysis_xml) @@ -90,13 +90,13 @@ def test_error_raised_when_schema_not_found(self): self.parser._load_schema("None") def test_error_raised_when_input_xml_not_valid_xml(self): - """Give parser xml with broken syntax, should fail.""" + """Give parser XML with broken syntax, should fail.""" study_xml = self.load_xml_from_file("study", "SRP000539_invalid.xml") with self.assertRaises(web.HTTPBadRequest): self.parser.parse("study", study_xml) def test_json_patch_mongo_conversion(self): - """Test json patch to mongo query conversion.""" + """Test JSON patch to mongo query conversion.""" json_patch = [ { "op": "add", From edb0d10bb37cbf18daa8dd15ce333575f3c53c28 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Thu, 11 Mar 2021 18:35:22 +0200 Subject: [PATCH 27/47] refactor eppn function name to externalId we are using either eppn or sub so let us make it more general --- metadata_backend/api/operators.py | 10 ++++++---- metadata_backend/database/db_service.py | 10 +++++----- tests/test_db_service.py | 16 ++++++++-------- tests/test_operators.py | 12 ++++++------ 4 files changed, 25 insertions(+), 23 deletions(-) diff --git a/metadata_backend/api/operators.py b/metadata_backend/api/operators.py index 00980c5a2..e4cfc186a 100644 --- a/metadata_backend/api/operators.py +++ b/metadata_backend/api/operators.py @@ -21,6 +21,8 @@ class BaseOperator(ABC): """Base class for operators, implements shared functionality. + This BaseOperator is mainly addressed for working with objects owned by + a user and that are clustered by folder. :param ABC: The abstract base class """ @@ -870,19 +872,19 @@ async def create_user(self, data: Tuple) -> str: """ user_data: Dict[str, Union[list, str]] = dict() - eppn = data[0] # this also can be sub key + externalId = data[0] # this also can be sub key name = data[1] try: - existing_user_id = await self.db_service.exists_eppn_user(eppn, name) + existing_user_id = await self.db_service.exists_user_by_externalId(externalId, name) if existing_user_id: - LOG.info(f"User with identifier: {eppn} exists, no need to create.") + LOG.info(f"User with identifier: {externalId} exists, no need to create.") return existing_user_id else: user_data["drafts"] = [] user_data["folders"] = [] user_data["userId"] = user_id = self._generate_user_id() user_data["name"] = name - user_data["eppn"] = eppn + user_data["externalId"] = externalId insert_success = await self.db_service.create("user", user_data) if not insert_success: reason = "Inserting user to database failed for some reason." diff --git a/metadata_backend/database/db_service.py b/metadata_backend/database/db_service.py index dded114a9..d2da06920 100644 --- a/metadata_backend/database/db_service.py +++ b/metadata_backend/database/db_service.py @@ -85,22 +85,22 @@ async def exists(self, collection: str, accession_id: str) -> bool: :returns: True if exists and False if it does not """ id_key = f"{collection}Id" if (collection in ["folder", "user"]) else "accessionId" - projection = {"_id": False, "eppn": False} if collection == "user" else {"_id": False} + projection = {"_id": False, "externalId": False} if collection == "user" else {"_id": False} find_by_id = {id_key: accession_id} exists = await self.database[collection].find_one(find_by_id, projection) LOG.debug(f"DB check exists for {accession_id} in collection {collection}.") return True if exists else False @auto_reconnect - async def exists_eppn_user(self, eppn: str, name: str) -> Union[None, str]: + async def exists_user_by_externalId(self, externalId: str, name: str) -> Union[None, str]: """Check user exists by its eppn. :param eppn: eduPersonPrincipalName to be searched :returns: True if exists and False if it does not """ - find_by_id = {"eppn": eppn, "name": name} - user = await self.database["user"].find_one(find_by_id, {"_id": False, "eppn": False}) - LOG.debug(f"DB check user exists for {eppn} returned {user}.") + find_by_id = {"externalId": externalId, "name": name} + user = await self.database["user"].find_one(find_by_id, {"_id": False, "externalId": False}) + LOG.debug(f"DB check user exists for {externalId} returned {user}.") return user["userId"] if user else None @auto_reconnect diff --git a/tests/test_db_service.py b/tests/test_db_service.py index 5887c5810..686d8e5d2 100644 --- a/tests/test_db_service.py +++ b/tests/test_db_service.py @@ -173,21 +173,21 @@ async def test_read_folder_returns_data(self): self.collection.find_one.assert_called_once_with({"folderId": self.f_id_stub}, {"_id": False}) self.assertEqual(found_folder, self.folder_stub) - async def test_eppn_exists_returns_false(self): - """Test that eppn exists method works and returns None.""" - found_doc = await self.test_service.exists_eppn_user("test_user@eppn.fi", "name") + async def test_externalId_exists_returns_false(self): + """Test that externalId exists method works and returns None.""" + found_doc = await self.test_service.exists_user_by_externalId("test_user@eppn.fi", "name") self.assertEqual(found_doc, None) self.collection.find_one.assert_called_once_with( - {"eppn": "test_user@eppn.fi", "name": "name"}, {"_id": False, "eppn": False} + {"externalId": "test_user@eppn.fi", "name": "name"}, {"_id": False, "externalId": False} ) - async def test_eppn_exists_returns_true(self): - """Test that eppn exists method works and returns user id.""" + async def test_externalId_exists_returns_true(self): + """Test that externalId exists method works and returns user id.""" self.collection.find_one.return_value = futurized(self.user_stub) - found_doc = await self.test_service.exists_eppn_user("test_user@eppn.fi", "name") + found_doc = await self.test_service.exists_user_by_externalId("test_user@eppn.fi", "name") self.assertEqual(found_doc, self.user_id_stub) self.collection.find_one.assert_called_once_with( - {"eppn": "test_user@eppn.fi", "name": "name"}, {"_id": False, "eppn": False} + {"externalId": "test_user@eppn.fi", "name": "name"}, {"_id": False, "externalId": False} ) async def test_aggregate_performed(self): diff --git a/tests/test_operators.py b/tests/test_operators.py index f3e1bb734..cee020585 100644 --- a/tests/test_operators.py +++ b/tests/test_operators.py @@ -804,8 +804,8 @@ async def test_delete_folder_fails(self): async def test_create_user_works_and_returns_userId(self): """Test create method for users work.""" operator = UserOperator(self.client) - data = "eppn", "name" - operator.db_service.exists_eppn_user.return_value = futurized(None) + data = "externalId", "name" + operator.db_service.exists_user_by_externalId.return_value = futurized(None) operator.db_service.create.return_value = futurized(True) user = await operator.create_user(data) operator.db_service.create.assert_called_once() @@ -814,8 +814,8 @@ async def test_create_user_works_and_returns_userId(self): async def test_create_user_on_create_fails(self): """Test create method fails on db create.""" operator = UserOperator(self.client) - data = "eppn", "name" - operator.db_service.exists_eppn_user.return_value = futurized(None) + data = "externalId", "name" + operator.db_service.exists_user_by_externalId.return_value = futurized(None) operator.db_service.create.return_value = futurized(False) with self.assertRaises(HTTPBadRequest): await operator.create_user(data) @@ -862,7 +862,7 @@ async def test_create_user_works_existing_userId(self): """Test create method for existing user.""" operator = UserOperator(self.client) data = "eppn", "name" - operator.db_service.exists_eppn_user.return_value = futurized(self.user_generated_id) + operator.db_service.exists_user_by_externalId.return_value = futurized(self.user_generated_id) user = await operator.create_user(data) operator.db_service.create.assert_not_called() self.assertEqual(user, self.user_generated_id) @@ -871,7 +871,7 @@ async def test_create_user_fails(self): """Test create user fails.""" data = "eppn", "name" operator = UserOperator(self.client) - operator.db_service.exists_eppn_user.side_effect = ConnectionFailure + operator.db_service.exists_user_by_externalId.side_effect = ConnectionFailure with self.assertRaises(HTTPBadRequest): await operator.create_user(data) From f844dd6cbd4b2f80199322052ba5096e063ef063 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Thu, 11 Mar 2021 18:35:34 +0200 Subject: [PATCH 28/47] refine json schema for forlder and users --- metadata_backend/helpers/schemas/folders.json | 8 +++++--- metadata_backend/helpers/schemas/users.json | 16 ++++++++-------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/metadata_backend/helpers/schemas/folders.json b/metadata_backend/helpers/schemas/folders.json index 9a309bb06..a78126df6 100644 --- a/metadata_backend/helpers/schemas/folders.json +++ b/metadata_backend/helpers/schemas/folders.json @@ -57,7 +57,8 @@ } } } - } + }, + "uniqueItems": true }, "drafts": { "type": "array", @@ -94,8 +95,9 @@ } } } - } + }, + "uniqueItems": true } }, "additionalProperties": false -} \ No newline at end of file +} diff --git a/metadata_backend/helpers/schemas/users.json b/metadata_backend/helpers/schemas/users.json index dccd7b59e..d17a45f87 100644 --- a/metadata_backend/helpers/schemas/users.json +++ b/metadata_backend/helpers/schemas/users.json @@ -40,17 +40,17 @@ "properties": {} } } - } + }, + "uniqueItems": true }, "folders": { "type": "array", "title": "The folders schema", - "items": [ - { - "type": "string", - "title": "Folder Id" - } - ] + "items": { + "type": "string", + "title": "Folder Id" + }, + "uniqueItems": true } } -} \ No newline at end of file +} From 99b847918c56973865857c925563c88d67da43ba Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Thu, 11 Mar 2021 18:57:22 +0200 Subject: [PATCH 29/47] no use fo JSON validate if data already inserted No need to read data and then validate it if data has been updated, the sanity check does not provide any role. --- metadata_backend/api/handlers.py | 3 +-- metadata_backend/api/operators.py | 36 +++++++++---------------------- tests/test_operators.py | 7 ------ 3 files changed, 11 insertions(+), 35 deletions(-) diff --git a/metadata_backend/api/handlers.py b/metadata_backend/api/handlers.py index 26e7859de..2eab91a3d 100644 --- a/metadata_backend/api/handlers.py +++ b/metadata_backend/api/handlers.py @@ -165,7 +165,7 @@ def get_page_param(param_name: str, default: int) -> int: try: param = int(req.query.get(param_name, default)) except ValueError: - reason = f"{param_name} must be a number, now it is " f"{req.query.get(param_name)}" + reason = f"{param_name} must be a number, now it is {req.query.get(param_name)}" LOG.error(reason) raise web.HTTPBadRequest(reason=reason) if param < 1: @@ -920,7 +920,6 @@ async def frontend(self, req: Request) -> Response: :param req: GET request :returns: Response containing frontpage static file """ - serve_path = self.path.joinpath("./" + req.path) if not serve_path.exists() or not serve_path.is_file(): diff --git a/metadata_backend/api/operators.py b/metadata_backend/api/operators.py index e4cfc186a..d0b311e42 100644 --- a/metadata_backend/api/operators.py +++ b/metadata_backend/api/operators.py @@ -49,9 +49,7 @@ async def create_metadata_object(self, schema_type: str, data: Union[Dict, str]) :returns: Accession id for the object inserted to database """ accession_id = await self._format_data_to_create_and_add_to_db(schema_type, data) - LOG.info( - f"Inserting object with schema {schema_type} to database " f"succeeded with accession id: {accession_id}" - ) + LOG.info(f"Inserting object with schema {schema_type} to database succeeded with accession id: {accession_id}") return accession_id async def replace_metadata_object(self, schema_type: str, accession_id: str, data: Union[Dict, str]) -> str: @@ -66,9 +64,7 @@ async def replace_metadata_object(self, schema_type: str, accession_id: str, dat :returns: Accession id for the object replaced to database """ await self._format_data_to_replace_and_add_to_db(schema_type, accession_id, data) - LOG.info( - f"Replacing object with schema {schema_type} to database " f"succeeded with accession id: {accession_id}" - ) + LOG.info(f"Replacing object with schema {schema_type} to database succeeded with accession id: {accession_id}") return accession_id async def update_metadata_object(self, schema_type: str, accession_id: str, data: Union[Dict, str]) -> str: @@ -83,9 +79,7 @@ async def update_metadata_object(self, schema_type: str, accession_id: str, data :returns: Accession id for the object updated to database """ await self._format_data_to_update_and_add_to_db(schema_type, accession_id, data) - LOG.info( - f"Updated object with schema {schema_type} to database " f"succeeded with accession id: {accession_id}" - ) + LOG.info(f"Updated object with schema {schema_type} to database succeeded with accession id: {accession_id}") return accession_id async def read_metadata_object(self, schema_type: str, accession_id: str) -> Tuple[Union[Dict, str], str]: @@ -200,10 +194,6 @@ async def _update_object_from_db(self, schema_type: str, accession_id: str, data LOG.error(reason) raise web.HTTPNotFound(reason=reason) update_success = await self.db_service.update(schema_type, accession_id, data) - sanity_check = await self.db_service.read(schema_type, accession_id) - # remove `draft-` from schema type - if not schema_type.startswith("draft"): - JSONValidator(sanity_check, schema_type).validate except (ConnectionFailure, OperationFailure) as error: reason = f"Error happened while getting object: {error}" LOG.error(reason) @@ -721,8 +711,6 @@ async def update_folder(self, folder_id: str, patch: List) -> str: """ try: update_success = await self.db_service.patch("folder", folder_id, patch) - sanity_check = await self.db_service.read("folder", folder_id) - JSONValidator(sanity_check, "folders").validate except (ConnectionFailure, OperationFailure) as error: reason = f"Error happened while getting folder: {error}" LOG.error(reason) @@ -748,8 +736,7 @@ async def remove_object(self, folder_id: str, collection: str, accession_id: str try: folder_path = "drafts" if collection.startswith("draft") else "metadataObjects" upd_content = {folder_path: {"accessionId": accession_id}} - result = await self.db_service.remove("folder", folder_id, upd_content) - JSONValidator(result, "folders").validate + await self.db_service.remove("folder", folder_id, upd_content) except (ConnectionFailure, OperationFailure) as error: reason = f"Error happened while getting user: {error}" LOG.error(reason) @@ -885,6 +872,7 @@ async def create_user(self, data: Tuple) -> str: user_data["userId"] = user_id = self._generate_user_id() user_data["name"] = name user_data["externalId"] = externalId + JSONValidator(user_data, "users") insert_success = await self.db_service.create("user", user_data) if not insert_success: reason = "Inserting user to database failed for some reason." @@ -924,8 +912,6 @@ async def update_user(self, user_id: str, patch: List) -> str: try: await self._check_user_exists(user_id) update_success = await self.db_service.patch("user", user_id, patch) - sanity_check = await self.db_service.read("user", user_id) - JSONValidator(sanity_check, "users").validate except (ConnectionFailure, OperationFailure) as error: reason = f"Error happened while getting user: {error}" LOG.error(reason) @@ -953,14 +939,14 @@ async def assign_objects(self, user_id: str, collection: str, object_ids: List) try: await self._check_user_exists(user_id) upd_content = {collection: {"$each": object_ids}} - result = await self.db_service.append("user", user_id, upd_content) - JSONValidator(result, "users").validate + assign_success = await self.db_service.append("user", user_id, upd_content) except (ConnectionFailure, OperationFailure) as error: reason = f"Error happened while getting user: {error}" LOG.error(reason) raise web.HTTPBadRequest(reason=reason) - except Exception as e: - reason = f"Updating user to database failed beacause of: {e}." + + if not assign_success: + reason = "Assigning objects to user failed." LOG.error(reason) raise web.HTTPBadRequest(reason=reason) @@ -985,9 +971,7 @@ async def remove_objects(self, user_id: str, collection: str, object_ids: List) remove_content = {"drafts": {"accessionId": obj}} else: remove_content = {"folders": obj} - result = await self.db_service.remove("user", user_id, remove_content) - LOG.info(f"result {result}") - JSONValidator(result, "users").validate + await self.db_service.remove("user", user_id, remove_content) except (ConnectionFailure, OperationFailure) as error: reason = f"Error happened while removing objects from user: {error}" LOG.error(reason) diff --git a/tests/test_operators.py b/tests/test_operators.py index cee020585..0ae87c16d 100644 --- a/tests/test_operators.py +++ b/tests/test_operators.py @@ -731,11 +731,9 @@ async def test_folder_update_passes_and_returns_id(self): """Test update method for folders works.""" patch = [{"op": "add", "path": "/name", "value": "test2"}] operator = FolderOperator(self.client) - operator.db_service.read.return_value = futurized(self.test_folder) operator.db_service.patch.return_value = futurized(True) folder = await operator.update_folder(self.test_folder, patch) operator.db_service.patch.assert_called_once() - self.assertEqual(len(operator.db_service.read.mock_calls), 1) self.assertEqual(folder["folderId"], self.folder_id) async def test_folder_update_fails_with_bad_patch(self): @@ -745,7 +743,6 @@ async def test_folder_update_fails_with_bad_patch(self): operator.db_service.read.return_value = futurized(self.test_folder) with self.assertRaises(HTTPBadRequest): await operator.update_folder(self.test_folder, patch) - operator.db_service.read.assert_called_once() async def test_folder_object_update_fails(self): """Test folder update fails.""" @@ -905,12 +902,10 @@ async def test_user_update_passes_and_returns_id(self): patch = [{"op": "add", "path": "/name", "value": "test2"}] operator = UserOperator(self.client) operator.db_service.exists.return_value = futurized(True) - operator.db_service.read.return_value = futurized(self.test_user) operator.db_service.patch.return_value = futurized(True) user = await operator.update_user(self.test_user, patch) operator.db_service.exists.assert_called_once() operator.db_service.patch.assert_called_once() - self.assertEqual(len(operator.db_service.read.mock_calls), 1) self.assertEqual(user["userId"], self.user_generated_id) async def test_user_update_fails_with_bad_patch(self): @@ -918,11 +913,9 @@ async def test_user_update_fails_with_bad_patch(self): patch = [{"op": "replace", "path": "/nothing"}] operator = UserOperator(self.client) operator.db_service.exists.return_value = futurized(True) - operator.db_service.read.return_value = futurized(self.test_user) with self.assertRaises(HTTPBadRequest): await operator.update_user(self.test_user, patch) operator.db_service.exists.assert_called_once() - operator.db_service.read.assert_called_once() async def test_update_user_fails(self): """Test user update fails.""" From b4a29992f4c4545fc4d40b511063021e768063e7 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Thu, 11 Mar 2021 23:28:01 +0200 Subject: [PATCH 30/47] switch to python 3.8 also rename aggregate to do_aggregate in DBService --- .github/workflows/docs.yml | 2 +- .github/workflows/int.yml | 2 +- .github/workflows/production.yml | 2 +- .github/workflows/style.yml | 2 +- .github/workflows/unit.yml | 8 +- Dockerfile | 6 +- Dockerfile-dev | 2 +- README.md | 2 +- docs/submitter.rst | 2 +- metadata_backend/api/operators.py | 4 +- metadata_backend/database/db_service.py | 2 +- setup.py | 2 +- tests/test_auth.py | 22 +-- tests/test_db_service.py | 81 ++++------ tests/test_handlers.py | 53 +++---- tests/test_health.py | 3 +- tests/test_operators.py | 203 +++++++++++++----------- tox.ini | 4 +- 18 files changed, 193 insertions(+), 209 deletions(-) diff --git a/.github/workflows/docs.yml b/.github/workflows/docs.yml index 55edbcc06..f7999a430 100644 --- a/.github/workflows/docs.yml +++ b/.github/workflows/docs.yml @@ -22,7 +22,7 @@ jobs: max-parallel: 4 matrix: os: [ubuntu-latest] - python-version: [3.7] + python-version: [3.8] runs-on: ${{ matrix.os }} diff --git a/.github/workflows/int.yml b/.github/workflows/int.yml index cf77020b9..b4b72feec 100644 --- a/.github/workflows/int.yml +++ b/.github/workflows/int.yml @@ -8,7 +8,7 @@ jobs: max-parallel: 4 matrix: os: [ubuntu-latest] - python-version: [3.7] + python-version: [3.8] runs-on: ${{ matrix.os }} diff --git a/.github/workflows/production.yml b/.github/workflows/production.yml index 7c9f77186..7f1b7023e 100644 --- a/.github/workflows/production.yml +++ b/.github/workflows/production.yml @@ -10,7 +10,7 @@ jobs: max-parallel: 4 matrix: os: [ubuntu-latest] - python-version: [3.7] + python-version: [3.8] runs-on: ${{ matrix.os }} diff --git a/.github/workflows/style.yml b/.github/workflows/style.yml index b6a943ad0..ef83af0ff 100644 --- a/.github/workflows/style.yml +++ b/.github/workflows/style.yml @@ -8,7 +8,7 @@ jobs: max-parallel: 4 matrix: os: [ubuntu-latest] - python-version: [3.7] + python-version: [3.8] runs-on: ${{ matrix.os }} diff --git a/.github/workflows/unit.yml b/.github/workflows/unit.yml index b8dcb4852..a1dd9597d 100644 --- a/.github/workflows/unit.yml +++ b/.github/workflows/unit.yml @@ -8,7 +8,7 @@ jobs: max-parallel: 4 matrix: os: [ubuntu-latest] - python-version: [3.7] + python-version: [3.8] runs-on: ${{ matrix.os }} @@ -26,8 +26,8 @@ jobs: run: | python -m pip install --upgrade pip pip install tox tox-gh-actions - - name: Run unit tests for python 3.7 - if: ${{ matrix.python-version == '3.7' }} + - name: Run unit tests for python 3.8 + if: ${{ matrix.python-version == '3.8' }} env: COVERALLS_REPO_TOKEN: ${{ secrets.COVERALLS_REPO_TOKEN }} - run: tox -e py37 + run: tox -e py38 diff --git a/Dockerfile b/Dockerfile index bac3be5c4..5396e1f36 100644 --- a/Dockerfile +++ b/Dockerfile @@ -12,7 +12,7 @@ RUN npm install -g npm@7.6.0 \ && npm install --production \ && npm run build --production -FROM python:3.7-alpine3.13 as BUILD-BACKEND +FROM python:3.8-alpine3.13 as BUILD-BACKEND RUN apk add --update \ && apk add --no-cache build-base curl-dev linux-headers bash git musl-dev libffi-dev \ @@ -30,7 +30,7 @@ RUN pip install --upgrade pip && \ pip install -r /root/submitter/requirements.txt && \ pip install /root/submitter -FROM python:3.7-alpine3.13 +FROM python:3.8-alpine3.13 RUN apk add --no-cache --update bash @@ -38,7 +38,7 @@ LABEL maintainer="CSC Developers" LABEL org.label-schema.schema-version="1.0" LABEL org.label-schema.vcs-url="https://github.com/CSCfi/metadata-submitter" -COPY --from=BUILD-BACKEND /usr/local/lib/python3.7/ /usr/local/lib/python3.7/ +COPY --from=BUILD-BACKEND /usr/local/lib/python3.8/ /usr/local/lib/python3.8/ COPY --from=BUILD-BACKEND /usr/local/bin/gunicorn /usr/local/bin/ diff --git a/Dockerfile-dev b/Dockerfile-dev index fc681a894..f28c2657a 100644 --- a/Dockerfile-dev +++ b/Dockerfile-dev @@ -1,4 +1,4 @@ -FROM python:3.7-slim +FROM python:3.8-slim RUN apt-get install ca-certificates diff --git a/README.md b/README.md index d97437d26..cf48d4444 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ Service also validates submitted metadata objects against EGA XSD metadata model ## Install and run Requirements: -- Python 3.7+ +- Python 3.8+ - MongoDB - Docker + docker-compose diff --git a/docs/submitter.rst b/docs/submitter.rst index e5c95a86f..403454bd9 100644 --- a/docs/submitter.rst +++ b/docs/submitter.rst @@ -5,7 +5,7 @@ Metadata Submitter Backend .. note:: Requirements: - - Python 3.7+ + - Python 3.8+ - MongoDB diff --git a/metadata_backend/api/operators.py b/metadata_backend/api/operators.py index d0b311e42..f2675130c 100644 --- a/metadata_backend/api/operators.py +++ b/metadata_backend/api/operators.py @@ -355,7 +355,7 @@ async def query_metadata_database( {"$project": {"_id": 0}}, ] try: - result_aggregate = await self.db_service.aggregate(schema_type, aggregate_query) + result_aggregate = await self.db_service.do_aggregate(schema_type, aggregate_query) except (ConnectionFailure, OperationFailure) as error: reason = f"Error happened while getting object: {error}" LOG.error(reason) @@ -369,7 +369,7 @@ async def query_metadata_database( page_size = len(data) if len(data) != page_size else page_size count_query = [{"$match": mongo_query}, redacted_content, {"$count": "total"}] - total_objects = await self.db_service.aggregate(schema_type, count_query) + total_objects = await self.db_service.do_aggregate(schema_type, count_query) LOG.debug(f"DB query: {que}") LOG.info( diff --git a/metadata_backend/database/db_service.py b/metadata_backend/database/db_service.py index d2da06920..1d1761811 100644 --- a/metadata_backend/database/db_service.py +++ b/metadata_backend/database/db_service.py @@ -270,7 +270,7 @@ async def get_count(self, collection: str, query: Dict) -> int: return await self.database[collection].count_documents(query) @auto_reconnect - async def aggregate(self, collection: str, query: List) -> List: + async def do_aggregate(self, collection: str, query: List) -> List: """Peform aggregate query. :param collection: Collection where document should be searched from diff --git a/setup.py b/setup.py index 1a90b3d1f..709058175 100644 --- a/setup.py +++ b/setup.py @@ -29,7 +29,7 @@ packages=find_packages(exclude=["tests"]), install_requires=requirements, extras_require={ - "test": ["aiounittest", "coverage", "coveralls", "pytest", "pytest-cov", "tox"], + "test": ["coverage", "coveralls", "pytest", "pytest-cov", "tox"], "docs": ["sphinx >= 1.4", "sphinx_rtd_theme"], }, package_data={ diff --git a/tests/test_auth.py b/tests/test_auth.py index 459224a41..4262efff6 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -15,7 +15,7 @@ jwt_data_claim_miss, jwt_data_bad_nonce, ) -from aiounittest import AsyncTestCase, futurized +from unittest import IsolatedAsyncioTestCase import json @@ -85,7 +85,7 @@ async def test_logout_works(self): self.assertEqual(self.client.app["Cookies"], set()) -class AccessHandlerPassTestCase(AsyncTestCase): +class AccessHandlerPassTestCase(IsolatedAsyncioTestCase): """API AccessHandler auth class functions.""" def setUp(self): @@ -114,8 +114,9 @@ def tearDown(self): async def test_get_jwk_fail(self): """Test retrieving JWK exception.""" - with self.assertRaises(HTTPUnauthorized): - await self.AccessHandler._get_key() + with patch("aiohttp.ClientSession.get", side_effect=HTTPUnauthorized): + with self.assertRaises(HTTPUnauthorized): + await self.AccessHandler._get_key() async def test_jwk_key(self): """Test get jwk key.""" @@ -137,8 +138,9 @@ async def test_set_user_fail(self): request = Mock_Request() tk = ("something",) session_id = "session_id" - with self.assertRaises(HTTPBadRequest): - await self.AccessHandler._set_user(request, session_id, tk) + with patch("aiohttp.ClientSession.get", side_effect=HTTPUnauthorized): + with self.assertRaises(HTTPBadRequest): + await self.AccessHandler._set_user(request, session_id, tk) async def test_set_user(self): """Test set user success.""" @@ -157,7 +159,7 @@ async def test_set_user(self): resp = MockResponse(data, 200) with patch("aiohttp.ClientSession.get", return_value=resp): - with patch("metadata_backend.api.operators.UserOperator.create_user", return_value=futurized(new_user_id)): + with patch("metadata_backend.api.operators.UserOperator.create_user", return_value=new_user_id): await self.AccessHandler._set_user(request, session_id, tk) self.assertIn("user_info", request.app["Session"][session_id]) @@ -195,7 +197,7 @@ async def test_callback_pass(self): with patch("aiohttp.ClientSession.post", return_value=resp_token): with patch("aiohttp.ClientSession.get", return_value=resp_jwk): - with patch("metadata_backend.api.auth.AccessHandler._set_user", return_value=futurized(None)): + with patch("metadata_backend.api.auth.AccessHandler._set_user", return_value=None): await self.AccessHandler.callback(request) async def test_callback_missing_claim(self): @@ -212,7 +214,7 @@ async def test_callback_missing_claim(self): with patch("aiohttp.ClientSession.post", return_value=resp_token): with patch("aiohttp.ClientSession.get", return_value=resp_jwk): - with patch("metadata_backend.api.auth.AccessHandler._set_user", return_value=futurized(None)): + with patch("metadata_backend.api.auth.AccessHandler._set_user", return_value=None): with self.assertRaises(HTTPUnauthorized): await self.AccessHandler.callback(request) @@ -230,6 +232,6 @@ async def test_callback_bad_claim(self): with patch("aiohttp.ClientSession.post", return_value=resp_token): with patch("aiohttp.ClientSession.get", return_value=resp_jwk): - with patch("metadata_backend.api.auth.AccessHandler._set_user", return_value=futurized(None)): + with patch("metadata_backend.api.auth.AccessHandler._set_user", return_value=None): with self.assertRaises(HTTPForbidden): await self.AccessHandler.callback(request) diff --git a/tests/test_db_service.py b/tests/test_db_service.py index 686d8e5d2..98518ec16 100644 --- a/tests/test_db_service.py +++ b/tests/test_db_service.py @@ -1,7 +1,7 @@ """Test db_services.""" -from unittest.mock import MagicMock, patch +from unittest.mock import MagicMock, patch, AsyncMock -from aiounittest import AsyncTestCase, futurized +from unittest import IsolatedAsyncioTestCase from bson import ObjectId from motor.motor_asyncio import AsyncIOMotorCursor from pymongo.errors import AutoReconnect, ConnectionFailure @@ -11,26 +11,7 @@ from pymongo import UpdateOne -class AsyncIterator: - """Async iterator based on range.""" - - def __init__(self, seq): - """Init iterator with sequence.""" - self.iter = iter(seq) - - def __aiter__(self): - """Return async iterator.""" - return self - - async def __anext__(self): - """Get next element in sequence.""" - try: - return next(self.iter) - except StopIteration: - raise StopAsyncIteration - - -class DatabaseTestCase(AsyncTestCase): +class DatabaseTestCase(IsolatedAsyncioTestCase): """Test different database operations.""" def setUp(self): @@ -42,14 +23,9 @@ def setUp(self): Monkey patching can probably be removed when upgrading requirements to python 3.8+ since Mock 4.0+ library has async version of MagicMock. """ - # setup async patch - async def async_patch(): - pass - - MagicMock.__await__ = lambda x: async_patch().__await__() self.client = MagicMock() self.database = MagicMock() - self.collection = MagicMock() + self.collection = AsyncMock() self.client.__getitem__.return_value = self.database self.database.__getitem__.return_value = self.collection self.test_service = DBService("testdb", self.client) @@ -82,57 +58,58 @@ def test_db_services_share_mongodb_client(self): async def test_create_inserts_data(self): """Test that create method works and returns success.""" - self.collection.insert_one.return_value = futurized(InsertOneResult(ObjectId("0123456789ab0123456789ab"), True)) + self.collection.insert_one.return_value = InsertOneResult(ObjectId("0123456789ab0123456789ab"), True) success = await self.test_service.create("testcollection", self.data_stub) self.collection.insert_one.assert_called_once_with(self.data_stub) self.assertTrue(success) async def test_create_reports_fail_correctly(self): """Test that failure is reported, when write not acknowledged.""" - self.collection.insert_one.return_value = futurized(InsertOneResult(None, False)) + self.collection.insert_one.return_value = InsertOneResult(None, False) success = await self.test_service.create("testcollection", self.data_stub) self.collection.insert_one.assert_called_once_with(self.data_stub) self.assertFalse(success) async def test_read_returns_data(self): """Test that read method works and returns data.""" - self.collection.find_one.return_value = futurized(self.data_stub) + self.collection.find_one.return_value = self.data_stub found_doc = await self.test_service.read("testcollection", self.id_stub) self.assertEqual(found_doc, self.data_stub) self.collection.find_one.assert_called_once_with({"accessionId": self.id_stub}, {"_id": False}) async def test_exists_returns_false(self): """Test that exists method works and returns false.""" + self.collection.find_one.return_value = None found_doc = await self.test_service.exists("testcollection", self.id_stub) self.assertEqual(found_doc, False) self.collection.find_one.assert_called_once_with({"accessionId": self.id_stub}, {"_id": False}) async def test_exists_returns_true(self): """Test that exists method works and returns True.""" - self.collection.find_one.return_value = futurized(self.data_stub) + self.collection.find_one.return_value = self.data_stub found_doc = await self.test_service.exists("testcollection", self.id_stub) self.assertEqual(found_doc, True) self.collection.find_one.assert_called_once_with({"accessionId": self.id_stub}, {"_id": False}) async def test_update_updates_data(self): """Test that update method works and returns success.""" - self.collection.find_one.return_value = futurized(self.data_stub) - self.collection.update_one.return_value = futurized(UpdateResult({}, True)) + self.collection.find_one.return_value = self.data_stub + self.collection.update_one.return_value = UpdateResult({}, True) success = await self.test_service.update("testcollection", self.id_stub, self.data_stub) self.collection.update_one.assert_called_once_with({"accessionId": self.id_stub}, {"$set": self.data_stub}) self.assertTrue(success) async def test_replace_replaces_data(self): """Test that replace method works and returns success.""" - self.collection.find_one.return_value = futurized(self.data_stub) - self.collection.replace_one.return_value = futurized(UpdateResult({}, True)) + self.collection.find_one.return_value = self.data_stub + self.collection.replace_one.return_value = UpdateResult({}, True) success = await self.test_service.replace("testcollection", self.id_stub, self.data_stub) self.collection.replace_one.assert_called_once_with({"accessionId": self.id_stub}, self.data_stub) self.assertTrue(success) async def test_delete_deletes_data(self): """Test that delete method works and returns success.""" - self.collection.delete_one.return_value = futurized(DeleteResult({"n": 1}, True)) + self.collection.delete_one.return_value = DeleteResult({"n": 1}, True) success = await self.test_service.delete("testcollection", self.id_stub) self.collection.delete_one.assert_called_once_with({"accessionId": self.id_stub}) self.assertTrue(success) @@ -140,13 +117,12 @@ async def test_delete_deletes_data(self): def test_query_executes_find(self): """Test that find is executed, so cursor is returned.""" self.collection.find.return_value = AsyncIOMotorCursor(None, None) - cursor = self.test_service.query("testcollection", {}) - self.assertEqual(type(cursor), AsyncIOMotorCursor) + self.test_service.query("testcollection", {}) self.collection.find.assert_called_once_with({}, {"_id": False}) async def test_count_returns_amount(self): """Test that get_count method works and returns amount.""" - self.collection.count_documents.return_value = futurized(100) + self.collection.count_documents.return_value = 100 count = await self.test_service.get_count("testcollection", {}) self.collection.count_documents.assert_called_once_with({}) self.assertEqual(count, 100) @@ -161,20 +137,21 @@ async def test_db_operation_is_retried_with_increasing_interval(self): async def test_create_folder_inserts_folder(self): """Test that create method works for folder and returns success.""" - self.collection.insert_one.return_value = futurized(InsertOneResult(ObjectId("0000000000aa1111111111bb"), True)) + self.collection.insert_one.return_value = InsertOneResult(ObjectId("0000000000aa1111111111bb"), True) folder = await self.test_service.create("folder", self.folder_stub) self.collection.insert_one.assert_called_once_with(self.folder_stub) self.assertTrue(folder) async def test_read_folder_returns_data(self): """Test that read method works for folder and returns folder.""" - self.collection.find_one.return_value = futurized(self.folder_stub) + self.collection.find_one.return_value = self.folder_stub found_folder = await self.test_service.read("folder", self.f_id_stub) self.collection.find_one.assert_called_once_with({"folderId": self.f_id_stub}, {"_id": False}) self.assertEqual(found_folder, self.folder_stub) async def test_externalId_exists_returns_false(self): """Test that externalId exists method works and returns None.""" + self.collection.find_one.return_value = None found_doc = await self.test_service.exists_user_by_externalId("test_user@eppn.fi", "name") self.assertEqual(found_doc, None) self.collection.find_one.assert_called_once_with( @@ -183,23 +160,23 @@ async def test_externalId_exists_returns_false(self): async def test_externalId_exists_returns_true(self): """Test that externalId exists method works and returns user id.""" - self.collection.find_one.return_value = futurized(self.user_stub) + self.collection.find_one.return_value = self.user_stub found_doc = await self.test_service.exists_user_by_externalId("test_user@eppn.fi", "name") self.assertEqual(found_doc, self.user_id_stub) self.collection.find_one.assert_called_once_with( {"externalId": "test_user@eppn.fi", "name": "name"}, {"_id": False, "externalId": False} ) - async def test_aggregate_performed(self): - """Test that aggregate is executed, so cursor is returned.""" - self.collection.aggregate.return_value = AsyncIterator(range(5)) - cursor = await self.test_service.aggregate("testcollection", []) - self.assertEqual(type(cursor), list) - self.collection.aggregate.assert_called_once_with([]) + # async def test_aggregate_performed(self): + # """Test that aggregate is executed, so cursor is returned.""" + # self.collection.aggregate.return_value = AsyncIterator(range(5)) + # cursor = await self.test_service.do_aggregate("testcollection", []) + # self.assertEqual(type(cursor), list) + # self.collection.aggregate.assert_called_once_with([]) async def test_append_data(self): """Test that append method works and returns data.""" - self.collection.find_one_and_update.return_value = futurized(self.data_stub) + self.collection.find_one_and_update.return_value = self.data_stub success = await self.test_service.append("testcollection", self.id_stub, self.data_stub) self.collection.find_one_and_update.assert_called_once_with( {"accessionId": self.id_stub}, @@ -211,7 +188,7 @@ async def test_append_data(self): async def test_remove_data(self): """Test that remove method works and returns data.""" - self.collection.find_one_and_update.return_value = futurized({}) + self.collection.find_one_and_update.return_value = {} success = await self.test_service.remove("testcollection", self.id_stub, self.data_stub) self.collection.find_one_and_update.assert_called_once_with( {"accessionId": self.id_stub}, @@ -226,7 +203,7 @@ async def test_patch_data(self): json_patch = [ {"op": "add", "path": "/metadataObjects/-", "value": {"accessionId": self.id_stub, "schema": "study"}}, ] - self.collection.bulk_write.return_value = futurized(UpdateResult({}, True)) + self.collection.bulk_write.return_value = UpdateResult({}, True) success = await self.test_service.patch("testcollection", self.id_stub, json_patch) self.collection.bulk_write.assert_called_once_with( [ diff --git a/tests/test_handlers.py b/tests/test_handlers.py index 009169c42..cfe59c4aa 100644 --- a/tests/test_handlers.py +++ b/tests/test_handlers.py @@ -5,7 +5,6 @@ from aiohttp import FormData from aiohttp.test_utils import AioHTTPTestCase, unittest_run_loop -from aiounittest import futurized from metadata_backend.api.middlewares import generate_cookie from .mockups import get_request_with_fernet @@ -131,74 +130,72 @@ def create_submission_data(self, files): async def fake_operator_read_metadata_object(self, schema_type, accession_id): """Fake read operation to return mocked JSON.""" - return await futurized((self.metadata_json, "application/json")) + return (self.metadata_json, "application/json") async def fake_operator_query_metadata_object(self, schema_type, query, page_num, page_size, filtered_list): """Fake query operation to return list containing mocked JSON.""" - return await futurized( - ([self.metadata_json], self.page_num, self.page_size, self.total_objects), - ) + return ([self.metadata_json], self.page_num, self.page_size, self.total_objects) async def fake_xmloperator_read_metadata_object(self, schema_type, accession_id): """Fake read operation to return mocked xml.""" - return await futurized((self.metadata_xml, "text/xml")) + return (self.metadata_xml, "text/xml") async def fake_xmloperator_create_metadata_object(self, schema_type, content): """Fake create operation to return mocked accessionId.""" - return await futurized(self.test_ega_string) + return self.test_ega_string async def fake_xmloperator_replace_metadata_object(self, schema_type, accession_id, content): """Fake replace operation to return mocked accessionId.""" - return await futurized(self.test_ega_string) + return self.test_ega_string async def fake_operator_create_metadata_object(self, schema_type, content): """Fake create operation to return mocked accessionId.""" - return await futurized(self.test_ega_string) + return self.test_ega_string async def fake_operator_update_metadata_object(self, schema_type, accession_id, content): """Fake update operation to return mocked accessionId.""" - return await futurized(self.test_ega_string) + return self.test_ega_string async def fake_operator_replace_metadata_object(self, schema_type, accession_id, content): """Fake replace operation to return mocked accessionId.""" - return await futurized(self.test_ega_string) + return self.test_ega_string async def fake_operator_delete_metadata_object(self, schema_type, accession_id): """Fake delete operation to await successful operation indicator.""" - return await futurized(True) + return True async def fake_folderoperator_create_folder(self, content): """Fake create operation to return mocked folderId.""" - return await futurized(self.folder_id) + return self.folder_id async def fake_folderoperator_read_folder(self, folder_id): """Fake read operation to return mocked folder.""" - return await futurized(self.test_folder) + return self.test_folder async def fake_folderoperator_delete_folder(self, folder_id): """Fake delete folder to await nothing.""" - return await futurized(None) + return None async def fake_folderoperator_check_object(self, schema_type, accession_id): """Fake check object in folder.""" data = True, self.folder_id, False - return await futurized(data) + return data async def fake_folderoperator_get_collection_objects(self, schema_type, accession_id): """Fake get collection of objects in folder.""" - return await futurized(["EDAG3991701442770179", "EGA123456"]) + return ["EDAG3991701442770179", "EGA123456"] async def fake_useroperator_user_has_folder(self, schema_type, user_id, folder_id): """Fake check object in folder.""" - return await futurized(True) + return True async def fake_useroperator_create_user(self, content): """Fake user operation to return mocked userId.""" - return await futurized(self.user_id) + return self.user_id async def fake_useroperator_read_user(self, user_id): """Fake read operation to return mocked user.""" - return await futurized(self.test_user) + return self.test_user @unittest_run_loop async def test_submit_endpoint_submission_does_not_fail(self): @@ -605,7 +602,7 @@ async def test_folder_creation_with_empty_body_fails(self): @unittest_run_loop async def test_get_folders_with_1_folder(self): """Test get_folders() endpoint returns list with 1 folder.""" - self.MockedFolderOperator().query_folders.return_value = futurized(self.test_folder) + self.MockedFolderOperator().query_folders.return_value = self.test_folder response = await self.client.get("/folders") self.MockedFolderOperator().query_folders.assert_called_once() self.assertEqual(response.status, 200) @@ -614,7 +611,7 @@ async def test_get_folders_with_1_folder(self): @unittest_run_loop async def test_get_folders_with_no_folders(self): """Test get_folders() endpoint returns empty list.""" - self.MockedFolderOperator().query_folders.return_value = futurized([]) + self.MockedFolderOperator().query_folders.return_value = [] response = await self.client.get("/folders") self.MockedFolderOperator().query_folders.assert_called_once() self.assertEqual(response.status, 200) @@ -642,7 +639,7 @@ async def test_update_folder_fails_with_wrong_key(self): @unittest_run_loop async def test_update_folder_passes(self): """Test that folder would update with correct keys.""" - self.MockedFolderOperator().update_folder.return_value = futurized(self.folder_id) + self.MockedFolderOperator().update_folder.return_value = self.folder_id data = [{"op": "replace", "path": "/name", "value": "test2"}] response = await self.client.patch("/folders/FOL12345678", json=data) self.MockedFolderOperator().update_folder.assert_called_once() @@ -653,7 +650,7 @@ async def test_update_folder_passes(self): @unittest_run_loop async def test_folder_is_published(self): """Test that folder would be published.""" - self.MockedFolderOperator().update_folder.return_value = futurized(self.folder_id) + self.MockedFolderOperator().update_folder.return_value = self.folder_id response = await self.client.patch("/publish/FOL12345678") self.MockedFolderOperator().update_folder.assert_called_once() self.assertEqual(response.status, 200) @@ -663,7 +660,7 @@ async def test_folder_is_published(self): @unittest_run_loop async def test_folder_deletion_is_called(self): """Test that folder would be deleted.""" - self.MockedFolderOperator().read_folder.return_value = futurized(self.test_folder) + self.MockedFolderOperator().read_folder.return_value = self.test_folder response = await self.client.delete("/folders/FOL12345678") self.MockedFolderOperator().read_folder.assert_called_once() self.MockedFolderOperator().delete_folder.assert_called_once() @@ -681,8 +678,8 @@ async def test_get_user_works(self): @unittest_run_loop async def test_user_deletion_is_called(self): """Test that user object would be deleted.""" - self.MockedUserOperator().read_user.return_value = futurized(self.test_user) - self.MockedUserOperator().delete_user.return_value = futurized(None) + self.MockedUserOperator().read_user.return_value = self.test_user + self.MockedUserOperator().delete_user.return_value = None await self.client.delete("/users/current") self.MockedUserOperator().read_user.assert_called_once() self.MockedUserOperator().delete_user.assert_called_once() @@ -700,7 +697,7 @@ async def test_update_user_fails_with_wrong_key(self): @unittest_run_loop async def test_update_user_passes(self): """Test that user object would update with correct keys.""" - self.MockedUserOperator().update_user.return_value = futurized(self.user_id) + self.MockedUserOperator().update_user.return_value = self.user_id data = [{"op": "add", "path": "/drafts/-", "value": "test_value"}] response = await self.client.patch("/users/current", json=data) self.MockedUserOperator().update_user.assert_called_once() diff --git a/tests/test_health.py b/tests/test_health.py index a92231f3a..db20cbdcc 100644 --- a/tests/test_health.py +++ b/tests/test_health.py @@ -3,7 +3,6 @@ from unittest.mock import patch from aiohttp.test_utils import AioHTTPTestCase, unittest_run_loop -from aiounittest import futurized from metadata_backend.server import init @@ -31,7 +30,7 @@ async def tearDownAsync(self): async def fake_asynciomotorclient_server_info(self): """Fake server info method for a motor client.""" - return await futurized(True) + return True @unittest_run_loop async def test_health_check_is_down(self): diff --git a/tests/test_operators.py b/tests/test_operators.py index 0ae87c16d..4b9e88bc5 100644 --- a/tests/test_operators.py +++ b/tests/test_operators.py @@ -6,13 +6,11 @@ from unittest.mock import MagicMock, patch, call from aiohttp.web import HTTPBadRequest, HTTPNotFound, HTTPUnprocessableEntity -from aiounittest import AsyncTestCase, futurized -from aiounittest.mock import AsyncMockIterator +from unittest import IsolatedAsyncioTestCase + from multidict import MultiDict, MultiDictProxy from pymongo.errors import ConnectionFailure -from .test_db_service import AsyncIterator - from metadata_backend.api.operators import ( FolderOperator, Operator, @@ -21,7 +19,26 @@ ) -class MockCursor(AsyncMockIterator): +class AsyncIterator: + """Async iterator based on range.""" + + def __init__(self, seq): + """Init iterator with sequence.""" + self.iter = iter(seq) + + def __aiter__(self): + """Return async iterator.""" + return self + + async def __anext__(self): + """Get next element in sequence.""" + try: + return next(self.iter) + except StopIteration: + raise StopAsyncIteration + + +class MockCursor(AsyncIterator): """Mock implementation of pymongo cursor. Takes iterable async mock and adds some pymongo cursor methods. @@ -47,7 +64,7 @@ def limit(self, count): return self -class TestOperators(AsyncTestCase): +class TestOperators(IsolatedAsyncioTestCase): """Test db-operator classes.""" def setUp(self): @@ -56,11 +73,6 @@ def setUp(self): Monkey patches MagicMock to work with async / await, then sets up other patches and mocks for tests. """ - # setup async patch - async def async_patch(): - pass - - MagicMock.__await__ = lambda x: async_patch().__await__() self.client = MagicMock() self.accession_id = uuid4().hex self.folder_id = uuid4().hex @@ -117,7 +129,7 @@ async def test_reading_metadata_works(self): "accessionId": "EGA123456", "foo": "bar", } - operator.db_service.read.return_value = futurized(data) + operator.db_service.read.return_value = data read_data, c_type = await operator.read_metadata_object("sample", "EGA123456") operator.db_service.read.assert_called_once_with("sample", "EGA123456") self.assertEqual(c_type, "application/json") @@ -135,7 +147,7 @@ async def test_reading_metadata_works_with_xml(self): """Test XML is read from db correctly.""" operator = XMLOperator(self.client) data = {"accessionId": "EGA123456", "content": ""} - operator.db_service.read.return_value = futurized(data) + operator.db_service.read.return_value = data r_data, c_type = await operator.read_metadata_object("sample", "EGA123456") operator.db_service.read.assert_called_once_with("sample", "EGA123456") self.assertEqual(c_type, "text/xml") @@ -144,7 +156,7 @@ async def test_reading_metadata_works_with_xml(self): async def test_reading_with_non_valid_id_raises_error(self): """Test read metadata HTTPNotFound is raised.""" operator = Operator(self.client) - operator.db_service.read.return_value = futurized(False) + operator.db_service.read.return_value = False with self.assertRaises(HTTPNotFound): await operator.read_metadata_object("study", "EGA123456") @@ -178,7 +190,7 @@ async def test_json_create_passes_and_returns_accessionId(self): "alias": "GSE10966", "descriptor": {"studyTitle": "Highly", "studyType": "Other"}, } - operator.db_service.create.return_value = futurized(True) + operator.db_service.create.return_value = True accession = await operator.create_metadata_object("study", data) operator.db_service.create.assert_called_once() self.assertEqual(accession, self.accession_id) @@ -191,8 +203,8 @@ async def test_json_replace_passes_and_returns_accessionId(self): "descriptor": {"studyTitle": "Highly", "studyType": "Other"}, } operator = Operator(self.client) - operator.db_service.exists.return_value = futurized(True) - operator.db_service.replace.return_value = futurized(True) + operator.db_service.exists.return_value = True + operator.db_service.replace.return_value = True accession = await operator.replace_metadata_object("study", self.accession_id, data) operator.db_service.replace.assert_called_once() self.assertEqual(accession, self.accession_id) @@ -200,8 +212,8 @@ async def test_json_replace_passes_and_returns_accessionId(self): async def test_json_replace_raises_if_not_exists(self): """Test replace method raises error.""" operator = Operator(self.client) - operator.db_service.exists.return_value = futurized(False) - operator.db_service.replace.return_value = futurized(True) + operator.db_service.exists.return_value = False + operator.db_service.replace.return_value = True with self.assertRaises(HTTPNotFound): await operator.replace_metadata_object("study", self.accession_id, {}) operator.db_service.replace.assert_called_once() @@ -209,8 +221,7 @@ async def test_json_replace_raises_if_not_exists(self): async def test_db_error_replace_raises_400_error(self): """Test replace metadata HTTPBadRequest is raised.""" operator = Operator(self.client) - operator.db_service.exists.return_value = futurized(True) - operator.db_service.read.side_effect = ConnectionFailure + operator.db_service.exists.side_effect = ConnectionFailure with self.assertRaises(HTTPBadRequest): await operator.replace_metadata_object("study", self.accession_id, {}) @@ -223,9 +234,9 @@ async def test_json_update_passes_and_returns_accessionId(self): "descriptor": {"studyTitle": "Highly", "studyType": "Other"}, } operator = Operator(self.client) - operator.db_service.read.return_value = futurized(db_data) - operator.db_service.exists.return_value = futurized(True) - operator.db_service.update.return_value = futurized(True) + operator.db_service.read.return_value = db_data + operator.db_service.exists.return_value = True + operator.db_service.update.return_value = True accession = await operator.update_metadata_object("study", self.accession_id, data) operator.db_service.update.assert_called_once() self.assertEqual(accession, self.accession_id) @@ -233,8 +244,8 @@ async def test_json_update_passes_and_returns_accessionId(self): async def test_json_update_raises_if_not_exists(self): """Test update method raises error.""" operator = Operator(self.client) - operator.db_service.exists.return_value = futurized(False) - operator.db_service.replace.return_value = futurized(True) + operator.db_service.exists.return_value = False + operator.db_service.replace.return_value = True with self.assertRaises(HTTPNotFound): await operator.update_metadata_object("study", self.accession_id, {}) operator.db_service.update.assert_called_once() @@ -242,8 +253,7 @@ async def test_json_update_raises_if_not_exists(self): async def test_db_error_update_raises_400_error(self): """Test update metadata HTTPBadRequest is raised.""" operator = Operator(self.client) - operator.db_service.exists.return_value = futurized(True) - operator.db_service.read.side_effect = ConnectionFailure + operator.db_service.exists.side_effect = ConnectionFailure with self.assertRaises(HTTPBadRequest): await operator.update_metadata_object("study", self.accession_id, {}) @@ -251,10 +261,10 @@ async def test_xml_create_passes_and_returns_accessionId(self): """Test create method for XML works. Patch JSON related calls.""" operator = XMLOperator(self.client) operator.db_service.db_client = self.client - operator.db_service.create.return_value = futurized(True) + operator.db_service.create.return_value = True with patch( ("metadata_backend.api.operators.Operator._format_data_to_create_and_add_to_db"), - return_value=futurized(self.accession_id), + return_value=self.accession_id, ): with patch("metadata_backend.api.operators.XMLToJSONParser"): accession = await operator.create_metadata_object("study", "") @@ -266,7 +276,7 @@ async def test_correct_data_is_set_to_json_when_creating(self): operator = Operator(self.client) with patch( ("metadata_backend.api.operators.Operator._insert_formatted_object_to_db"), - return_value=futurized(self.accession_id), + return_value=self.accession_id, ) as mocked_insert: with patch("metadata_backend.api.operators.datetime") as m_date: m_date.utcnow.return_value = datetime.datetime(2020, 4, 14) @@ -285,9 +295,7 @@ async def test_correct_data_is_set_to_json_when_creating(self): async def test_wront_data_is_set_to_json_when_replacing(self): """Test operator replace catches error.""" operator = Operator(self.client) - with patch( - "metadata_backend.api.operators.Operator._replace_object_from_db", return_value=futurized(self.accession_id) - ): + with patch("metadata_backend.api.operators.Operator._replace_object_from_db", return_value=self.accession_id): with patch("metadata_backend.api.operators.datetime") as m_date: m_date.utcnow.return_value = datetime.datetime(2020, 4, 14) with self.assertRaises(HTTPBadRequest): @@ -308,7 +316,7 @@ async def test_correct_data_is_set_to_json_when_replacing(self): """Test operator replaces object and adds necessary info.""" operator = Operator(self.client) with patch( - "metadata_backend.api.operators.Operator._replace_object_from_db", return_value=futurized(self.accession_id) + "metadata_backend.api.operators.Operator._replace_object_from_db", return_value=self.accession_id ) as mocked_insert: with patch("metadata_backend.api.operators.datetime") as m_date: m_date.utcnow.return_value = datetime.datetime(2020, 4, 14) @@ -325,7 +333,7 @@ async def test_correct_data_is_set_to_json_when_updating(self): operator = Operator(self.client) with patch( ("metadata_backend.api.operators.Operator._update_object_from_db"), - return_value=futurized(self.accession_id), + return_value=self.accession_id, ) as mocked_insert: with patch("metadata_backend.api.operators.datetime") as m_date: m_date.utcnow.return_value = datetime.datetime(2020, 4, 14) @@ -342,7 +350,7 @@ async def test_wrong_data_is_set_to_json_when_updating(self): operator = Operator(self.client) with patch( ("metadata_backend.api.operators.Operator._update_object_from_db"), - return_value=futurized(self.accession_id), + return_value=self.accession_id, ): with patch("metadata_backend.api.operators.datetime") as m_date: m_date.utcnow.return_value = datetime.datetime(2020, 4, 14) @@ -367,11 +375,11 @@ async def test_correct_data_is_set_to_xml_when_creating(self): xml_data = "" with patch( ("metadata_backend.api.operators.Operator._format_data_to_create_and_add_to_db"), - return_value=futurized(self.accession_id), + return_value=self.accession_id, ): with patch( ("metadata_backend.api.operators.XMLOperator._insert_formatted_object_to_db"), - return_value=futurized(self.accession_id), + return_value=self.accession_id, ) as m_insert: with patch("metadata_backend.api.operators.XMLToJSONParser"): acc = await (operator._format_data_to_create_and_add_to_db("study", xml_data)) @@ -387,11 +395,11 @@ async def test_correct_data_is_set_to_xml_when_replacing(self): xml_data = "" with patch( "metadata_backend.api.operators.Operator._format_data_to_replace_and_add_to_db", - return_value=futurized(self.accession_id), + return_value=self.accession_id, ): with patch( "metadata_backend.api.operators.XMLOperator._replace_object_from_db", - return_value=futurized(self.accession_id), + return_value=self.accession_id, ) as m_insert: with patch("metadata_backend.api.operators.XMLToJSONParser"): acc = await (operator._format_data_to_replace_and_add_to_db("study", self.accession_id, xml_data)) @@ -406,8 +414,8 @@ async def test_deleting_metadata_deletes_json_and_xml(self): """Test metadata is deleted.""" operator = Operator(self.client) operator.db_service.db_client = self.client - operator.db_service.exists.return_value = futurized(True) - operator.db_service.delete.return_value = futurized(True) + operator.db_service.exists.return_value = True + operator.db_service.delete.return_value = True await operator.delete_metadata_object("sample", "EGA123456") self.assertEqual(operator.db_service.delete.call_count, 2) operator.db_service.delete.assert_called_with("sample", "EGA123456") @@ -416,8 +424,8 @@ async def test_deleting_metadata_delete_raises(self): """Test error raised with delete.""" operator = Operator(self.client) operator.db_service.db_client = self.client - operator.db_service.exists.return_value = futurized(False) - operator.db_service.delete.return_value = futurized(True) + operator.db_service.exists.return_value = False + operator.db_service.delete.return_value = True with self.assertRaises(HTTPNotFound): await operator.delete_metadata_object("sample", "EGA123456") self.assertEqual(operator.db_service.delete.call_count, 2) @@ -427,8 +435,8 @@ async def test_deleting_metadata_delete_raises_bad_request(self): """Test bad request error raised with delete.""" operator = Operator(self.client) operator.db_service.db_client = self.client - operator.db_service.exists.return_value = futurized(True) - operator.db_service.delete.return_value = futurized(False) + operator.db_service.exists.return_value = True + operator.db_service.delete.return_value = False with self.assertRaises(HTTPBadRequest): await operator.delete_metadata_object("sample", "EGA123456") self.assertEqual(operator.db_service.delete.call_count, 2) @@ -446,7 +454,7 @@ async def test_working_query_params_are_passed_to_db_query(self): } ] study_total = [{"total": 0}] - operator.db_service.aggregate.side_effect = [futurized(study_test), futurized(study_total)] + operator.db_service.do_aggregate.side_effect = [study_test, study_total] query = MultiDictProxy(MultiDict([("studyAttributes", "foo")])) await operator.query_metadata_database("study", query, 1, 10, []) calls = [ @@ -483,7 +491,7 @@ async def test_working_query_params_are_passed_to_db_query(self): ], ), ] - operator.db_service.aggregate.assert_has_calls(calls, any_order=True) + operator.db_service.do_aggregate.assert_has_calls(calls, any_order=True) async def test_non_working_query_params_are_not_passed_to_db_query(self): """Test that database with empty query, when url params are wrong.""" @@ -497,11 +505,11 @@ async def test_non_working_query_params_are_not_passed_to_db_query(self): } ] study_total = [{"total": 0}] - operator.db_service.aggregate.side_effect = [futurized(study_test), futurized(study_total)] + operator.db_service.do_aggregate.side_effect = [study_test, study_total] query = MultiDictProxy(MultiDict([("swag", "littinen")])) with patch( "metadata_backend.api.operators.Operator._format_read_data", - return_value=futurized(study_test), + return_value=study_test, ): await operator.query_metadata_database("study", query, 1, 10, []) calls = [ @@ -524,8 +532,8 @@ async def test_non_working_query_params_are_not_passed_to_db_query(self): ], ), ] - operator.db_service.aggregate.assert_has_calls(calls, any_order=True) - self.assertEqual(operator.db_service.aggregate.call_count, 2) + operator.db_service.do_aggregate.assert_has_calls(calls, any_order=True) + self.assertEqual(operator.db_service.do_aggregate.call_count, 2) async def test_query_result_is_parsed_correctly(self): """Test JSON is read and correct pagination values are returned.""" @@ -545,7 +553,7 @@ async def test_query_result_is_parsed_correctly(self): }, ] study_total = [{"total": 100}] - operator.db_service.aggregate.side_effect = [futurized(multiple_result), futurized(study_total)] + operator.db_service.do_aggregate.side_effect = [multiple_result, study_total] query = MultiDictProxy(MultiDict([])) ( parsed, @@ -568,7 +576,7 @@ async def test_non_empty_query_result_raises_notfound(self): query = MultiDictProxy(MultiDict([])) with patch( "metadata_backend.api.operators.Operator._format_read_data", - return_value=futurized([]), + return_value=[], ): with self.assertRaises(HTTPNotFound): await operator.query_metadata_database("study", query, 1, 10, []) @@ -577,11 +585,11 @@ async def test_query_skip_and_limit_are_set_correctly(self): """Test custom skip and limits.""" operator = Operator(self.client) data = {"foo": "bar"} - result = futurized([]) - operator.db_service.aggregate.side_effect = [result, futurized([{"total": 0}])] + result = [] + operator.db_service.do_aggregate.side_effect = [result, [{"total": 0}]] with patch( "metadata_backend.api.operators.Operator._format_read_data", - return_value=futurized(data), + return_value=data, ): await operator.query_metadata_database("sample", {}, 3, 50, []) calls = [ @@ -604,14 +612,14 @@ async def test_query_skip_and_limit_are_set_correctly(self): ], ), ] - operator.db_service.aggregate.assert_has_calls(calls, any_order=True) - self.assertEqual(operator.db_service.aggregate.call_count, 2) + operator.db_service.do_aggregate.assert_has_calls(calls, any_order=True) + self.assertEqual(operator.db_service.do_aggregate.call_count, 2) async def test_create_folder_works_and_returns_folderId(self): """Test create method for folders work.""" operator = FolderOperator(self.client) data = {"name": "Mock folder", "description": "test mock folder"} - operator.db_service.create.return_value = futurized(True) + operator.db_service.create.return_value = True folder = await operator.create_folder(data) operator.db_service.create.assert_called_once() self.assertEqual(folder, self.folder_id) @@ -628,7 +636,7 @@ async def test_create_folder_db_create_fails(self): """Test create method for folders db create fails.""" operator = FolderOperator(self.client) data = {"name": "Mock folder", "description": "test mock folder"} - operator.db_service.create.return_value = futurized(False) + operator.db_service.create.return_value = False with self.assertRaises(HTTPBadRequest): await operator.create_folder(data) @@ -715,7 +723,7 @@ async def test_get_objects_folder_no_data(self): async def test_reading_folder_works(self): """Test folder is read from db correctly.""" operator = FolderOperator(self.client) - operator.db_service.read.return_value = futurized(self.test_folder) + operator.db_service.read.return_value = self.test_folder read_data = await operator.read_folder(self.folder_id) operator.db_service.read.assert_called_once_with("folder", self.folder_id) self.assertEqual(read_data, self.test_folder) @@ -731,7 +739,7 @@ async def test_folder_update_passes_and_returns_id(self): """Test update method for folders works.""" patch = [{"op": "add", "path": "/name", "value": "test2"}] operator = FolderOperator(self.client) - operator.db_service.patch.return_value = futurized(True) + operator.db_service.patch.return_value = True folder = await operator.update_folder(self.test_folder, patch) operator.db_service.patch.assert_called_once() self.assertEqual(folder["folderId"], self.folder_id) @@ -740,21 +748,21 @@ async def test_folder_update_fails_with_bad_patch(self): """Test folder update raises error with improper JSON Patch.""" patch = [{"op": "replace", "path": "/nothing"}] operator = FolderOperator(self.client) - operator.db_service.read.return_value = futurized(self.test_folder) + operator.db_service.patch.return_value = False with self.assertRaises(HTTPBadRequest): await operator.update_folder(self.test_folder, patch) async def test_folder_object_update_fails(self): """Test folder update fails.""" operator = FolderOperator(self.client) - operator.db_service.update.side_effect = ConnectionFailure + operator.db_service.patch.side_effect = ConnectionFailure with self.assertRaises(HTTPBadRequest): await operator.update_folder(self.test_folder, []) async def test_folder_object_remove_passes(self): """Test remove object method for folders works.""" operator = FolderOperator(self.client) - operator.db_service.remove.return_value = futurized(self.test_folder) + operator.db_service.remove.return_value = self.test_folder await operator.remove_object(self.test_folder, "study", self.accession_id) operator.db_service.remove.assert_called_once() self.assertEqual(len(operator.db_service.remove.mock_calls), 1) @@ -769,24 +777,24 @@ async def test_folder_object_remove_fails(self): async def test_check_folder_exists_fails(self): """Test fails exists fails.""" operator = FolderOperator(self.client) - operator.db_service.exists.return_value = futurized(False) + operator.db_service.exists.return_value = False with self.assertRaises(HTTPNotFound): await operator.check_folder_exists(self.folder_id) operator.db_service.exists.assert_called_once() async def test_deleting_folder_passes(self): - """Test folder is deleted correctly.""" + """Test folder is deleted correctly, if not published.""" operator = FolderOperator(self.client) - operator.db_service.exists.return_value = futurized(True) - operator.db_service.delete.return_value = futurized(True) + operator.db_service.published_folder.return_value = False + operator.db_service.delete.return_value = True await operator.delete_folder(self.folder_id) operator.db_service.delete.assert_called_with("folder", self.folder_id) async def test_deleting_folder_fails_on_delete(self): - """Test folder fails on db delete.""" + """Test folder fails on db delete, if not published.""" operator = FolderOperator(self.client) - operator.db_service.exists.return_value = futurized(True) - operator.db_service.delete.return_value = futurized(False) + operator.db_service.published_folder.return_value = False + operator.db_service.delete.return_value = False with self.assertRaises(HTTPBadRequest): await operator.delete_folder(self.folder_id) operator.db_service.delete.assert_called_with("folder", self.folder_id) @@ -794,7 +802,7 @@ async def test_deleting_folder_fails_on_delete(self): async def test_delete_folder_fails(self): """Test folder delete fails.""" operator = FolderOperator(self.client) - operator.db_service.exists.side_effect = ConnectionFailure + operator.db_service.published_folder.side_effect = ConnectionFailure with self.assertRaises(HTTPBadRequest): await operator.delete_folder(self.folder_id) @@ -802,8 +810,8 @@ async def test_create_user_works_and_returns_userId(self): """Test create method for users work.""" operator = UserOperator(self.client) data = "externalId", "name" - operator.db_service.exists_user_by_externalId.return_value = futurized(None) - operator.db_service.create.return_value = futurized(True) + operator.db_service.exists_user_by_externalId.return_value = None + operator.db_service.create.return_value = True user = await operator.create_user(data) operator.db_service.create.assert_called_once() self.assertEqual(user, self.user_generated_id) @@ -812,8 +820,8 @@ async def test_create_user_on_create_fails(self): """Test create method fails on db create.""" operator = UserOperator(self.client) data = "externalId", "name" - operator.db_service.exists_user_by_externalId.return_value = futurized(None) - operator.db_service.create.return_value = futurized(False) + operator.db_service.exists_user_by_externalId.return_value = None + operator.db_service.create.return_value = False with self.assertRaises(HTTPBadRequest): await operator.create_user(data) operator.db_service.create.assert_called_once() @@ -859,7 +867,7 @@ async def test_create_user_works_existing_userId(self): """Test create method for existing user.""" operator = UserOperator(self.client) data = "eppn", "name" - operator.db_service.exists_user_by_externalId.return_value = futurized(self.user_generated_id) + operator.db_service.exists_user_by_externalId.return_value = self.user_generated_id user = await operator.create_user(data) operator.db_service.create.assert_not_called() self.assertEqual(user, self.user_generated_id) @@ -875,8 +883,8 @@ async def test_create_user_fails(self): async def test_reading_user_works(self): """Test user object is read from db correctly.""" operator = UserOperator(self.client) - operator.db_service.exists.return_value = futurized(True) - operator.db_service.read.return_value = futurized(self.test_user) + operator.db_service.exists.return_value = True + operator.db_service.read.return_value = self.test_user read_data = await operator.read_user(self.user_id) operator.db_service.exists.assert_called_once() operator.db_service.read.assert_called_once_with("user", self.user_id) @@ -892,7 +900,7 @@ async def test_read_user_fails(self): async def test_check_user_exists_fails(self): """Test user exists fails.""" operator = UserOperator(self.client) - operator.db_service.exists.return_value = futurized(False) + operator.db_service.exists.return_value = False with self.assertRaises(HTTPNotFound): await operator._check_user_exists(self.user_id) operator.db_service.exists.assert_called_once() @@ -901,8 +909,8 @@ async def test_user_update_passes_and_returns_id(self): """Test update method for users works.""" patch = [{"op": "add", "path": "/name", "value": "test2"}] operator = UserOperator(self.client) - operator.db_service.exists.return_value = futurized(True) - operator.db_service.patch.return_value = futurized(True) + operator.db_service.exists.return_value = True + operator.db_service.patch.return_value = True user = await operator.update_user(self.test_user, patch) operator.db_service.exists.assert_called_once() operator.db_service.patch.assert_called_once() @@ -912,7 +920,8 @@ async def test_user_update_fails_with_bad_patch(self): """Test user update raises error with improper JSON Patch.""" patch = [{"op": "replace", "path": "/nothing"}] operator = UserOperator(self.client) - operator.db_service.exists.return_value = futurized(True) + operator.db_service.exists.return_value = True + operator.db_service.patch.return_value = False with self.assertRaises(HTTPBadRequest): await operator.update_user(self.test_user, patch) operator.db_service.exists.assert_called_once() @@ -927,16 +936,16 @@ async def test_update_user_fails(self): async def test_deleting_user_passes(self): """Test user is deleted correctly.""" operator = UserOperator(self.client) - operator.db_service.exists.return_value = futurized(True) - operator.db_service.delete.return_value = futurized(True) + operator.db_service.exists.return_value = True + operator.db_service.delete.return_value = True await operator.delete_user(self.user_id) operator.db_service.delete.assert_called_with("user", "current") async def test_deleting_user_fails_on_delete(self): """Test user fails on delete operation.""" operator = UserOperator(self.client) - operator.db_service.exists.return_value = futurized(True) - operator.db_service.delete.return_value = futurized(False) + operator.db_service.exists.return_value = True + operator.db_service.delete.return_value = False with self.assertRaises(HTTPBadRequest): await operator.delete_user(self.user_id) operator.db_service.delete.assert_called_with("user", "current") @@ -951,8 +960,8 @@ async def test_deleting_user_fails(self): async def test_user_objects_remove_passes(self): """Test remove objects method for users works.""" operator = UserOperator(self.client) - operator.db_service.exists.return_value = futurized(True) - operator.db_service.remove.return_value = futurized(self.test_user) + operator.db_service.exists.return_value = True + operator.db_service.remove.return_value = self.test_user await operator.remove_objects(self.user_generated_id, "study", ["id"]) operator.db_service.exists.assert_called_once() operator.db_service.remove.assert_called_once() @@ -961,7 +970,7 @@ async def test_user_objects_remove_passes(self): async def test_user_objects_remove_fails(self): """Test remove objects method for users fails.""" operator = UserOperator(self.client) - operator.db_service.exists.return_value = futurized(True) + operator.db_service.exists.return_value = True operator.db_service.remove.side_effect = ConnectionFailure with self.assertRaises(HTTPBadRequest): await operator.remove_objects(self.user_generated_id, "study", ["id"]) @@ -969,8 +978,8 @@ async def test_user_objects_remove_fails(self): async def test_user_objects_append_passes(self): """Test append objects method for users works.""" operator = UserOperator(self.client) - operator.db_service.exists.return_value = futurized(True) - operator.db_service.append.return_value = futurized(self.test_user) + operator.db_service.exists.return_value = True + operator.db_service.append.return_value = self.test_user await operator.assign_objects(self.user_generated_id, "study", []) operator.db_service.exists.assert_called_once() operator.db_service.append.assert_called_once() @@ -979,8 +988,8 @@ async def test_user_objects_append_passes(self): async def test_user_objects_append_on_result_fails(self): """Test append objects method for users fails on db response validation.""" operator = UserOperator(self.client) - operator.db_service.exists.return_value = futurized(True) - operator.db_service.append.return_value = futurized(False) + operator.db_service.exists.return_value = True + operator.db_service.append.return_value = False with self.assertRaises(HTTPBadRequest): await operator.assign_objects(self.user_generated_id, "study", []) operator.db_service.exists.assert_called_once() diff --git a/tox.ini b/tox.ini index 98b995762..5d67ea0fd 100644 --- a/tox.ini +++ b/tox.ini @@ -1,5 +1,5 @@ [tox] -envlist = py37, flake8, mypy, docs, black +envlist = py38, flake8, mypy, docs, black skipsdist = True [flake8] @@ -51,4 +51,4 @@ commands = py.test -x --cov=metadata_backend tests/ [gh-actions] python = - 3.7: flake8, py37, docs, mypy, black + 3.8: flake8, py38, docs, mypy, black From 0511f21ed9176ae8db8f5f8ced392b76735b8574 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Fri, 12 Mar 2021 00:24:22 +0200 Subject: [PATCH 31/47] test config and publish folder db query --- metadata_backend/conf/conf.py | 63 +++++++++++++++++++---------------- tests/test_conf.py | 59 ++++++++++++++++++++++++++++++++ tests/test_db_service.py | 9 +++++ 3 files changed, 102 insertions(+), 29 deletions(-) create mode 100644 tests/test_conf.py diff --git a/metadata_backend/conf/conf.py b/metadata_backend/conf/conf.py index 3ff21c015..ebe4e0b35 100644 --- a/metadata_backend/conf/conf.py +++ b/metadata_backend/conf/conf.py @@ -35,6 +35,7 @@ import os from pathlib import Path from distutils.util import strtobool +from typing import Tuple from motor.motor_asyncio import AsyncIOMotorClient @@ -51,37 +52,41 @@ # If both MONGO_DATABASE and MONGO_AUTHDB are unspecified, # the client will attempt to authenticate the specified user to the admin database. -mongo_user = os.getenv("MONGO_USERNAME", "admin") -mongo_password = os.getenv("MONGO_PASSWORD", "admin") -mongo_host = os.getenv("MONGO_HOST", "localhost:27017") -mongo_database = os.getenv("MONGO_DATABASE", "") -_base = f"mongodb://{mongo_user}:{mongo_password}@{mongo_host}/{mongo_database}" -if strtobool(os.getenv("MONGO_SSL", "False")): - _ca = os.getenv("MONGO_SSL_CA", None) - _key = os.getenv("MONGO_SSL_CLIENT_KEY", None) - _cert = os.getenv("MONGO_SSL_CLIENT_CERT", None) - tls = f"?tls=true&tlsCAFile={_ca}&ssl_keyfile={_key}&ssl_certfile={_cert}" - url = f"{_base}{tls}" -elif strtobool(os.getenv("MONGO_SSL", "False")) and bool(os.getenv("MONGO_AUTHDB")): - _ca = os.getenv("MONGO_SSL_CA", None) - _key = os.getenv("MONGO_SSL_CLIENT_KEY", None) - _cert = os.getenv("MONGO_SSL_CLIENT_CERT", None) - if _ca and _key and _cert: - tls = f"?tls=true&tlsCAFile={_ca}&ssl_keyfile={_key}&ssl_certfile={_cert}" + +def set_conf() -> Tuple[str, str]: + """Set config based on env vars.""" + mongo_user = os.getenv("MONGO_USERNAME", "admin") + mongo_password = os.getenv("MONGO_PASSWORD", "admin") + mongo_host = os.getenv("MONGO_HOST", "localhost:27017") + mongo_database = os.getenv("MONGO_DATABASE", "") + _base = f"mongodb://{mongo_user}:{mongo_password}@{mongo_host}/{mongo_database}" + if strtobool(os.getenv("MONGO_SSL", "False")): + _ca = os.getenv("MONGO_SSL_CA", None) + _key = os.getenv("MONGO_SSL_CLIENT_KEY", None) + _cert = os.getenv("MONGO_SSL_CLIENT_CERT", None) + if bool(os.getenv("MONGO_AUTHDB")) and _ca and _key and _cert: + tls = f"?tls=true&tlsCAFile={_ca}&ssl_keyfile={_key}&ssl_certfile={_cert}" + _authdb = str(os.getenv("MONGO_AUTHDB")) + auth = f"&authSource={_authdb}" + url = f"{_base}{tls}{auth}" + else: + tls = f"?tls=true&tlsCAFile={_ca}&ssl_keyfile={_key}&ssl_certfile={_cert}" + url = f"{_base}{tls}" + elif bool(os.getenv("MONGO_AUTHDB")): _authdb = str(os.getenv("MONGO_AUTHDB")) - auth = f"&authSource={_authdb}" - url = f"{_base}{tls}{auth}" + auth = f"?authSource={_authdb}" + url = f"{_base}{auth}" else: - LOG.error("Missing CA, key or certificate.") -elif bool(os.getenv("MONGO_AUTHDB")): - _authdb = str(os.getenv("MONGO_AUTHDB")) - auth = f"?authSource={_authdb}" - url = f"{_base}{auth}" -else: - url = _base - -if os.getenv("MONGO_DATABASE", "") == "": - mongo_database = "default" + url = _base + + if os.getenv("MONGO_DATABASE", "") == "": + mongo_database = "default" + + return url, mongo_database + + +url, mongo_database = set_conf() + LOG.debug(f"mongodb connection string is {url}") diff --git a/tests/test_conf.py b/tests/test_conf.py new file mode 100644 index 000000000..e55be8938 --- /dev/null +++ b/tests/test_conf.py @@ -0,0 +1,59 @@ +"""Test configuration.""" + +import os +from unittest import TestCase, mock + +from metadata_backend.conf.conf import set_conf + + +class ConfigTests(TestCase): + """Test configuration.""" + + @mock.patch.dict(os.environ, {"MONGO_DATABASE": "ceva"}) + @mock.patch.dict(os.environ, {"MONGO_SSL": "True"}) + @mock.patch.dict(os.environ, {"MONGO_SSL_CA": "ca.crt"}) + @mock.patch.dict(os.environ, {"MONGO_SSL_CLIENT_KEY": "client.key"}) + @mock.patch.dict(os.environ, {"MONGO_SSL_CLIENT_CERT": "client.crt"}) + @mock.patch.dict(os.environ, {"MONGO_AUTHDB": "admin"}) + def test_all_values(self): + """Test all env values set.""" + url, mongo_database = set_conf() + self.assertEqual( + mongo_database, + "ceva", + ) + _params = "ceva?tls=true&tlsCAFile=ca.crt&ssl_keyfile=client.key&ssl_certfile=client.crt&authSource=admin" + self.assertEqual( + url, + f"mongodb://admin:admin@localhost:27017/{_params}", + ) + + @mock.patch.dict(os.environ, {"MONGO_DATABASE": "ceva"}) + @mock.patch.dict(os.environ, {"MONGO_SSL": "False"}) + @mock.patch.dict(os.environ, {"MONGO_AUTHDB": "admin"}) + def test_no_ssl_value(self): + """Test no ssl env values set.""" + url, mongo_database = set_conf() + self.assertEqual( + mongo_database, + "ceva", + ) + self.assertEqual( + url, + "mongodb://admin:admin@localhost:27017/ceva?authSource=admin", + ) + + @mock.patch.dict(os.environ, {"MONGO_DATABASE": "ceva"}) + @mock.patch.dict(os.environ, {"MONGO_DATABASE": "ceva"}) + @mock.patch.dict(os.environ, {"MONGO_SSL": "True"}) + @mock.patch.dict(os.environ, {"MONGO_SSL_CA": "ca.crt"}) + @mock.patch.dict(os.environ, {"MONGO_SSL_CLIENT_KEY": "client.key"}) + @mock.patch.dict(os.environ, {"MONGO_SSL_CLIENT_CERT": "client.crt"}) + def test_ssl_no_db_value(self): + """Test ssl env values set, but no db.""" + url, _ = set_conf() + _params = "ceva?tls=true&tlsCAFile=ca.crt&ssl_keyfile=client.key&ssl_certfile=client.crt" + self.assertEqual( + url, + f"mongodb://admin:admin@localhost:27017/{_params}", + ) diff --git a/tests/test_db_service.py b/tests/test_db_service.py index 98518ec16..42d7bdd97 100644 --- a/tests/test_db_service.py +++ b/tests/test_db_service.py @@ -149,6 +149,15 @@ async def test_read_folder_returns_data(self): self.collection.find_one.assert_called_once_with({"folderId": self.f_id_stub}, {"_id": False}) self.assertEqual(found_folder, self.folder_stub) + async def test_published_folder_returns_data(self): + """Test that published folder checks if folder is published.""" + self.collection.find_one.return_value = self.folder_stub + found_folder = await self.test_service.published_folder(self.f_id_stub) + self.collection.find_one.assert_called_once_with( + {"published": True, "folderId": self.f_id_stub}, {"_id": False} + ) + self.assertEqual(found_folder, True) + async def test_externalId_exists_returns_false(self): """Test that externalId exists method works and returns None.""" self.collection.find_one.return_value = None From 6709c216e46b43f417f63ca6d85e6fd06e0c364d Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Fri, 12 Mar 2021 08:33:14 +0200 Subject: [PATCH 32/47] fix aggregate test --- tests/test_db_service.py | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/tests/test_db_service.py b/tests/test_db_service.py index 42d7bdd97..538ff95ca 100644 --- a/tests/test_db_service.py +++ b/tests/test_db_service.py @@ -9,6 +9,7 @@ from metadata_backend.database.db_service import DBService from pymongo import UpdateOne +from .test_operators import AsyncIterator class DatabaseTestCase(IsolatedAsyncioTestCase): @@ -176,12 +177,19 @@ async def test_externalId_exists_returns_true(self): {"externalId": "test_user@eppn.fi", "name": "name"}, {"_id": False, "externalId": False} ) - # async def test_aggregate_performed(self): - # """Test that aggregate is executed, so cursor is returned.""" - # self.collection.aggregate.return_value = AsyncIterator(range(5)) - # cursor = await self.test_service.do_aggregate("testcollection", []) - # self.assertEqual(type(cursor), list) - # self.collection.aggregate.assert_called_once_with([]) + async def test_aggregate_performed(self): + """Test that aggregate is executed, so cursor is returned.""" + # this does not like the coroutine that AsyncMock returns + _client = MagicMock() + _database = MagicMock() + _collection = MagicMock() + _client.__getitem__.return_value = _database + _database.__getitem__.return_value = _collection + _test_service = DBService("testdb", _client) + _collection.aggregate.return_value = AsyncIterator(range(5)) + cursor = await _test_service.do_aggregate("testcollection", []) + self.assertEqual(type(cursor), list) + _collection.aggregate.assert_called_once_with([]) async def test_append_data(self): """Test that append method works and returns data.""" From 8ccd0df9a91817717ba07f6b0bf04cd5496b64b3 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Fri, 12 Mar 2021 09:43:36 +0200 Subject: [PATCH 33/47] split RestAPIHandler as it became too large Split into Object, User, Folder API Handlers to make it easier to identify operations. rename RestApiHandler to RestAPIHandler --- metadata_backend/api/handlers.py | 148 +++++++++++++++++-------------- metadata_backend/server.py | 82 +++++++++-------- 2 files changed, 126 insertions(+), 104 deletions(-) diff --git a/metadata_backend/api/handlers.py b/metadata_backend/api/handlers.py index 2eab91a3d..9aa26f3ac 100644 --- a/metadata_backend/api/handlers.py +++ b/metadata_backend/api/handlers.py @@ -26,7 +26,7 @@ from ..conf.conf import aai_config -class RESTApiHandler: +class RESTAPIHandler: """Handler for REST API methods.""" def _check_schema_exists(self, schema_type: str) -> None: @@ -40,26 +40,6 @@ def _check_schema_exists(self, schema_type: str) -> None: LOG.error(reason) raise web.HTTPNotFound(reason=reason) - def _header_links(self, url: str, page: int, size: int, total_objects: int) -> CIMultiDict[str]: - """Create link header for pagination. - - :param url: base url for request - :param page: current page - :param size: results per page - :param total_objects: total objects to compute the total pages - :returns: JSON with query results - """ - total_pages = ceil(total_objects / size) - prev_link = f'<{url}?page={page-1}&per_page={size}>; rel="prev", ' if page > 1 else "" - next_link = f'<{url}?page={page+1}&per_page={size}>; rel="next", ' if page < total_pages else "" - last_link = f'<{url}?page={total_pages}&per_page={size}>; rel="last"' if page < total_pages else "" - comma = ", " if page > 1 and page < total_pages else "" - first_link = f'<{url}?page=1&per_page={size}>; rel="first"{comma}' if page > 1 else "" - links = f"{prev_link}{next_link}{first_link}{last_link}" - link_headers = CIMultiDict(Link=f"{links}") - LOG.debug("Link headers created") - return link_headers - async def _handle_check_ownedby_user(self, req: Request, collection: str, accession_id: str) -> bool: """Check if object belongs to user. @@ -148,6 +128,77 @@ async def _filter_by_user(self, req: Request, collection: str, seq: List) -> Asy if await self._handle_check_ownedby_user(req, collection, el["accessionId"]): yield el + async def _get_data(self, req: Request) -> Dict: + """Get the data content from a request. + + :param req: POST/PUT/PATCH request + :raises: HTTPBadRequest if request does not have proper JSON data + :returns: JSON content of the request + """ + try: + content = await req.json() + return content + except json.decoder.JSONDecodeError as e: + reason = "JSON is not correctly formatted." f" See: {e}" + LOG.error(reason) + raise web.HTTPBadRequest(reason=reason) + + async def get_schema_types(self, req: Request) -> Response: + """Get all possible metadata schema types from database. + + Basically returns which objects user can submit and query for. + :param req: GET Request + :returns: JSON list of schema types + """ + types_json = json.dumps([x["description"] for x in schema_types.values()]) + LOG.info(f"GET schema types. Retrieved {len(schema_types)} schemas.") + return web.Response(body=types_json, status=200, content_type="application/json") + + async def get_json_schema(self, req: Request) -> Response: + """Get all JSON Schema for a specific schema type. + + Basically returns which objects user can submit and query for. + :param req: GET Request + :raises: HTTPBadRequest if request does not find the schema + :returns: JSON list of schema types + """ + schema_type = req.match_info["schema"] + self._check_schema_exists(schema_type) + + try: + schema = JSONSchemaLoader().get_schema(schema_type) + LOG.info(f"{schema_type} schema loaded.") + return web.Response(body=json.dumps(schema), status=200, content_type="application/json") + + except SchemaNotFoundException as error: + reason = f"{error} ({schema_type})" + LOG.error(reason) + raise web.HTTPBadRequest(reason=reason) + + +class ObjectAPIHandler(RESTAPIHandler): + """API Handler for Objects.""" + + def _header_links(self, url: str, page: int, size: int, total_objects: int) -> CIMultiDict[str]: + """Create link header for pagination. + + :param url: base url for request + :param page: current page + :param size: results per page + :param total_objects: total objects to compute the total pages + :returns: JSON with query results + """ + total_pages = ceil(total_objects / size) + prev_link = f'<{url}?page={page-1}&per_page={size}>; rel="prev", ' if page > 1 else "" + next_link = f'<{url}?page={page+1}&per_page={size}>; rel="next", ' if page < total_pages else "" + last_link = f'<{url}?page={total_pages}&per_page={size}>; rel="last"' if page < total_pages else "" + comma = ", " if page > 1 and page < total_pages else "" + first_link = f'<{url}?page=1&per_page={size}>; rel="first"{comma}' if page > 1 else "" + links = f"{prev_link}{next_link}{first_link}{last_link}" + link_headers = CIMultiDict(Link=f"{links}") + LOG.debug("Link headers created") + return link_headers + async def _handle_query(self, req: Request) -> Response: """Handle query results. @@ -205,53 +256,6 @@ def get_page_param(param_name: str, default: int) -> int: content_type="application/json", ) - async def _get_data(self, req: Request) -> Dict: - """Get the data content from a request. - - :param req: POST/PUT/PATCH request - :raises: HTTPBadRequest if request does not have proper JSON data - :returns: JSON content of the request - """ - try: - content = await req.json() - return content - except json.decoder.JSONDecodeError as e: - reason = "JSON is not correctly formatted." f" See: {e}" - LOG.error(reason) - raise web.HTTPBadRequest(reason=reason) - - async def get_schema_types(self, req: Request) -> Response: - """Get all possible metadata schema types from database. - - Basically returns which objects user can submit and query for. - :param req: GET Request - :returns: JSON list of schema types - """ - types_json = json.dumps([x["description"] for x in schema_types.values()]) - LOG.info(f"GET schema types. Retrieved {len(schema_types)} schemas.") - return web.Response(body=types_json, status=200, content_type="application/json") - - async def get_json_schema(self, req: Request) -> Response: - """Get all JSON Schema for a specific schema type. - - Basically returns which objects user can submit and query for. - :param req: GET Request - :raises: HTTPBadRequest if request does not find the schema - :returns: JSON list of schema types - """ - schema_type = req.match_info["schema"] - self._check_schema_exists(schema_type) - - try: - schema = JSONSchemaLoader().get_schema(schema_type) - LOG.info(f"{schema_type} schema loaded.") - return web.Response(body=json.dumps(schema), status=200, content_type="application/json") - - except SchemaNotFoundException as error: - reason = f"{error} ({schema_type})" - LOG.error(reason) - raise web.HTTPBadRequest(reason=reason) - async def get_object(self, req: Request) -> Response: """Get one metadata object by its accession id. @@ -462,6 +466,10 @@ async def patch_object(self, req: Request) -> Response: LOG.info(f"PATCH object with accession ID {accession_id} in schema {collection} was successful.") return web.Response(body=body, status=200, content_type="application/json") + +class FolderAPIHandler(RESTAPIHandler): + """API Handler for folders.""" + async def get_folders(self, req: Request) -> Response: """Get all possible object folders from database. @@ -655,6 +663,10 @@ async def delete_folder(self, req: Request) -> Response: LOG.info(f"DELETE folder with ID {_folder_id} was successful.") return web.Response(status=204) + +class UserAPIHandler(RESTAPIHandler): + """API Handler for users.""" + async def get_user(self, req: Request) -> Response: """Get one user by its user ID. diff --git a/metadata_backend/server.py b/metadata_backend/server.py index 8066ef63c..49de9e973 100644 --- a/metadata_backend/server.py +++ b/metadata_backend/server.py @@ -8,7 +8,14 @@ import secrets import time -from .api.handlers import RESTApiHandler, StaticHandler, SubmissionAPIHandler +from .api.handlers import ( + RESTAPIHandler, + StaticHandler, + SubmissionAPIHandler, + FolderAPIHandler, + UserAPIHandler, + ObjectAPIHandler, +) from .api.auth import AccessHandler from .api.middlewares import http_error_handler, check_login from .api.health import HealthHandler @@ -57,55 +64,58 @@ async def init() -> web.Application: server.middlewares.append(http_error_handler) server.middlewares.append(check_login) - rest_handler = RESTApiHandler() - submission_handler = SubmissionAPIHandler() + _handler = RESTAPIHandler() + _object = ObjectAPIHandler() + _folder = FolderAPIHandler() + _user = UserAPIHandler() + _submission = SubmissionAPIHandler() api_routes = [ - web.get("/schemas", rest_handler.get_schema_types), - web.get("/schemas/{schema}", rest_handler.get_json_schema), - web.get("/objects/{schema}/{accessionId}", rest_handler.get_object), - web.delete("/objects/{schema}/{accessionId}", rest_handler.delete_object), - web.get("/objects/{schema}", rest_handler.query_objects), - web.post("/objects/{schema}", rest_handler.post_object), - web.put("/objects/{schema}/{accessionId}", rest_handler.put_object), - web.get("/drafts/{schema}/{accessionId}", rest_handler.get_object), - web.put("/drafts/{schema}/{accessionId}", rest_handler.put_object), - web.patch("/drafts/{schema}/{accessionId}", rest_handler.patch_object), - web.patch("/objects/{schema}/{accessionId}", rest_handler.patch_object), - web.delete("/drafts/{schema}/{accessionId}", rest_handler.delete_object), - web.post("/drafts/{schema}", rest_handler.post_object), - web.get("/folders", rest_handler.get_folders), - web.post("/folders", rest_handler.post_folder), - web.get("/folders/{folderId}", rest_handler.get_folder), - web.patch("/folders/{folderId}", rest_handler.patch_folder), - web.delete("/folders/{folderId}", rest_handler.delete_folder), - web.patch("/publish/{folderId}", rest_handler.publish_folder), - web.get("/users/{userId}", rest_handler.get_user), - web.patch("/users/{userId}", rest_handler.patch_user), - web.delete("/users/{userId}", rest_handler.delete_user), - web.post("/submit", submission_handler.submit), - web.post("/validate", submission_handler.validate), + web.get("/schemas", _handler.get_schema_types), + web.get("/schemas/{schema}", _handler.get_json_schema), + web.get("/objects/{schema}/{accessionId}", _object.get_object), + web.delete("/objects/{schema}/{accessionId}", _object.delete_object), + web.get("/objects/{schema}", _object.query_objects), + web.post("/objects/{schema}", _object.post_object), + web.put("/objects/{schema}/{accessionId}", _object.put_object), + web.get("/drafts/{schema}/{accessionId}", _object.get_object), + web.put("/drafts/{schema}/{accessionId}", _object.put_object), + web.patch("/drafts/{schema}/{accessionId}", _object.patch_object), + web.patch("/objects/{schema}/{accessionId}", _object.patch_object), + web.delete("/drafts/{schema}/{accessionId}", _object.delete_object), + web.post("/drafts/{schema}", _object.post_object), + web.get("/folders", _folder.get_folders), + web.post("/folders", _folder.post_folder), + web.get("/folders/{folderId}", _folder.get_folder), + web.patch("/folders/{folderId}", _folder.patch_folder), + web.delete("/folders/{folderId}", _folder.delete_folder), + web.patch("/publish/{folderId}", _folder.publish_folder), + web.get("/users/{userId}", _user.get_user), + web.patch("/users/{userId}", _user.patch_user), + web.delete("/users/{userId}", _user.delete_user), + web.post("/submit", _submission.submit), + web.post("/validate", _submission.validate), ] server.router.add_routes(api_routes) LOG.info("Server configurations and routes loaded") - access_handler = AccessHandler(aai_config) + _access = AccessHandler(aai_config) aai_routes = [ - web.get("/aai", access_handler.login), - web.get("/logout", access_handler.logout), - web.get("/callback", access_handler.callback), + web.get("/aai", _access.login), + web.get("/logout", _access.logout), + web.get("/callback", _access.callback), ] server.router.add_routes(aai_routes) LOG.info("AAI routes loaded") - health_handler = HealthHandler() + _health = HealthHandler() health_routes = [ - web.get("/health", health_handler.get_health_status), + web.get("/health", _health.get_health_status), ] server.router.add_routes(health_routes) LOG.info("Health routes loaded") if frontend_static_files.exists(): - static_handler = StaticHandler(frontend_static_files) + _static = StaticHandler(frontend_static_files) frontend_routes = [ - web.static("/static", static_handler.setup_static()), - web.get("/{path:.*}", static_handler.frontend), + web.static("/static", _static.setup_static()), + web.get("/{path:.*}", _static.frontend), ] server.router.add_routes(frontend_routes) LOG.info("Frontend routes loaded") From 9811302e59a0cad2c5bfa208cf304935d8b37bd6 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Fri, 12 Mar 2021 09:51:09 +0200 Subject: [PATCH 34/47] fix typo in docs --- docs/frontend.rst | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/docs/frontend.rst b/docs/frontend.rst index 075070792..8796fa77f 100644 --- a/docs/frontend.rst +++ b/docs/frontend.rst @@ -61,9 +61,8 @@ Form components Form components are crucial part of the application: - All submissions and folder creation are made with `react-hook-form `_. - Latter uses form as a reference so submission can be triggered outside the form. -- Form forschema based forms are created with customschema parser, which builds - ``react-hook-form`` based forms from givenschema. The forms are validated againstschema with ``Ajv``. + Latter uses form as a reference so submission can be triggered outside the form. JSON schema based forms are created with custom JSON schema parser, which builds + ``react-hook-form`` based forms from given schema. The forms are validated against the JSON schema with ``Ajv``. React-hook-form is used for performance reasons: it uses uncontrolled components so adding a lot of fields to array doesn't slow rendering of the application. Redux store From 6095f12c7a133b296d7c252d58599a049436a571 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Fri, 12 Mar 2021 13:14:22 +0200 Subject: [PATCH 35/47] handle patch req validation for folders & users #161 require validation for requests via JSON schema but this is not possible via mongo and easier to implement at request level rather than when inserting data --- metadata_backend/api/handlers.py | 141 ++++++++++++++++++++++++------- tests/test_handlers.py | 2 +- 2 files changed, 110 insertions(+), 33 deletions(-) diff --git a/metadata_backend/api/handlers.py b/metadata_backend/api/handlers.py index 9aa26f3ac..1af475e0f 100644 --- a/metadata_backend/api/handlers.py +++ b/metadata_backend/api/handlers.py @@ -5,7 +5,7 @@ from collections import Counter from math import ceil from pathlib import Path -from typing import Dict, List, Tuple, Union, cast, AsyncGenerator +from typing import Dict, List, Tuple, Union, cast, AsyncGenerator, Any from aiohttp import BodyPartReader, web from aiohttp.web import Request, Response @@ -470,6 +470,59 @@ async def patch_object(self, req: Request) -> Response: class FolderAPIHandler(RESTAPIHandler): """API Handler for folders.""" + def _check_patch_folder(self, patch_ops: Any) -> None: + """Check patch operations in request are valid. + + We check that ``metadataObjects`` and ``drafts`` have ``_required_values``. + For tags we check that the ``submissionType`` takes either ``XML`` or + ``Form`` as values. + :param patch_ops: JSON patch request + :raises: HTTPBadRequest if request does not fullfil one of requirements + :raises: HTTPUnauthorized if request tries to do anything else than add or replace + :returns: None + """ + _required_paths = ["/name", "/description"] + _required_values = ["schema", "accessionId"] + _arrays = ["/metadataObjects/-", "/drafts/-"] + _tags = re.compile("^/(metadataObjects|drafts)/[0-9]*/(tags)$") + + for op in patch_ops: + if _tags.match(op["path"]): + LOG.info(f"{op['op']} on tags in folder") + if "submissionType" in op["value"].keys() and op["value"]["submissionType"] not in ["XML", "Form"]: + reason = "submissionType is restricted to either XML or Form values." + LOG.error(reason) + raise web.HTTPBadRequest(reason=reason) + pass + else: + if all(i not in op["path"] for i in _required_paths + _arrays): + reason = f"Request contains '{op['path']}' key that cannot be updated to folders." + LOG.error(reason) + raise web.HTTPBadRequest(reason=reason) + if op["op"] in ["remove", "copy", "test", "move"]: + reason = f"{op['op']} on {op['path']} is not allowed." + LOG.error(reason) + raise web.HTTPUnauthorized(reason=reason) + if op["op"] == "replace" and op["path"] in _arrays: + reason = f"{op['op']} on {op['path']}; replacing all objects is not allowed." + LOG.error(reason) + raise web.HTTPUnauthorized(reason=reason) + if op["path"] in _arrays: + _ops = op["value"] if isinstance(op["value"], list) else [op["value"]] + for item in _ops: + if not all(key in item.keys() for key in _required_values): + reason = "accessionId and schema are required fields." + LOG.error(reason) + raise web.HTTPBadRequest(reason=reason) + if ( + "tags" in item + and "submissionType" in item["tags"] + and item["tags"]["submissionType"] not in ["XML", "Form"] + ): + reason = "submissionType is restricted to either XML or Form values." + LOG.error(reason) + raise web.HTTPBadRequest(reason=reason) + async def get_folders(self, req: Request) -> Response: """Get all possible object folders from database. @@ -557,26 +610,7 @@ async def patch_folder(self, req: Request) -> Response: # Check patch operations in request are valid patch_ops = await self._get_data(req) - allowed_paths = ["/name", "/description"] - array_paths = ["/metadataObjects/-", "/drafts/-"] - tags_path = re.compile("^/(metadataObjects|drafts)/[0-9]*/(tags)$") - for op in patch_ops: - if tags_path.match(op["path"]): - LOG.info(f"{op['op']} on tags in folder") - pass - else: - if all(i not in op["path"] for i in allowed_paths + array_paths): - reason = f"Request contains '{op['path']}' key that cannot be updated to folders." - LOG.error(reason) - raise web.HTTPBadRequest(reason=reason) - if op["op"] in ["remove", "copy", "test", "move"]: - reason = f"{op['op']} on {op['path']} is not allowed." - LOG.error(reason) - raise web.HTTPUnauthorized(reason=reason) - if op["op"] == "replace" and op["path"] in array_paths: - reason = f"{op['op']} on {op['path']}; replacing all objects is not allowed." - LOG.error(reason) - raise web.HTTPUnauthorized(reason=reason) + self._check_patch_folder(patch_ops) check_user = await self._handle_check_ownedby_user(req, "folders", folder_id) if not check_user: @@ -667,6 +701,59 @@ async def delete_folder(self, req: Request) -> Response: class UserAPIHandler(RESTAPIHandler): """API Handler for users.""" + def _check_patch_user(self, patch_ops: Any) -> None: + """Check patch operations in request are valid. + + We check that ``folders`` have string values (one or a list) + and ``drafts`` have ``_required_values``. + For tags we check that the ``submissionType`` takes either ``XML`` or + ``Form`` as values. + :param patch_ops: JSON patch request + :raises: HTTPBadRequest if request does not fullfil one of requirements + :raises: HTTPUnauthorized if request tries to do anything else than add or replace + :returns: None + """ + _arrays = ["/drafts/-", "/folders/-"] + _required_values = ["schema", "accessionId"] + _tags = re.compile("^/(drafts)/[0-9]*/(tags)$") + for op in patch_ops: + if _tags.match(op["path"]): + LOG.info(f"{op['op']} on tags in folder") + if "submissionType" in op["value"].keys() and op["value"]["submissionType"] not in ["XML", "Form"]: + reason = "submissionType is restricted to either XML or Form values." + LOG.error(reason) + raise web.HTTPBadRequest(reason=reason) + pass + else: + if all(i not in op["path"] for i in _arrays): + reason = f"Request contains '{op['path']}' key that cannot be updated to user object" + LOG.error(reason) + raise web.HTTPBadRequest(reason=reason) + if op["op"] in ["remove", "copy", "test", "move", "replace"]: + reason = f"{op['op']} on {op['path']} is not allowed." + LOG.error(reason) + raise web.HTTPUnauthorized(reason=reason) + if op["path"] == "/folders/-": + if not (isinstance(op["value"], str) or isinstance(op["value"], list)): + reason = "We only accept string folder IDs." + LOG.error(reason) + raise web.HTTPBadRequest(reason=reason) + if op["path"] == "/drafts/-": + _ops = op["value"] if isinstance(op["value"], list) else [op["value"]] + for item in _ops: + if not all(key in item.keys() for key in _required_values): + reason = "accessionId and schema are required fields." + LOG.error(reason) + raise web.HTTPBadRequest(reason=reason) + if ( + "tags" in item + and "submissionType" in item["tags"] + and item["tags"]["submissionType"] not in ["XML", "Form"] + ): + reason = "submissionType is restricted to either XML or Form values." + LOG.error(reason) + raise web.HTTPBadRequest(reason=reason) + async def get_user(self, req: Request) -> Response: """Get one user by its user ID. @@ -700,18 +787,8 @@ async def patch_user(self, req: Request) -> Response: raise web.HTTPUnauthorized(reason="Only current user operations are allowed") db_client = req.app["db_client"] - # Check patch operations in request are valid patch_ops = await self._get_data(req) - allowed_paths = ["/drafts/-", "/folders/-"] - for op in patch_ops: - if all(i not in op["path"] for i in allowed_paths): - reason = f"Request contains '{op['path']}' key that cannot be updated to user object" - LOG.error(reason) - raise web.HTTPBadRequest(reason=reason) - if op["op"] in ["remove", "copy", "test", "move", "replace"]: - reason = f"{op['op']} on {op['path']} is not allowed." - LOG.error(reason) - raise web.HTTPUnauthorized(reason=reason) + self._check_patch_user(patch_ops) operator = UserOperator(db_client) diff --git a/tests/test_handlers.py b/tests/test_handlers.py index cfe59c4aa..fd8ccabca 100644 --- a/tests/test_handlers.py +++ b/tests/test_handlers.py @@ -698,7 +698,7 @@ async def test_update_user_fails_with_wrong_key(self): async def test_update_user_passes(self): """Test that user object would update with correct keys.""" self.MockedUserOperator().update_user.return_value = self.user_id - data = [{"op": "add", "path": "/drafts/-", "value": "test_value"}] + data = [{"op": "add", "path": "/drafts/-", "value": [{"accessionId": "3", "schema": "sample"}]}] response = await self.client.patch("/users/current", json=data) self.MockedUserOperator().update_user.assert_called_once() self.assertEqual(response.status, 200) From de68441d292648450b97a67b2012d6e434f07964 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Fri, 12 Mar 2021 15:32:52 +0200 Subject: [PATCH 36/47] check object is published before updating & fix typos --- metadata_backend/api/handlers.py | 23 +++++++++++++++-------- metadata_backend/api/operators.py | 2 +- metadata_backend/helpers/validator.py | 3 +++ 3 files changed, 19 insertions(+), 9 deletions(-) diff --git a/metadata_backend/api/handlers.py b/metadata_backend/api/handlers.py index 1af475e0f..607f8d290 100644 --- a/metadata_backend/api/handlers.py +++ b/metadata_backend/api/handlers.py @@ -376,7 +376,7 @@ async def delete_object(self, req: Request) -> Response: if check_user: await user_op.remove_objects(current_user, "drafts", [accession_id]) else: - reason = "This object does not seem to belong to anything." + reason = "This object does not seem to belong to any user." LOG.error(reason) raise web.HTTPUnprocessableEntity(reason=reason) @@ -388,11 +388,10 @@ async def delete_object(self, req: Request) -> Response: async def put_object(self, req: Request) -> Response: """Replace metadata object in database. - For JSON request body we validate that it consists of all the - required fields before replacing in the DB. + For JSON request we don't allow replacing in the DB. :param req: PUT request - :raises: HTTPUnauthorized if object not owned by user + :raises: HTTPUnauthorized if object not owned by user or if object is published :returns: JSON response containing accessionId for submitted object """ schema_type = req.match_info["schema"] @@ -460,6 +459,14 @@ async def patch_object(self, req: Request) -> Response: LOG.error(reason) raise web.HTTPUnauthorized(reason=reason) + folder_op = FolderOperator(db_client) + exists, _, published = await folder_op.check_object_in_folder(collection, accession_id) + if exists: + if published: + reason = "Published objects cannot be updated." + LOG.error(reason) + raise web.HTTPUnauthorized(reason=reason) + accession_id = await operator.update_metadata_object(collection, accession_id, content) body = json.dumps({"accessionId": accession_id}) @@ -490,7 +497,7 @@ def _check_patch_folder(self, patch_ops: Any) -> None: if _tags.match(op["path"]): LOG.info(f"{op['op']} on tags in folder") if "submissionType" in op["value"].keys() and op["value"]["submissionType"] not in ["XML", "Form"]: - reason = "submissionType is restricted to either XML or Form values." + reason = "submissionType is restricted to either 'XML' or 'Form' values." LOG.error(reason) raise web.HTTPBadRequest(reason=reason) pass @@ -519,7 +526,7 @@ def _check_patch_folder(self, patch_ops: Any) -> None: and "submissionType" in item["tags"] and item["tags"]["submissionType"] not in ["XML", "Form"] ): - reason = "submissionType is restricted to either XML or Form values." + reason = "submissionType is restricted to either 'XML' or 'Form' values." LOG.error(reason) raise web.HTTPBadRequest(reason=reason) @@ -720,7 +727,7 @@ def _check_patch_user(self, patch_ops: Any) -> None: if _tags.match(op["path"]): LOG.info(f"{op['op']} on tags in folder") if "submissionType" in op["value"].keys() and op["value"]["submissionType"] not in ["XML", "Form"]: - reason = "submissionType is restricted to either XML or Form values." + reason = "submissionType is restricted to either 'XML' or 'Form' values." LOG.error(reason) raise web.HTTPBadRequest(reason=reason) pass @@ -750,7 +757,7 @@ def _check_patch_user(self, patch_ops: Any) -> None: and "submissionType" in item["tags"] and item["tags"]["submissionType"] not in ["XML", "Form"] ): - reason = "submissionType is restricted to either XML or Form values." + reason = "submissionType is restricted to either 'XML' or 'Form' values." LOG.error(reason) raise web.HTTPBadRequest(reason=reason) diff --git a/metadata_backend/api/operators.py b/metadata_backend/api/operators.py index f2675130c..c8a90ccc2 100644 --- a/metadata_backend/api/operators.py +++ b/metadata_backend/api/operators.py @@ -404,7 +404,7 @@ async def _format_data_to_create_and_add_to_db(self, schema_type: str, data: Dic async def _format_data_to_replace_and_add_to_db(self, schema_type: str, accession_id: str, data: Dict) -> str: """Format JSON metadata object and replace it in db. - Replace information to object before adding to db. + Replace information in object before adding to db. We will not replace accessionId, publishDate or dateCreated, as these are generated when created. diff --git a/metadata_backend/helpers/validator.py b/metadata_backend/helpers/validator.py index 19fba7e81..4e969b2b7 100644 --- a/metadata_backend/helpers/validator.py +++ b/metadata_backend/helpers/validator.py @@ -83,6 +83,9 @@ def is_valid(self) -> bool: def extend_with_default(validator_class: Draft7Validator) -> Draft7Validator: """Include default values present in JSON Schema. + This feature is included even though some default values might cause + unwanted behaviour when submitting a schema. + Source: https://python-jsonschema.readthedocs.io FAQ """ validate_properties = validator_class.VALIDATORS["properties"] From 2dace42a588ce08955ac3c1b551c25b96b36f4e0 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Fri, 12 Mar 2021 20:16:28 +0200 Subject: [PATCH 37/47] more code refactor and cleanup --- docs/code.rst | 1 + metadata_backend/api/auth.py | 2 +- metadata_backend/api/handlers.py | 78 ++++++++++--------------------- metadata_backend/api/operators.py | 17 +++---- 4 files changed, 36 insertions(+), 62 deletions(-) diff --git a/docs/code.rst b/docs/code.rst index 16915d440..509c45a85 100644 --- a/docs/code.rst +++ b/docs/code.rst @@ -24,6 +24,7 @@ Metadata Backend API metadata_backend.api.auth metadata_backend.api.handlers + metadata_backend.api.health metadata_backend.api.middlewares metadata_backend.api.operators diff --git a/metadata_backend/api/auth.py b/metadata_backend/api/auth.py index f47a50de0..cf85bc61d 100644 --- a/metadata_backend/api/auth.py +++ b/metadata_backend/api/auth.py @@ -217,7 +217,7 @@ async def _set_user(self, req: Request, session_id: str, token: str) -> None: async def _get_key(self) -> dict: """Get OAuth2 public key and transform it to usable pem key. - :raises: 401 in case JWK could not be retrieved + :raises: HTTPUnauthorized in case JWK could not be retrieved :returns: dictionary with JWK (JSON Web Keys) """ try: diff --git a/metadata_backend/api/handlers.py b/metadata_backend/api/handlers.py index 607f8d290..2f9d0225a 100644 --- a/metadata_backend/api/handlers.py +++ b/metadata_backend/api/handlers.py @@ -56,6 +56,7 @@ async def _handle_check_ownedby_user(self, req: Request, collection: str, access db_client = req.app["db_client"] current_user = get_session(req)["user_info"] user_op = UserOperator(db_client) + _check = False if collection != "folders": @@ -63,18 +64,25 @@ async def _handle_check_ownedby_user(self, req: Request, collection: str, access check, folder_id, published = await folder_op.check_object_in_folder(collection, accession_id) if published: - return True + _check = True elif check: # if the draft object is found in folder we just need to check if the folder belongs to user - return await user_op.check_user_has_doc("folders", current_user, folder_id) + _check = await user_op.check_user_has_doc("folders", current_user, folder_id) elif collection.startswith("draft"): # if collection is draft but not found in a folder we also check if object is in drafts of the user # they will be here if they will not be deleted after publish - return await user_op.check_user_has_doc(collection, current_user, accession_id) + _check = await user_op.check_user_has_doc(collection, current_user, accession_id) else: - return False + _check = False else: - return await user_op.check_user_has_doc(collection, current_user, accession_id) + _check = await user_op.check_user_has_doc(collection, current_user, accession_id) + + if not _check: + reason = f"The ID: {accession_id} does not belong to current user." + LOG.error(reason) + raise web.HTTPUnauthorized(reason=reason) + + return _check async def _get_collection_objects( self, folder_op: AsyncIOMotorClient, collection: str, seq: List @@ -263,7 +271,6 @@ async def get_object(self, req: Request) -> Response: set, otherwise json. :param req: GET request - :raises: HTTPUnauthorized if object not owned by user :returns: JSON or XML response containing metadata object """ accession_id = req.match_info["accessionId"] @@ -278,11 +285,7 @@ async def get_object(self, req: Request) -> Response: await operator.check_exists(collection, accession_id) - check_user = await self._handle_check_ownedby_user(req, collection, accession_id) - if not check_user: - reason = f"The object {accession_id} does not belong to current user." - LOG.error(reason) - raise web.HTTPUnauthorized(reason=reason) + await self._handle_check_ownedby_user(req, collection, accession_id) data, content_type = await operator.read_metadata_object(type_collection, accession_id) @@ -343,7 +346,8 @@ async def delete_object(self, req: Request) -> Response: """Delete metadata object from database. :param req: DELETE request - :raises: HTTPUnauthorized if object not owned by user or folder published + :raises: HTTPUnauthorized if folder published + :raises: HTTPUnprocessableEntity if object does not belong to current user :returns: HTTPNoContent response """ schema_type = req.match_info["schema"] @@ -355,11 +359,7 @@ async def delete_object(self, req: Request) -> Response: await Operator(db_client).check_exists(collection, accession_id) - check_user = await self._handle_check_ownedby_user(req, collection, accession_id) - if not check_user: - reason = f"The object {accession_id} does not belong to current user." - LOG.error(reason) - raise web.HTTPUnauthorized(reason=reason) + await self._handle_check_ownedby_user(req, collection, accession_id) folder_op = FolderOperator(db_client) exists, folder_id, published = await folder_op.check_object_in_folder(collection, accession_id) @@ -391,7 +391,7 @@ async def put_object(self, req: Request) -> Response: For JSON request we don't allow replacing in the DB. :param req: PUT request - :raises: HTTPUnauthorized if object not owned by user or if object is published + :raises: HTTPUnsupportedMediaType if JSON replace is attempted :returns: JSON response containing accessionId for submitted object """ schema_type = req.match_info["schema"] @@ -416,11 +416,7 @@ async def put_object(self, req: Request) -> Response: await operator.check_exists(collection, accession_id) - check_user = await self._handle_check_ownedby_user(req, collection, accession_id) - if not check_user: - reason = f"The object {accession_id} does not belong to current user." - LOG.error(reason) - raise web.HTTPUnauthorized(reason=reason) + await self._handle_check_ownedby_user(req, collection, accession_id) accession_id = await operator.replace_metadata_object(collection, accession_id, content) @@ -434,7 +430,7 @@ async def patch_object(self, req: Request) -> Response: We do not support patch for XML. :param req: PATCH request - :raises: HTTPUnauthorized if object not owned by user + :raises: HTTPUnauthorized if object is in published folder :returns: JSON response containing accessionId for submitted object """ schema_type = req.match_info["schema"] @@ -453,11 +449,7 @@ async def patch_object(self, req: Request) -> Response: await operator.check_exists(collection, accession_id) - check_user = await self._handle_check_ownedby_user(req, collection, accession_id) - if not check_user: - reason = f"The object {accession_id} does not belong to current user." - LOG.error(reason) - raise web.HTTPUnauthorized(reason=reason) + await self._handle_check_ownedby_user(req, collection, accession_id) folder_op = FolderOperator(db_client) exists, _, published = await folder_op.check_object_in_folder(collection, accession_id) @@ -589,11 +581,7 @@ async def get_folder(self, req: Request) -> Response: await operator.check_folder_exists(folder_id) - check_user = await self._handle_check_ownedby_user(req, "folders", folder_id) - if not check_user: - reason = f"The folder {folder_id} does not belong to current user." - LOG.error(reason) - raise web.HTTPNotFound(reason=reason) + await self._handle_check_ownedby_user(req, "folders", folder_id) folder = await operator.read_folder(folder_id) @@ -604,8 +592,6 @@ async def patch_folder(self, req: Request) -> Response: """Update object folder with a specific folder id. :param req: PATCH request - :raises: HTTPUnauthorized if folder not owned by user, HTTPBadRequest if JSONpatch operation - is not allowed :returns: JSON response containing folder ID for updated folder """ folder_id = req.match_info["folderId"] @@ -619,11 +605,7 @@ async def patch_folder(self, req: Request) -> Response: patch_ops = await self._get_data(req) self._check_patch_folder(patch_ops) - check_user = await self._handle_check_ownedby_user(req, "folders", folder_id) - if not check_user: - reason = f"The folder {folder_id} does not belong to current user." - LOG.error(reason) - raise web.HTTPUnauthorized(reason=reason) + await self._handle_check_ownedby_user(req, "folders", folder_id) folder = await operator.update_folder(folder_id, patch_ops if isinstance(patch_ops, list) else [patch_ops]) @@ -635,7 +617,6 @@ async def publish_folder(self, req: Request) -> Response: """Update object folder specifically into published state. :param req: PATCH request - :raises: HTTPUnauthorized if folder not owned by user :returns: JSON response containing folder ID for updated folder """ folder_id = req.match_info["folderId"] @@ -644,11 +625,7 @@ async def publish_folder(self, req: Request) -> Response: await operator.check_folder_exists(folder_id) - check_user = await self._handle_check_ownedby_user(req, "folders", folder_id) - if not check_user: - reason = f"The folder {folder_id} does not belong to current user." - LOG.error(reason) - raise web.HTTPUnauthorized(reason=reason) + await self._handle_check_ownedby_user(req, "folders", folder_id) folder = await operator.read_folder(folder_id) @@ -672,7 +649,6 @@ async def delete_folder(self, req: Request) -> Response: """Delete object folder from database. :param req: DELETE request - :raises: HTTPUnauthorized if folder not owned by user :returns: HTTP No Content response """ folder_id = req.match_info["folderId"] @@ -682,11 +658,7 @@ async def delete_folder(self, req: Request) -> Response: await operator.check_folder_exists(folder_id) await operator.check_folder_published(folder_id) - check_user = await self._handle_check_ownedby_user(req, "folders", folder_id) - if not check_user: - reason = f"The folder {folder_id} does not belong to current user." - LOG.error(reason) - raise web.HTTPUnauthorized(reason=reason) + await self._handle_check_ownedby_user(req, "folders", folder_id) obj_ops = Operator(db_client) diff --git a/metadata_backend/api/operators.py b/metadata_backend/api/operators.py index c8a90ccc2..7ad813f5b 100644 --- a/metadata_backend/api/operators.py +++ b/metadata_backend/api/operators.py @@ -603,9 +603,10 @@ async def check_object_in_folder(self, collection: str, accession_id: str) -> Tu """ try: folder_path = "drafts" if collection.startswith("draft") else "metadataObjects" - folder_query = {folder_path: {"$elemMatch": {"accessionId": accession_id, "schema": collection}}} - folder_cursor = self.db_service.query("folder", folder_query) + folder_cursor = self.db_service.query( + "folder", {folder_path: {"$elemMatch": {"accessionId": accession_id, "schema": collection}}} + ) folder_check = [folder async for folder in folder_cursor] except (ConnectionFailure, OperationFailure) as error: reason = f"Error happened while inserting user: {error}" @@ -632,9 +633,10 @@ async def get_collection_objects(self, folder_id: str, collection: str) -> List: """ try: folder_path = "drafts" if collection.startswith("draft") else "metadataObjects" - folder_query = {"$and": [{folder_path: {"$elemMatch": {"schema": collection}}}, {"folderId": folder_id}]} - folder_cursor = self.db_service.query("folder", folder_query) + folder_cursor = self.db_service.query( + "folder", {"$and": [{folder_path: {"$elemMatch": {"schema": collection}}}, {"folderId": folder_id}]} + ) folders = [folder async for folder in folder_cursor] except (ConnectionFailure, OperationFailure) as error: reason = f"Error happened while inserting user: {error}" @@ -680,8 +682,8 @@ async def query_folders(self, que: Dict) -> List: :param que: Dict containing query information :returns: Query result as list """ - folder_cursor = self.db_service.query("folder", que) - folders = [folder async for folder in folder_cursor] + _cursor = self.db_service.query("folder", que) + folders = [folder async for folder in _cursor] return folders async def read_folder(self, folder_id: str) -> Dict: @@ -938,8 +940,7 @@ async def assign_objects(self, user_id: str, collection: str, object_ids: List) """ try: await self._check_user_exists(user_id) - upd_content = {collection: {"$each": object_ids}} - assign_success = await self.db_service.append("user", user_id, upd_content) + assign_success = await self.db_service.append("user", user_id, {collection: {"$each": object_ids}}) except (ConnectionFailure, OperationFailure) as error: reason = f"Error happened while getting user: {error}" LOG.error(reason) From 485fcb5bd2b5d698fd2e75e1e3b9975e436eb9e4 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Mon, 15 Mar 2021 13:00:44 +0200 Subject: [PATCH 38/47] externalId to external_id for vars and functions --- metadata_backend/api/operators.py | 8 ++++---- metadata_backend/database/db_service.py | 6 +++--- tests/test_db_service.py | 8 ++++---- tests/test_operators.py | 8 ++++---- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/metadata_backend/api/operators.py b/metadata_backend/api/operators.py index 7ad813f5b..ee3fcad99 100644 --- a/metadata_backend/api/operators.py +++ b/metadata_backend/api/operators.py @@ -861,19 +861,19 @@ async def create_user(self, data: Tuple) -> str: """ user_data: Dict[str, Union[list, str]] = dict() - externalId = data[0] # this also can be sub key + external_id = data[0] # this also can be sub key name = data[1] try: - existing_user_id = await self.db_service.exists_user_by_externalId(externalId, name) + existing_user_id = await self.db_service.exists_user_by_external_id(external_id, name) if existing_user_id: - LOG.info(f"User with identifier: {externalId} exists, no need to create.") + LOG.info(f"User with identifier: {external_id} exists, no need to create.") return existing_user_id else: user_data["drafts"] = [] user_data["folders"] = [] user_data["userId"] = user_id = self._generate_user_id() user_data["name"] = name - user_data["externalId"] = externalId + user_data["externalId"] = external_id JSONValidator(user_data, "users") insert_success = await self.db_service.create("user", user_data) if not insert_success: diff --git a/metadata_backend/database/db_service.py b/metadata_backend/database/db_service.py index 1d1761811..d19c346f4 100644 --- a/metadata_backend/database/db_service.py +++ b/metadata_backend/database/db_service.py @@ -92,15 +92,15 @@ async def exists(self, collection: str, accession_id: str) -> bool: return True if exists else False @auto_reconnect - async def exists_user_by_externalId(self, externalId: str, name: str) -> Union[None, str]: + async def exists_user_by_external_id(self, external_id: str, name: str) -> Union[None, str]: """Check user exists by its eppn. :param eppn: eduPersonPrincipalName to be searched :returns: True if exists and False if it does not """ - find_by_id = {"externalId": externalId, "name": name} + find_by_id = {"externalId": external_id, "name": name} user = await self.database["user"].find_one(find_by_id, {"_id": False, "externalId": False}) - LOG.debug(f"DB check user exists for {externalId} returned {user}.") + LOG.debug(f"DB check user exists for {external_id} returned {user}.") return user["userId"] if user else None @auto_reconnect diff --git a/tests/test_db_service.py b/tests/test_db_service.py index 538ff95ca..0153138f4 100644 --- a/tests/test_db_service.py +++ b/tests/test_db_service.py @@ -159,19 +159,19 @@ async def test_published_folder_returns_data(self): ) self.assertEqual(found_folder, True) - async def test_externalId_exists_returns_false(self): + async def test_external_id_exists_returns_false(self): """Test that externalId exists method works and returns None.""" self.collection.find_one.return_value = None - found_doc = await self.test_service.exists_user_by_externalId("test_user@eppn.fi", "name") + found_doc = await self.test_service.exists_user_by_external_id("test_user@eppn.fi", "name") self.assertEqual(found_doc, None) self.collection.find_one.assert_called_once_with( {"externalId": "test_user@eppn.fi", "name": "name"}, {"_id": False, "externalId": False} ) - async def test_externalId_exists_returns_true(self): + async def test_external_id_exists_returns_true(self): """Test that externalId exists method works and returns user id.""" self.collection.find_one.return_value = self.user_stub - found_doc = await self.test_service.exists_user_by_externalId("test_user@eppn.fi", "name") + found_doc = await self.test_service.exists_user_by_external_id("test_user@eppn.fi", "name") self.assertEqual(found_doc, self.user_id_stub) self.collection.find_one.assert_called_once_with( {"externalId": "test_user@eppn.fi", "name": "name"}, {"_id": False, "externalId": False} diff --git a/tests/test_operators.py b/tests/test_operators.py index 4b9e88bc5..1ccd7b423 100644 --- a/tests/test_operators.py +++ b/tests/test_operators.py @@ -810,7 +810,7 @@ async def test_create_user_works_and_returns_userId(self): """Test create method for users work.""" operator = UserOperator(self.client) data = "externalId", "name" - operator.db_service.exists_user_by_externalId.return_value = None + operator.db_service.exists_user_by_external_id.return_value = None operator.db_service.create.return_value = True user = await operator.create_user(data) operator.db_service.create.assert_called_once() @@ -820,7 +820,7 @@ async def test_create_user_on_create_fails(self): """Test create method fails on db create.""" operator = UserOperator(self.client) data = "externalId", "name" - operator.db_service.exists_user_by_externalId.return_value = None + operator.db_service.exists_user_by_external_id.return_value = None operator.db_service.create.return_value = False with self.assertRaises(HTTPBadRequest): await operator.create_user(data) @@ -867,7 +867,7 @@ async def test_create_user_works_existing_userId(self): """Test create method for existing user.""" operator = UserOperator(self.client) data = "eppn", "name" - operator.db_service.exists_user_by_externalId.return_value = self.user_generated_id + operator.db_service.exists_user_by_external_id.return_value = self.user_generated_id user = await operator.create_user(data) operator.db_service.create.assert_not_called() self.assertEqual(user, self.user_generated_id) @@ -876,7 +876,7 @@ async def test_create_user_fails(self): """Test create user fails.""" data = "eppn", "name" operator = UserOperator(self.client) - operator.db_service.exists_user_by_externalId.side_effect = ConnectionFailure + operator.db_service.exists_user_by_external_id.side_effect = ConnectionFailure with self.assertRaises(HTTPBadRequest): await operator.create_user(data) From 999ce706d8693c17fa00b9567590e64d33d8d6a1 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Mon, 15 Mar 2021 13:41:45 +0200 Subject: [PATCH 39/47] customise build branch for front-end --- .github/workflows/production.yml | 11 +++++++++++ Dockerfile | 4 +++- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/.github/workflows/production.yml b/.github/workflows/production.yml index 7f1b7023e..cc70326fb 100644 --- a/.github/workflows/production.yml +++ b/.github/workflows/production.yml @@ -31,6 +31,15 @@ jobs: with: driver-opts: network=host + - name: Set build target branch + run: | + if [[ ${{ github.event.pull_request.base.ref }} == master ]]; then + BRANCH=master + else + BRANCH=develop + fi + echo "BUILD_BRANCH=$BRANCH" >> $GITHUB_ENV + - name: Build uses: docker/build-push-action@v2 with: @@ -40,6 +49,8 @@ jobs: tags: localhost:5000/metadata-submitter:latest cache-from: localhost:5000/metadata-submitter:latest cache-to: type=local,dest=/tmp/.buildx-cache + build-args: | + BRANCH=${{ env.BUILD_BRANCH }} - name: Run production container run: docker run --rm -p 5430:5430 --name ms -d -t localhost:5000/metadata-submitter:latest diff --git a/Dockerfile b/Dockerfile index 5396e1f36..04c312cc3 100644 --- a/Dockerfile +++ b/Dockerfile @@ -4,7 +4,9 @@ RUN apk add --update \ && apk add --no-cache git\ && rm -rf /var/cache/apk/* -RUN git clone https://github.com/CSCfi/metadata-submitter-frontend.git +ARG BRANCH=master + +RUN git clone -b ${BRANCH} https://github.com/CSCfi/metadata-submitter-frontend.git WORKDIR /metadata-submitter-frontend RUN npm install -g npm@7.6.0 \ From 1047c091a0afd29935f55760bfe6c98a9c04f358 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Mon, 15 Mar 2021 14:36:23 +0200 Subject: [PATCH 40/47] publish adjustments --- .github/workflows/publish.yml | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index 3d8a29954..bc60397a0 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -21,14 +21,18 @@ jobs: VERSION=edge if [[ $GITHUB_REF == refs/tags/* ]]; then VERSION=${GITHUB_REF#refs/tags/} + BRANCH=develop elif [[ $GITHUB_REF == refs/heads/* ]]; then BRANCH=$(echo ${GITHUB_REF#refs/heads/} | sed -r 's#/+#-#g') if [[ $BRANCH == master ]]; then VERSION=latest + BRANCH=master elif [[ $BRANCH == develop ]]; then VERSION=stage + BRANCH=develop fi fi + echo "BUILD_BRANCH=$BRANCH" >> $GITHUB_ENV TAGS="${DOCKER_IMAGE}:${VERSION}" echo ::set-output name=version::${VERSION} echo ::set-output name=tags::${TAGS} @@ -51,6 +55,8 @@ jobs: file: ./Dockerfile push: ${{ github.event_name != 'pull_request' }} tags: ${{ steps.prep.outputs.tags }} + build-args: | + BRANCH=${{ env.BUILD_BRANCH }} cache-from: type=registry,ref=cscfi/metadata-submitter:latest cache-to: type=local,dest=/tmp/.buildx-cache labels: | From ba1e91efdb28b4a1236bb1135190e66ac6938301 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Tue, 16 Mar 2021 12:29:05 +0200 Subject: [PATCH 41/47] run dependabot monday at 7 to update dependencies --- .github/dependabot.yml | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 .github/dependabot.yml diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 000000000..98a227276 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,10 @@ +version: 2 +updates: + - package-ecosystem: "pip" + directory: "/" + schedule: + interval: "weekly" + day: "monday" + labels: + - "pip dependencies" + open-pull-requests-limit: 10 From e7c81d398fe131a15f3d7a01c508d139fa1f040a Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Tue, 16 Mar 2021 13:23:57 +0200 Subject: [PATCH 42/47] watch github actions and docker dependecies --- .github/dependabot.yml | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 98a227276..10c35c1da 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -8,3 +8,19 @@ updates: labels: - "pip dependencies" open-pull-requests-limit: 10 + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "weekly" + day: "monday" + labels: + - "github actions" + open-pull-requests-limit: 10 + - package-ecosystem: docker + directory: "/" + schedule: + interval: "weekly" + day: "monday" + labels: + - "docker dependencies" + open-pull-requests-limit: 10 From cd85804255ba7f14c6f2d9986b5edca82dd42d6c Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Sun, 21 Mar 2021 19:13:58 +0200 Subject: [PATCH 43/47] bump to 0.9.0 --- docs/conf.py | 2 +- metadata_backend/__init__.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/conf.py b/docs/conf.py index f263de16b..7147af2a7 100644 --- a/docs/conf.py +++ b/docs/conf.py @@ -11,7 +11,7 @@ author = "CSC Developers" # The full version, including alpha/beta/rc tags -release = "0.8.1" +release = "0.9.0" # -- General configuration --------------------------------------------------- diff --git a/metadata_backend/__init__.py b/metadata_backend/__init__.py index ae3078524..16234b251 100644 --- a/metadata_backend/__init__.py +++ b/metadata_backend/__init__.py @@ -1,5 +1,5 @@ """Backend for submitting and validating XML Files containing ENA metadata.""" __title__ = "metadata_backend" -__version__ = VERSION = "0.8.1" +__version__ = VERSION = "0.9.0" __author__ = "CSC Developers" From 262aa284f942ac7f67e12145a938d4c0755a4d80 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Mon, 22 Mar 2021 09:09:24 +0200 Subject: [PATCH 44/47] use front-end master branch on tags --- .github/workflows/publish.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index bc60397a0..44dad60c4 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -21,7 +21,7 @@ jobs: VERSION=edge if [[ $GITHUB_REF == refs/tags/* ]]; then VERSION=${GITHUB_REF#refs/tags/} - BRANCH=develop + BRANCH=master elif [[ $GITHUB_REF == refs/heads/* ]]; then BRANCH=$(echo ${GITHUB_REF#refs/heads/} | sed -r 's#/+#-#g') if [[ $BRANCH == master ]]; then From 80cebce5b173a285ef04bfa5f039898ff5655f02 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Mon, 22 Mar 2021 09:19:07 +0200 Subject: [PATCH 45/47] update api specification and fix users schema --- docs/specification.yml | 92 +++++++++++++++------ metadata_backend/helpers/schemas/users.json | 11 ++- 2 files changed, 79 insertions(+), 24 deletions(-) diff --git a/docs/specification.yml b/docs/specification.yml index 51c0d7b8c..aa6cf8d6f 100644 --- a/docs/specification.yml +++ b/docs/specification.yml @@ -46,6 +46,12 @@ paths: application/json: schema: $ref: "#/components/schemas/401Unauthorized" + 403: + description: Forbidden + content: + application/json: + schema: + $ref: "#/components/schemas/403Forbidden" /validate: post: tags: @@ -76,6 +82,12 @@ paths: application/json: schema: $ref: "#/components/schemas/401Unauthorized" + 403: + description: Forbidden + content: + application/json: + schema: + $ref: "#/components/schemas/403Forbidden" /schemas: get: tags: @@ -100,35 +112,49 @@ paths: application/json: schema: $ref: "#/components/schemas/401Unauthorized" - /objects: + 403: + description: Forbidden + content: + application/json: + schema: + $ref: "#/components/schemas/403Forbidden" + /schemas/{schema}: get: tags: - Query - summary: List of objects. By default it will show 10 results per page. + summary: Retrieve JSON schema by its name. parameters: - - in: query - name: page - schema: - type: string - description: Page number - - in: query - name: per_page + - name: schema + in: path + description: Name of the schema. schema: type: string - description: Results per page + required: true responses: 200: description: OK content: application/json: schema: - $ref: "#/components/schemas/ObjectList" - 404: - description: Not Found + type: object + 400: + description: Bad Request content: application/json: schema: - $ref: "#/components/schemas/404NotFound" + $ref: "#/components/schemas/400BadRequest" + 401: + description: Unauthorized + content: + application/json: + schema: + $ref: "#/components/schemas/401Unauthorized" + 403: + description: Forbidden + content: + application/json: + schema: + $ref: "#/components/schemas/403Forbidden" /objects/{schema}: get: tags: @@ -304,6 +330,12 @@ paths: application/json: schema: $ref: "#/components/schemas/401Unauthorized" + 403: + description: Forbidden + content: + application/json: + schema: + $ref: "#/components/schemas/403Forbidden" /objects/{schema}/{accessionId}: get: tags: @@ -413,9 +445,6 @@ paths: schema: type: string format: binary - application/json: - schema: - type: object parameters: - name: schema in: path @@ -644,9 +673,6 @@ paths: schema: type: string format: binary - application/json: - schema: - type: object parameters: - name: schema in: path @@ -1323,8 +1349,27 @@ components: drafts: type: array items: - type: string - description: Folder Id + type: object + required: + - accessionId + - schema + additionalProperties: false + properties: + accessionId: + type: string + description: Accession id generated to identify an object + schema: + type: string + description: type of schema this Accession ID relates to and was added in submit + tags: + type: object + description: Different tags to describe the object. + additionalProperties: true + properties: + submissionType: + type: string + description: Type of submission + enum: ["XML", "Form"] folders: type: array items: @@ -1351,11 +1396,12 @@ components: op: type: string description: JSON Patch operation + enum: ["add", "replace"] example: add path: type: string description: Key that will be patched - example: /metadataObjects + example: /metadataObjects/- value: additionalProperties: oneOf: diff --git a/metadata_backend/helpers/schemas/users.json b/metadata_backend/helpers/schemas/users.json index d17a45f87..0f021b1eb 100644 --- a/metadata_backend/helpers/schemas/users.json +++ b/metadata_backend/helpers/schemas/users.json @@ -37,7 +37,16 @@ "type": "object", "title": "Different tags to describe the template object.", "additionalProperties": true, - "properties": {} + "properties": { + "submissionType": { + "type": "string", + "title": "Type of submission", + "enum": [ + "XML", + "Form" + ] + } + } } } }, From 0430228d764e38ab9d06632b794dcd5ea887c9c1 Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Mon, 22 Mar 2021 09:41:42 +0200 Subject: [PATCH 46/47] update front-end description --- docs/frontend.rst | 110 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 108 insertions(+), 2 deletions(-) diff --git a/docs/frontend.rst b/docs/frontend.rst index 8796fa77f..d1d84da5f 100644 --- a/docs/frontend.rst +++ b/docs/frontend.rst @@ -41,11 +41,10 @@ To run the frontend from command line use: After installing and running, frontend can be found from ``http://localhost:3000``. -.. hint:: Some functionality in frontend required a working backend. +.. hint:: Some functionality in frontend requires a working backend. Follow the instructions in :ref:`deploy` for setting it up. - Internal structure ------------------ @@ -65,6 +64,113 @@ Form components are crucial part of the application: ``react-hook-form`` based forms from given schema. The forms are validated against the JSON schema with ``Ajv``. React-hook-form is used for performance reasons: it uses uncontrolled components so adding a lot of fields to array doesn't slow rendering of the application. +Constants +--------- + +Folder ``src/constants`` holds all the constants used in the application. The constants are uniquely defined and separated into different files according to its related context. For example, the file ``constants/wizardObject.js`` contains unique constants regarding to ``wizardObject`` such as: ``ObjectTypes, ObjectStatus, etc.`` + +The purposes of using these `constants` are: + +- to avoid hard coding the values of variables repeatedly +- to keep the consistency when defining the values of variables +- to reuse those predefined values across the application + +Example of defining and using a constant: + +- First, define the constant object ``ObjectSubmissionTypes`` in ``constants/wizardObject.js`` + +.. code-block:: javascript + + export const ObjectSubmissionTypes = { + form: "Form", + xml: "XML", + existing: "Existing", + } + + +- Then, use this constant in `WizardComponents/WizardObjectIndex`: + +.. code-block:: javascript + + import { ObjectSubmissionTypes } from "constants/wizardObject" + + switch (currentSubmissionType) { + case ObjectSubmissionTypes.form: { + target = "form" + break + } + case ObjectSubmissionTypes.xml: { + target = "XML upload" + break + } + case ObjectSubmissionTypes.existing: { + target = "drafts" + break + } + } + + +Commonly used data types +------------------------ + +All commonly used data types of variables are defined in the file ``index.js`` in folder ``src/types``. The purposes are: + +- to avoid hard coding the same data types frequently in different files +- to keep track and consistency of the data types across different files + +For example: + +- declare and export these data types in ``src/types/index.js`` + +.. code-block:: javascript + + export type ObjectInsideFolder = { + accessionId: string, + schema: string, + } + + export type ObjectTags = { + submissionType: string, + fileName?: string, + } + + export type ObjectInsideFolderWithTags = ObjectInsideFolder & { tags: ObjectTags } + + +- import and reuse the data types in different files: +- Reuse type ``ObjectInsideFolder`` in ``features/wizardSubmissionFolderSlice.js``: + +.. code-block:: javascript + + import type { ObjectInsideFolder } from "types" + + export const addObjectToFolder = ( + folderID: string, + objectDetails: ObjectInsideFolder + ) => {} + + export const addObjectToDrafts = ( + folderID: string, + objectDetails: ObjectInsideFolder + ) => {} + + +- Reuse type ``ObjectInsideFolderWithTags`` consequently in both ``WizardComponents/WizardSavedObjectsList.js`` and ``WizardSteps/WizardShowSummaryStep.js``: + +.. code-block:: javascript + + import type { ObjectInsideFolderWithTags } from "types" + + type WizardSavedObjectsListProps = { submissions: Array } + + +.. code-block:: javascript + + import type { ObjectInsideFolderWithTags } from "types" + + type GroupedBySchema = {| [Schema]: Array |} + + Redux store ----------- From 78ff08853aeefa66edff207c34c9c6984e537aca Mon Sep 17 00:00:00 2001 From: Stefan Negru Date: Mon, 22 Mar 2021 11:04:26 +0200 Subject: [PATCH 47/47] target develop branch by default --- .github/dependabot.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 10c35c1da..c51b4589e 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -7,6 +7,7 @@ updates: day: "monday" labels: - "pip dependencies" + target-branch: "develop" open-pull-requests-limit: 10 - package-ecosystem: "github-actions" directory: "/" @@ -15,6 +16,7 @@ updates: day: "monday" labels: - "github actions" + target-branch: "develop" open-pull-requests-limit: 10 - package-ecosystem: docker directory: "/" @@ -23,4 +25,5 @@ updates: day: "monday" labels: - "docker dependencies" + target-branch: "develop" open-pull-requests-limit: 10