feat(RHINENG-25223): Update workloads filtering logic to use OR operator#3839
Open
computercamplove wants to merge 13 commits intomasterfrom
Open
feat(RHINENG-25223): Update workloads filtering logic to use OR operator#3839computercamplove wants to merge 13 commits intomasterfrom
computercamplove wants to merge 13 commits intomasterfrom
Conversation
Contributor
SC Environment Impact AssessmentOverall Impact: ⚪ NONE No SC Environment-specific impacts detected in this PR. What was checkedThis PR was automatically scanned for:
|
Contributor
Author
|
WIP - need to update IQE tests |
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- Given that
_build_workloads_filteris now used for both workload and non-workload fields (via_process_standard_group), consider renaming it (and possibly moving it) to better reflect its broader purpose and avoid confusion for future maintainers.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Given that `_build_workloads_filter` is now used for both workload and non-workload fields (via `_process_standard_group`), consider renaming it (and possibly moving it) to better reflect its broader purpose and avoid confusion for future maintainers.
## Individual Comments
### Comment 1
<location path="tests/test_api_hosts_get.py" line_range="2792-2801" />
<code_context>
+def test_query_multiple_workloads_uses_or_logic(db_create_host, api_get, sp_filter_param):
</code_context>
<issue_to_address>
**issue (testing):** Add a case where a single host matches multiple workloads to verify deduplication and semantics.
The current test confirms OR semantics across the three workloads and excludes unrelated workloads. To strengthen it, please add a case where a single host satisfies at least two workload predicates (e.g., `sap_system=True` and `ansible.controller_version=1.2.3`) and assert that:
- The host ID appears only once in `response_ids`, and
- The total result count reflects deduplication.
This will validate both the OR logic and that multi-matching hosts don’t produce duplicate results.
</issue_to_address>
### Comment 2
<location path="tests/test_api_hosts_get.py" line_range="2772-2781" />
<code_context>
+@pytest.mark.parametrize(
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a negative test for multiple workload filters that should return an empty result set.
Right now all `sp_filter_param` cases ensure at least one host matches, which validates the OR behavior. Please add one parametrized case where valid workload filters collectively match no hosts (e.g., a `sap_sids` and `ansible.controller_version` that don’t exist on any created host), and assert a 200 response with an empty `results` list. This protects against regressions where the OR/AND logic returns all hosts or errors instead of an empty set.
Suggested implementation:
```python
response_status, response_data = api_get(url)
assert response_status == 400
assert "Param filter must be appended with [] to accept multiple values." in response_data["detail"]
@pytest.mark.parametrize(
"sp_filter_param",
(
(
"[workloads][sap][sids][contains][]=NONEXISTENT_SID"
"&filter[system_profile][workloads][sap][sap_system][]=true"
"&filter[system_profile][workloads][ansible][controller_version][]=9.9.9"
),
),
)
def test_get_hosts_sp_workload_filters_no_matches(api_get, sp_filter_param):
url = build_hosts_url(f"?filter[system_profile]{sp_filter_param}")
response_status, response_data = api_get(url)
assert response_status == 200
assert response_data["results"] == []
@pytest.mark.parametrize(
```
1. This new test assumes there is an existing `build_hosts_url` helper (or equivalent) used elsewhere in this file to construct the hosts URL. If the helper is named differently, update the call in `test_get_hosts_sp_workload_filters_no_matches` accordingly.
2. The specific workload filters (`NONEXISTENT_SID` and `9.9.9`) should be chosen such that they do not match any of the hosts created in the fixtures for this test module. If test data fixtures include those values, adjust the filter values to something guaranteed not to exist.
3. If the API response schema differs (e.g., results are nested or paginated differently), adjust the final assertion to check the correct path to the hosts list, ensuring it asserts that the result set is empty while status is 200.
</issue_to_address>
### Comment 3
<location path="api/filtering/db_custom_filters.py" line_range="398" />
<code_context>
+ return field_name in WORKLOADS_FIELDS or field_name == "workloads"
+
+
+def _process_workload_group(grouped_filter_param):
+ """Workloads always use OR conjunction for multiple values."""
+ if isinstance(grouped_filter_param, list):
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the two separate workload/standard group-processing helpers with a single helper parameterized by `is_workload` to unify the control flow and reduce duplication.
You can simplify the new logic by collapsing the two thin helpers into a single helper that takes `is_workload` as a parameter, so the control flow stays in one place while preserving the new workload-grouping behavior.
For example:
```python
def _build_group_filter(grouped_filter_param, *, is_workload: bool):
if isinstance(grouped_filter_param, list):
if is_workload:
# Workloads: OR across values
return or_(*(_build_workloads_filter(f) for f in grouped_filter_param))
# Standard fields: AND for arrays, OR otherwise
field_filter = _get_field_filter_for_deepest_param(
system_profile_spec(), grouped_filter_param[0]
)
conjunction = and_ if field_filter == "array" else or_
return conjunction(_build_workloads_filter(f) for f in grouped_filter_param)
# Single filter object: workloads and standard share the same builder
return _build_workloads_filter(grouped_filter_param)
```
Then `build_system_profile_filter` can express the full behavior in one coherent loop, while still separating workload filters for a final OR-group:
```python
def build_system_profile_filter(system_profile_param: dict) -> tuple:
standard_filters = []
workload_filters = []
filter_param_list = _unique_paths(system_profile_param, ["operating_system"])
for grouped_filter_param in filter_param_list:
is_workload = _is_workload_filter(grouped_filter_param)
group_filter = _build_group_filter(grouped_filter_param, is_workload=is_workload)
if is_workload:
workload_filters.append(group_filter)
else:
standard_filters.append(group_filter)
system_profile_filter = tuple(standard_filters)
if workload_filters:
system_profile_filter += (or_(*workload_filters),)
return system_profile_filter
```
This keeps:
- The new semantics of grouping all workload filters into a single `or_(*workload_filters)` term.
- The existing rule “arrays use AND, everything else uses OR” for non-workload fields.
But it removes duplicated branching on `isinstance(..., list)` and the extra `_process_workload_group` / `_process_standard_group` indirection, making the control flow easier to follow.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| "[workloads][sap][sids][contains][]=ABC&filter[system_profile][workloads][sap][sids][contains][]=GHI", | ||
| "[sap][sids][contains][]=ABC&filter[system_profile][sap][sids][contains][]=GHI", | ||
| "[workloads][sap][sids][contains][]=HIJ&filter[system_profile][workloads][sap][sids][contains][]=GHI", | ||
| "[sap][sids][contains][]=HIJ&filter[system_profile][sap][sids][contains][]=GHI", |
Contributor
Author
There was a problem hiding this comment.
updated the 'not found' tests to use non-existent workload SAP SID value - because the filtering logic was refactored from AND to OR
9368e67 to
6d774b0
Compare
f0d5ee1 to
5771612
Compare
added 2 commits
March 30, 2026 11:39
2168899 to
7116c28
Compare
Contributor
Author
|
/retest |
3 similar comments
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
Contributor
Author
|
/retest |
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
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-25223.
Currently, the backend implementation for this filter uses AND logic. This task requires updating the backend to use OR logic to ensure the Workload filter behaves consistently with all other existing Inventory filters (e.g., System Type, Status, Tags, and Data Collector)
PR Checklist
Secure Coding Practices Documentation Reference
You can find documentation on this checklist here.
Secure Coding Checklist
Summary by Sourcery
Adjust system profile workload filtering to use OR logic across multiple workload criteria while keeping standard filters behavior unchanged.
New Features:
Bug Fixes:
Enhancements:
Tests: