fix(RHINENG-24563): Implement client-side sorting for RBAC v2 (updated, created, name) since RBAC API doesn't support order_how#3791
Closed
thearifismail wants to merge 4 commits intomasterfrom
Closed
fix(RHINENG-24563): Implement client-side sorting for RBAC v2 (updated, created, name) since RBAC API doesn't support order_how#3791thearifismail wants to merge 4 commits intomasterfrom
thearifismail wants to merge 4 commits intomasterfrom
Conversation
…d, created, name) since RBAC API doesn't support order_how
Contributor
SC Environment Impact AssessmentOverall Impact: 🟢 LOW View full reportSummary
Detailed Findings🟢 LOW ImpactFeature flag change detected
Required Actions
This assessment was automatically generated. Please review carefully and consult with the ROSA Core team for critical/high impact changes. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The client-side sorting branch maps
order_by == "type"to the"ungrouped"boolean field, which may be surprising for callers expecting sorting by the RBAC workspacetype; consider either sorting by the actual type value or adding a clear comment explaining that a request fortypeintentionally sorts byungroupedinstead. - In the client-side sorting path you perform two consecutive
sort()calls to get a secondary key onname; this could be simplified and made more explicit by using a single sort with a composite key (e.g.,key=lambda g: (g.get(sort_field), g.get("name", ""))) instead of relying on sort stability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The client-side sorting branch maps `order_by == "type"` to the `"ungrouped"` boolean field, which may be surprising for callers expecting sorting by the RBAC workspace `type`; consider either sorting by the actual type value or adding a clear comment explaining that a request for `type` intentionally sorts by `ungrouped` instead.
- In the client-side sorting path you perform two consecutive `sort()` calls to get a secondary key on `name`; this could be simplified and made more explicit by using a single sort with a composite key (e.g., `key=lambda g: (g.get(sort_field), g.get("name", ""))`) instead of relying on sort stability.
## Individual Comments
### Comment 1
<location path="api/group.py" line_range="240-249" />
<code_context>
+ # Fetch all groups, sort in Python, then paginate
+
+ # Step 1: Fetch all groups from RBAC v2 (up to limit)
+ group_list, total = get_rbac_workspaces(
+ name, 1, MAX_GROUPS_FOR_HOST_COUNT_SORTING, group_type, None, None
+ )
+
+ # Validate that we can sort all groups
+ if total > MAX_GROUPS_FOR_HOST_COUNT_SORTING:
+ log_get_group_list_failed(logger)
+ abort(
+ 400,
+ f"Cannot sort by {order_by}: organization has {total} groups, which exceeds "
+ f"the maximum of {MAX_GROUPS_FOR_HOST_COUNT_SORTING} groups that can be sorted. "
+ f"Please use filters (name, group_type) to narrow your results.",
+ )
+
</code_context>
<issue_to_address>
**suggestion:** Decouple the host-count-specific limit from the generic client-side sorting limit.
`MAX_GROUPS_FOR_HOST_COUNT_SORTING` now acts as a global cap for all client-side sorting (e.g., `updated`, `created`, `name`, `type`), not just host-count-related behavior. This couples a host-count-specific constant to general sorting. Consider introducing a generic limit (e.g., `MAX_GROUPS_FOR_CLIENT_SIDE_SORTING`) or renaming the existing constant to reflect its broader scope so host-count-specific changes don’t unintentionally affect other sorts.
Suggested implementation:
```python
group_list, total = get_rbac_workspaces(
name, 1, MAX_GROUPS_FOR_CLIENT_SIDE_SORTING, group_type, None, None
)
```
1. Define a new constant `MAX_GROUPS_FOR_CLIENT_SIDE_SORTING` in this module (or the shared config/constants module where `MAX_GROUPS_FOR_HOST_COUNT_SORTING` is defined) with the desired generic limit value.
2. Add/adjust any necessary imports so `MAX_GROUPS_FOR_CLIENT_SIDE_SORTING` is available in `api/group.py`.
3. Leave `MAX_GROUPS_FOR_HOST_COUNT_SORTING` in place for host-count-specific logic only, ensuring no other non-host-count code paths reference it.
</issue_to_address>
### Comment 2
<location path="tests/test_api_groups_get.py" line_range="709-718" />
<code_context>
+def test_get_groups_rbac_v2_smart_defaults_for_updated(mocker, api_get):
</code_context>
<issue_to_address>
**suggestion (testing):** Extend smart‑default ordering test to cover other sortable fields and explicit order_how overrides.
To better exercise the client-side sorting logic and the new `default_order_how` mapping, please also cover:
- `order_by=created` with no `order_how` (default DESC)
- `order_by=name` with no `order_how` (default ASC)
- `order_by=type` with no `order_how` (default ASC)
- Each of `updated`, `created`, `name`, `type` with `order_how=ASC` and `order_how=DESC` explicitly set, to confirm the explicit value overrides the smart default.
You can either parametrize this test or add sibling tests to achieve this coverage.
Suggested implementation:
```python
import pytest
@pytest.mark.parametrize(
"order_by, order_how_param, expected_order_how",
[
# Smart defaults (no explicit order_how)
("updated", None, "DESC"), # "Last modified" default
("created", None, "DESC"), # "Created" default
("name", None, "ASC"), # "Name" default
("type", None, "ASC"), # "Type" default
# Explicit overrides
("updated", "ASC", "ASC"),
("updated", "DESC", "DESC"),
("created", "ASC", "ASC"),
("created", "DESC", "DESC"),
("name", "ASC", "ASC"),
("name", "DESC", "DESC"),
("type", "ASC", "ASC"),
("type", "DESC", "DESC"),
],
)
def test_get_groups_rbac_v2_smart_defaults(
mocker, api_get, order_by, order_how_param, expected_order_how
):
"""
Test GET /groups with RBAC v2 applies client-side smart defaults and honors explicit
`order_how` overrides for all sortable fields.
Since RBAC v2 API doesn't support `order_how` parameter, host-inventory implements
client-side sorting. When users click a sortable column without explicitly specifying
`order_how`, they expect the "smart default" for that column. When `order_how` is
explicitly provided, it must override the smart default.
This test verifies that client-side sorting applies the correct smart defaults for
`updated`, `created`, `name`, and `type`, and that explicit `order_how` values are
honored, to match RBAC v1 behavior.
"""
# Patch the RBAC v2 workspace-fetching function so we can inspect how it is called.
# NOTE: keep this target consistent with the rest of this test module.
get_rbac_workspaces = mocker.patch(
"app.api.groups.get_rbac_workspaces", return_value=([], 0)
)
# Build query params based on the parametrized values.
query_params = {"order_by": order_by}
if order_how_param is not None:
query_params["order_how"] = order_how_param
# Act: call the groups endpoint using RBAC v2.
response = api_get("/api/inventory/v1/groups", query_string=query_params)
# Sanity check the request succeeded.
assert response.status_code == 200
# Verify that get_rbac_workspaces was called once with the expected order_by/order_how.
get_rbac_workspaces.assert_called_once()
call_args, _ = get_rbac_workspaces.call_args
# For RBAC v2, the function is called with positional args; index 4 is order_by
# and index 5 is order_how, as asserted in the other client-side sorting tests.
assert call_args[4] == order_by
assert call_args[5] == expected_order_how
```
To make this work with your existing test suite you should:
1. Adjust the patch target `"app.api.groups.get_rbac_workspaces"` to match whatever target you are *already* using in this file when asserting `call_args[0][4]` / `call_args[0][5]` for RBAC v2; reusing that same import path keeps tests consistent.
2. Confirm the URL `"/api/inventory/v1/groups"` matches the rest of this file; if your helper `api_get` already hard‑codes the base path or a different route, update the URL here to match.
3. If `pytest` is already imported at the top of the file, remove the duplicate `import pytest` in this block and keep just a single import.
4. If your existing `test_get_groups_rbac_v2_smart_defaults_for_updated` test contained additional setup/fixtures (e.g. enabling RBAC v2, seeding data, feature flags), move that setup into this parametrized test so behavior is identical for all parameter combinations.
</issue_to_address>
### Comment 3
<location path="tests/test_api_groups_get.py" line_range="768-773" />
<code_context>
+
+ assert_response_status(response_status, 200)
+
+ # Verify the results are in the expected order (DESC by default)
+ # If smart defaults weren't applied, results would be in ASC or random order
+ results = response_data["results"]
+ assert len(results) == 2
+ assert results[0]["name"] == "workspace-a" # Newer first (DESC order)
+ assert results[1]["name"] == "workspace-b" # Older second
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test that validates deterministic secondary sorting by name for ties.
The current test only verifies primary ordering (newer vs older). Please add a case where multiple workspaces share the same `updated` (or `created`) value but have different `name`s, and assert that within that tie they are ordered lexicographically by `name`. This makes the secondary sort behavior explicit and guards against regressions if the sorting logic changes.
Suggested implementation:
```python
query = "?order_by=updated"
response_status, response_data = api_get(build_groups_url(query=query))
assert_response_status(response_status, 200)
# Verify deterministic secondary sorting by name when `updated` timestamps are equal.
# We reconfigure the mock so that all returned workspaces share the same `updated` value
# but have different `name`s; the API should order them lexicographically by `name`.
tied_updated = "2024-01-01T00:00:00+00:00"
mock_tied_workspaces = [
{
"id": "ws-1",
"name": "workspace-a",
"updated": tied_updated,
},
{
"id": "ws-2",
"name": "workspace-b",
"updated": tied_updated,
},
{
"id": "ws-3",
"name": "workspace-c",
"updated": tied_updated,
},
]
mock_get_rbac_workspaces.return_value = (mock_tied_workspaces, len(mock_tied_workspaces))
response_status, response_data = api_get(build_groups_url(query=query))
assert_response_status(response_status, 200)
tied_results = response_data["results"]
# Within the tie on `updated`, results must be ordered lexicographically by `name`
assert [w["name"] for w in tied_results] == sorted([w["name"] for w in mock_tied_workspaces])
```
Depending on the rest of the test suite and the shape of `workspace` objects expected by the API code, you may need to:
1. Add any required fields (e.g. `created`, `display_name`, `host_count`, etc.) to each dict in `mock_tied_workspaces` so the endpoint logic does not fail when accessing them.
2. Ensure this block is inside the same test that verifies ordering by `updated` (the one whose `query` is `"?order_by=updated"`). If the function name implies it should only be testing part of this behavior, consider extracting the new assertions into a dedicated test instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
thearifismail
changed the title
Contributor
|
👋 Hi, this PR has been inactive for 7 days. |
Contributor
|
This PR has been automatically closed due to 15 days of inactivity. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR is being created to address RHINENG-24563.
This PR fixes the order_how issue which has been a supported case because host-inventory used to pull groups data from its database using SQL queries. Now RBAC V2 does not support sorting by
order_how.@computercamplove support "order_how" is not trivial. What makes me uncomfortable is the size of the
get_group_list()function, which was already too long. I thought I had created a JIRA card to refactor this function in future.If possible I would like to fix this issue later, what do you think?
PR Checklist
Secure Coding Practices Documentation Reference
You can find documentation on this checklist here.
Secure Coding Checklist
Summary by Sourcery
Implement client-side sorting for RBAC v2 group listings when ordering by updated, created, name, or type, with smart defaults to mirror previous RBAC v1 behavior.
Bug Fixes:
Enhancements:
Tests: