Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

[RHINENG-1284] Tests are failing in test_api_assignment_rules_create.py #1440

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+21 −19
Open

[RHINENG-1284] Tests are failing in test_api_assignment_rules_create.py #1440

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
110542c
fix(RHINENG-1284): Add constrains to AssignmentRule model
jpramos123 Jul 28, 2023
79d26b2
fix(RHINENG-1284): Fix tests
jpramos123 Jul 28, 2023
4e90432
Apply suggestions from code review
jpramos123 Jul 31, 2023
8cb8470
Update tests/test_api_assignment_rules_create.py
jpramos123 Aug 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions app/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

didn't this got removed because it broke the filtering?
https://redhat-internal.slack.com/archives/CQFKM031T/p1689276193983449?thread_ts=1689271922.019339&cid=CQFKM031T

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can remove it from the model.py file and create a new migration to remove this constraint from the column, or we can add it as this PR suggests and it will be aligned with the DB modeling we have today.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jpramos123 I was checking the migration files and since this change doesn't break the tests we can leave it as it is.

filter = db.Column(JSONB, nullable=False)
enabled = db.Column(db.Boolean(), default=True)
created_on = db.Column(db.DateTime(timezone=True), default=_time_now)
Expand Down
4 changes: 2 additions & 2 deletions tests/fixtures/db_fixtures.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Comment on lines +110 to +111
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this necessary? Isn't the assignment rule ID unique?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It led to a issue with multiple primary keys, and was not able to fetch the data using only the ID.
See the error below:

sqlalchemy.exc.InvalidRequestError: Incorrect number of values in identifier to formulate primary key for query.get(); primary key columns are 'assignment_rules.id','assignment_rules.group_id'

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah... so then my follow-up question is, why do we need the primary key to consist of both the ID and group ID, when the ID should already be unique and non-nullable?
If we make the primary key the combination of those two columns, we technically open up the possibility to having duplicate values for ID (as long as group_id is different)

Copy link
Contributor Author

@jpramos123 jpramos123 Aug 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

We do not hehe.

But it was initially created this way:
https://github.com/RedHatInsights/insights-host-inventory/pull/1377/files#diff-6b8bcd51a947a9eb1db19cd9157a27b5bea41a8191cca8ad7b2ee297b0a3e9f7R28

In this case, we can create a new migration to remove the constraint from this column. As we are already making sure group_id is unique: https://github.com/RedHatInsights/insights-host-inventory/pull/1418/files#diff-be43a8e7819671e5eca91faed55b391bf96c0a0149d54a213c8fe3bf3163447a

Thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

We have an idxassrulesorgid which should work to make org_id unique, but maybe what we needed was a unique constraint.

insights-host-inventory/migrations/versions/1689cda89252_add_assignment_rules_table.py

Line 38 in 85e95ce

op.create_index("idxassrulesorgid", "assignment_rules", ["org_id"])

Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally, I don't think it makes sense for the table's primary key to be a combination of unique IDs. If we already have an id field, that should be the primary key; otherwise it adds unnecessary complexity because you need both the assignment rule ID and its group ID in order to fetch the record, right?


return _db_get_assignment_rule

Expand Down
10 changes: 4 additions & 6 deletions tests/test_api_assignment_rules_create.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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",
Expand All @@ -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 = {
Expand All @@ -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 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):
Expand Down
12 changes: 6 additions & 6 deletions tests/test_api_assignment_rules_get.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())

Expand Down Expand Up @@ -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):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not an issue with your changes, but am I missing the point of this test? It doesn't appear to be testing any scenario related to having a bad name (and both responses return 200)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, it validates that 3 rules were created, make an api call with a name that will not be in the list, and validate the return is an empty list.

We do not trigger an error when fetching rules that are not in the DB

@thearifismail Can you add more context here?

Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

from what I read, the query.filter function doesn't return an error if no results were found

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
Expand All @@ -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
Expand Down
6 changes: 3 additions & 3 deletions tests/test_models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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(
Expand Down