From 110542c4cb2c585990900e2473dba76f8e087022 Mon Sep 17 00:00:00 2001 From: Joao Paulo Ramos Date: Fri, 28 Jul 2023 18:07:17 -0300 Subject: [PATCH 1/4] fix(RHINENG-1284): Add constrains to AssignmentRule model --- app/models.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/app/models.py b/app/models.py index 7d7c304d3..46faabd69 100644 --- a/app/models.py +++ b/app/models.py @@ -480,7 +480,11 @@ def __init__( class AssignmentRule(db.Model): __tablename__ = "assignment_rules" - __table_args__ = (Index("idxassrulesorgid", "org_id"),) + __table_args__ = ( + Index("idxassrulesorgid", "org_id"), + UniqueConstraint("org_id", "name", name="assignment_rules_org_id_name_key"), + UniqueConstraint("group_id", name="assignment_rules_unique_group_id"), + ) def __init__( self, @@ -524,7 +528,7 @@ def update(self, input_ar): account = db.Column(db.String(10)) name = db.Column(db.String(255), nullable=False) description = db.Column(db.String(255)) - group_id = db.Column(UUID(as_uuid=True), ForeignKey("groups.id")) + group_id = db.Column(UUID(as_uuid=True), ForeignKey("groups.id"), primary_key=True) filter = db.Column(JSONB, nullable=False) enabled = db.Column(db.Boolean(), default=True) created_on = db.Column(db.DateTime(timezone=True), default=_time_now) From 79d26b2c0539e3a11a171cf3001c91f0b48c2e16 Mon Sep 17 00:00:00 2001 From: Joao Paulo Ramos Date: Fri, 28 Jul 2023 18:08:21 -0300 Subject: [PATCH 2/4] fix(RHINENG-1284): Fix tests --- tests/fixtures/db_fixtures.py | 4 ++-- tests/test_api_assignment_rules_create.py | 10 ++++------ tests/test_api_assignment_rules_get.py | 12 ++++++------ tests/test_models.py | 6 +++--- 4 files changed, 15 insertions(+), 17 deletions(-) diff --git a/tests/fixtures/db_fixtures.py b/tests/fixtures/db_fixtures.py index 5a69ea748..c8b12d818 100644 --- a/tests/fixtures/db_fixtures.py +++ b/tests/fixtures/db_fixtures.py @@ -107,8 +107,8 @@ def _db_get_groups_for_host(host_id): @pytest.fixture(scope="function") def db_get_assignment_rule(flask_app): - def _db_get_assignment_rule(ar_id): - return AssignmentRule.query.get(ar_id) + def _db_get_assignment_rule(ar_id, group_id): + return AssignmentRule.query.get((ar_id, group_id)) return _db_get_assignment_rule diff --git a/tests/test_api_assignment_rules_create.py b/tests/test_api_assignment_rules_create.py index c6c0804ec..19f8fa537 100644 --- a/tests/test_api_assignment_rules_create.py +++ b/tests/test_api_assignment_rules_create.py @@ -1,5 +1,3 @@ -import pytest - from tests.helpers.api_utils import assert_assign_rule_response from tests.helpers.api_utils import assert_response_status from tests.helpers.api_utils import create_mock_rbac_response @@ -63,9 +61,8 @@ def test_create_assignment_rule_group_not_exists(api_create_assign_rule): assert assign_rule_data["group_id"] in response_data["detail"] -@pytest.mark.skip(reason="Need to validate why the test is creating duplicates in the DB") def test_create_assignemnt_rule_same_name(api_create_assign_rule, db_create_group): - group = db_create_group("my_group") + group = db_create_group("mygroup") assign_rule_data = { "name": "myRule1", "description": "something", @@ -75,13 +72,14 @@ def test_create_assignemnt_rule_same_name(api_create_assign_rule, db_create_grou }, "enabled": True, } + response_status, _ = api_create_assign_rule(assign_rule_data) response_status, response_data = api_create_assign_rule(assign_rule_data) + assert_response_status(response_status, expected_status=400) assert assign_rule_data["name"] in response_data["detail"] -@pytest.mark.skip(reason="Need to validate why the test is creating duplicates in the DB") def test_create_assignemnt_rule_same_group(api_create_assign_rule, db_create_group): group = db_create_group("my_group") assign_rule_data = { @@ -100,7 +98,7 @@ def test_create_assignemnt_rule_same_group(api_create_assign_rule, db_create_gro response_status, _ = api_create_assign_rule(assign_rule_data) response_status, response_data = api_create_assign_rule(assign_rule_same_group) assert_response_status(response_status, expected_status=400) - assert group in response_data["detail"] + assert assign_rule_data["group_id"] in response_data["detail"] def test_create_assignment_rule_RBAC_denied(subtests, mocker, api_create_assign_rule, db_create_group, enable_rbac): diff --git a/tests/test_api_assignment_rules_get.py b/tests/test_api_assignment_rules_get.py index e70d03bad..5f1edb4de 100644 --- a/tests/test_api_assignment_rules_get.py +++ b/tests/test_api_assignment_rules_get.py @@ -6,11 +6,11 @@ def test_basic_assignment_rule_query(db_create_assignment_rule, db_create_group, api_get): - group = db_create_group("TestGroup") + groups = [db_create_group(f"TestGroup_{i}") for i in range(3)] filter = {"AND": [{"fqdn": {"eq": "foo.bar.com"}}]} assignment_rule_id_list = [ - str(db_create_assignment_rule(f"assignment {idx}", group.id, filter, True).id) for idx in range(3) + str(db_create_assignment_rule(f"assignment {idx}", groups[idx].id, filter, True).id) for idx in range(3) ] response_status, response_data = api_get(build_assignment_rules_url()) @@ -38,10 +38,10 @@ def test_get_assignment_rule_by_name(db_create_assignment_rule, db_create_group, def test_get_assignment_rule_with_bad_name(db_create_assignment_rule, db_create_group, api_get): - group = db_create_group("TestGroup") + groups = [db_create_group(f"TestGroup_{i}") for i in range(3)] filter = {"AND": [{"fqdn": {"eq": "foo.bar.com"}}]} - _ = [db_create_assignment_rule(f"assignment {idx}", group.id, filter, True).id for idx in range(3)] + _ = [db_create_assignment_rule(f"assignment {idx}", groups[idx].id, filter, True).id for idx in range(3)] response_status, response_data = api_get(build_assignment_rules_url()) assert response_status == 200 @@ -66,10 +66,10 @@ def test_get_assignment_rule_with_bad_name(db_create_assignment_rule, db_create_ ), ) def test_order_by_and_order_how(db_create_assignment_rule, db_create_group, api_get, order_by, order_how): - group = db_create_group("TestGroup") + groups = [db_create_group(f"TestGroup_{i}") for i in range(3)] filter = {"AND": [{"fqdn": {"eq": "foo.bar.com"}}]} - _ = [db_create_assignment_rule(f"assignment {idx}", group.id, filter, True) for idx in range(3)] + _ = [db_create_assignment_rule(f"assignment {idx}", groups[idx].id, filter, True).id for idx in range(3)] _, response_data = api_get(build_assignment_rules_url()) assert len(response_data["results"]) == 3 diff --git a/tests/test_models.py b/tests/test_models.py index 8e1f3ed69..567381794 100644 --- a/tests/test_models.py +++ b/tests/test_models.py @@ -653,7 +653,7 @@ def test_create_assignment_rule(db_create_group, db_create_assignment_rule, db_g group = db_create_group(group_name) ar = db_create_assignment_rule("default assignment", group.id, {"AND": [{"fqdn": {"eq": "foo.bar.com"}}]}, True) - assert db_get_assignment_rule(ar.id) + assert db_get_assignment_rule(ar.id, group.id) def test_delete_assignment_rule( @@ -663,10 +663,10 @@ def test_delete_assignment_rule( group = db_create_group(group_name) ar = db_create_assignment_rule("default assignment", group.id, {"AND": [{"fqdn": {"eq": "foo.bar.com"}}]}, True) - assert db_get_assignment_rule(ar.id) + assert db_get_assignment_rule(ar.id, group.id) db_delete_assignment_rule(ar.id) - assert not db_get_assignment_rule(ar.id) + assert not db_get_assignment_rule(ar.id, group.id) def test_create_default_account_staleness_culling( From 4e9043237e52cd712fa779099c60d90555284b78 Mon Sep 17 00:00:00 2001 From: Joao Paulo Ramos Date: Mon, 31 Jul 2023 13:46:05 -0300 Subject: [PATCH 3/4] Apply suggestions from code review Co-authored-by: Muhammad Arif <40067075+thearifismail@users.noreply.github.com> --- tests/test_api_assignment_rules_create.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_api_assignment_rules_create.py b/tests/test_api_assignment_rules_create.py index 19f8fa537..d1c1d1b48 100644 --- a/tests/test_api_assignment_rules_create.py +++ b/tests/test_api_assignment_rules_create.py @@ -98,7 +98,7 @@ def test_create_assignemnt_rule_same_group(api_create_assign_rule, db_create_gro response_status, _ = api_create_assign_rule(assign_rule_data) response_status, response_data = api_create_assign_rule(assign_rule_same_group) assert_response_status(response_status, expected_status=400) - assert assign_rule_data["group_id"] in response_data["detail"] + f'{assign_rule_data["group_id"]} already exist' in response_data["detail"] def test_create_assignment_rule_RBAC_denied(subtests, mocker, api_create_assign_rule, db_create_group, enable_rbac): From 8cb84707e5a08a9a8d191c4210b3ae78f55ba73d Mon Sep 17 00:00:00 2001 From: Joao Paulo Ramos Date: Tue, 1 Aug 2023 11:20:05 -0300 Subject: [PATCH 4/4] Update tests/test_api_assignment_rules_create.py Co-authored-by: Asa Price --- tests/test_api_assignment_rules_create.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_api_assignment_rules_create.py b/tests/test_api_assignment_rules_create.py index d1c1d1b48..4ceb00178 100644 --- a/tests/test_api_assignment_rules_create.py +++ b/tests/test_api_assignment_rules_create.py @@ -98,7 +98,7 @@ def test_create_assignemnt_rule_same_group(api_create_assign_rule, db_create_gro response_status, _ = api_create_assign_rule(assign_rule_data) response_status, response_data = api_create_assign_rule(assign_rule_same_group) assert_response_status(response_status, expected_status=400) - f'{assign_rule_data["group_id"]} already exist' in response_data["detail"] + assert f'{assign_rule_data["group_id"]} already exist' in response_data["detail"] def test_create_assignment_rule_RBAC_denied(subtests, mocker, api_create_assign_rule, db_create_group, enable_rbac):