From 7351fde1abc81201b0d32a667296c94b0bd446d1 Mon Sep 17 00:00:00 2001 From: Martin Lettry Date: Wed, 19 Apr 2023 14:06:35 +0200 Subject: [PATCH] model: Update role_id column * fix role instantiation * closes https://github.com/inveniosoftware/invenio-app-rdm/issues/2186 Co-authored-by: jrcastro2 --- ...nge_fk_accountsrole_to_string_downgrade.py | 40 ++++++++++++++++ ...hange_fk_accountsrole_to_string_upgrade.py | 47 +++++++++++++++++++ invenio_access/models.py | 2 +- run-tests.sh | 27 ++++++++++- setup.cfg | 4 +- tests/test_permissions.py | 2 +- tests/test_permissions_cache.py | 16 +++---- 7 files changed, 124 insertions(+), 14 deletions(-) create mode 100644 invenio_access/alembic/842a62b56e60_change_fk_accountsrole_to_string_downgrade.py create mode 100644 invenio_access/alembic/f9843093f686_change_fk_accountsrole_to_string_upgrade.py diff --git a/invenio_access/alembic/842a62b56e60_change_fk_accountsrole_to_string_downgrade.py b/invenio_access/alembic/842a62b56e60_change_fk_accountsrole_to_string_downgrade.py new file mode 100644 index 0000000..7f38136 --- /dev/null +++ b/invenio_access/alembic/842a62b56e60_change_fk_accountsrole_to_string_downgrade.py @@ -0,0 +1,40 @@ +# This file is part of Invenio. +# Copyright (C) 2016-2018 CERN. +# +# Invenio is free software; you can redistribute it and/or modify it +# under the terms of the MIT License; see LICENSE file for more details. + +"""Change FK AccountsRole to string (downgrade recipe).""" + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision = "842a62b56e60" +down_revision = "04480be1593e" +branch_labels = () +depends_on = None + + +def upgrade(): + """Upgrade database.""" + pass + + +def downgrade(): + """Downgrade database.""" + op.alter_column( + "access_actionsroles", + "role_id", + existing_type=sa.String(80), + type_=sa.Integer, + postgresql_using="role_id::integer", + ) + op.create_foreign_key( + op.f("fk_access_actionsroles_role_id_accounts_role"), + "access_actionsroles", + "accounts_role", + ["role_id"], + ["id"], + ondelete="CASCADE", + ) diff --git a/invenio_access/alembic/f9843093f686_change_fk_accountsrole_to_string_upgrade.py b/invenio_access/alembic/f9843093f686_change_fk_accountsrole_to_string_upgrade.py new file mode 100644 index 0000000..93ce17d --- /dev/null +++ b/invenio_access/alembic/f9843093f686_change_fk_accountsrole_to_string_upgrade.py @@ -0,0 +1,47 @@ +# +# This file is part of Invenio. +# Copyright (C) 2016-2018 CERN. +# +# Invenio is free software; you can redistribute it and/or modify it +# under the terms of the MIT License; see LICENSE file for more details. + +"""Change FK AccountsRole to string (upgrade recipe). + +This recipe only contains the upgrade because as it directly depends on invenio-accounts recipe. That recipe is in +charge of deleting all the constraints on the role_id, including foreign keys using the role_id declared in this module. +Therefore, when in order to downgrade we need to split the recipes to be able to first execute the recipe in +invenio-accounts (f2522cdd5fcd) and after that we can execute the downgrade recipe (842a62b56e60). +""" + +import sqlalchemy as sa +from alembic import op + +# revision identifiers, used by Alembic. +revision = "f9843093f686" +down_revision = ("f2522cdd5fcd", "842a62b56e60") # Depends on invenio-accounts revision id (f2522cdd5fcd) +branch_labels = () +depends_on = None + + +def upgrade(): + """Upgrade database.""" + op.alter_column( + "access_actionsroles", + "role_id", + existing_type=sa.Integer, + type_=sa.String(80), + postgresql_using="role_id::integer", + ) + op.create_foreign_key( + op.f("fk_access_actionsroles_role_id_accounts_role"), + "access_actionsroles", + "accounts_role", + ["role_id"], + ["id"], + ondelete="CASCADE", + ) + + +def downgrade(): + """Downgrade database.""" + pass diff --git a/invenio_access/models.py b/invenio_access/models.py index e83af76..b915712 100644 --- a/invenio_access/models.py +++ b/invenio_access/models.py @@ -156,7 +156,7 @@ class ActionRoles(ActionNeedMixin, db.Model): ) role_id = db.Column( - db.Integer(), + db.String(80), db.ForeignKey(Role.id, ondelete="CASCADE"), nullable=False, index=True, diff --git a/run-tests.sh b/run-tests.sh index fc86e5b..8fa27d1 100755 --- a/run-tests.sh +++ b/run-tests.sh @@ -18,12 +18,35 @@ set -o nounset function cleanup() { eval "$(docker-services-cli down --env)" } -trap cleanup EXIT + +# Check for arguments +# Note: "-k" would clash with "pytest" +keep_services=0 +pytest_args=() +for arg in $@; do + # from the CLI args, filter out some known values and forward the rest to "pytest" + # note: we don't use "getopts" here b/c of some limitations (e.g. long options), + # which means that we can't combine short options (e.g. "./run-tests -Kk pattern") + case ${arg} in + -K|--keep-services) + keep_services=1 + ;; + *) + pytest_args+=( ${arg} ) + ;; + esac +done + +if [[ ${keep_services} -eq 0 ]]; then + trap cleanup EXIT +fi python -m check_manifest python -m setup extract_messages --dry-run python -m sphinx.cmd.build -qnNW docs docs/_build/html eval "$(docker-services-cli up --db ${DB:-postgresql} --cache ${CACHE:-redis} --env)" -python -m pytest +# Note: expansion of pytest_args looks like below to not cause an unbound +# variable error when 1) "nounset" and 2) the array is empty. +python -m pytest ${pytest_args[@]+"${pytest_args[@]}"} tests_exit_code=$? exit "$tests_exit_code" diff --git a/setup.cfg b/setup.cfg index d4467f0..507f701 100644 --- a/setup.cfg +++ b/setup.cfg @@ -28,7 +28,7 @@ python_requires = >=3.7 zip_safe = False install_requires = invenio-admin>=1.2.0 - invenio-accounts>=1.4.13 + invenio-accounts>=2.2.0 invenio-base>=1.2.11 invenio-i18n>=2.0.0 @@ -37,7 +37,7 @@ tests = pytest-black>=0.3.0,<0.3.10 cachelib>=0.1 pytest-invenio>=1.4.1 - invenio-db[mysql, postgresql]>=1.0.14 + invenio-db[mysql, postgresql]>=1.1.2 redis>=2.10.5 sphinx>=4.5 # Kept for backwards compatibility diff --git a/tests/test_permissions.py b/tests/test_permissions.py index cfbf60d..023a54e 100644 --- a/tests/test_permissions.py +++ b/tests/test_permissions.py @@ -84,7 +84,7 @@ def create_roles(*names): """Helper to create roles.""" roles = [] for name in names: - role = Role(name=name) + role = Role(id=name, name=name) db.session.add(role) roles.append(role) db.session.commit() diff --git a/tests/test_permissions_cache.py b/tests/test_permissions_cache.py index a732d8f..ddebeac 100644 --- a/tests/test_permissions_cache.py +++ b/tests/test_permissions_cache.py @@ -147,7 +147,7 @@ def test_invenio_access_permission_cache_redis(app, dynamic_permission): ) -def test_intenio_access_cache_performance(app, dynamic_permission): +def test_invenio_access_cache_performance(app, dynamic_permission): """Performance test simulating 1000 users.""" InvenioAccess(app, cache=None) with app.test_request_context(): @@ -161,7 +161,7 @@ def test_intenio_access_cache_performance(app, dynamic_permission): roles = [] actions = [] for i in range(actions_roles_number): - role = Role(name="role{0}".format(i)) + role = Role(id=str(i), name="role{0}".format(i)) roles.append(role) db.session.add(role) db.session.flush() @@ -472,12 +472,12 @@ def test_invenio_access_permission_cache_roles_updates(app, dynamic_permission): InvenioAccess(app, cache=cache) with app.test_request_context(): # Creation of some data to test. - role_1 = Role(name="role_1") - role_2 = Role(name="role_2") - role_3 = Role(name="role_3") - role_4 = Role(name="role_4") - role_5 = Role(name="role_5") - role_6 = Role(name="role_6") + role_1 = Role(id="role_1", name="role_1") + role_2 = Role(id="role_2", name="role_2") + role_3 = Role(id="role_3", name="role_3") + role_4 = Role(id="role_4", name="role_4") + role_5 = Role(id="role_5", name="role_5") + role_6 = Role(id="role_6", name="role_6") db.session.add(role_1) db.session.add(role_2)