refactor(RHINENG-5484): refactor operating_system filter #2141

FabriciaDinizRH · 2024-12-18T19:21:38Z

Overview

This PR is being created to address RHINENG-5484.

PR Checklist

Secure Coding Practices Documentation Reference

You can find documentation on this checklist here.

Secure Coding Checklist

FabriciaDinizRH · 2024-12-18T20:04:08Z

I added some commits I was not supposed to, I'm fixing it

kruai · 2024-12-18T21:27:03Z

api/filtering/db_custom_filters.py

+    for os_name in filter_param.keys(): # this doesn't account for "os_name" instead of os names
        if os_name not in (os_names := _get_valid_os_names()):


I'd recommend checking for the key "name" between these 2 lines and doing the custom logic there:

Suggested change

for os_name in filter_param.keys(): # this doesn't account for "os_name" instead of os names

if os_name not in (os_names := _get_valid_os_names()):

for os_name in filter_param.keys():

if os_name == "name":

# Return an OsComparison object that implies all hosts with a matching OS name.

# Maybe if OsComparison.comparator is "eq" or "neq" and the major version is None?

elif os_name not in (os_names := _get_valid_os_names()):

kruai · 2024-12-18T21:50:33Z

api/filtering/db_custom_filters.py

@@ -158,6 +169,7 @@ def build_operating_system_filter(filter_param: dict) -> tuple:
            os_filter_list.append(os_field.astext.operate(comparator, None))

        elif comparison.comparator == "eq":
+            print("~~~~~~~~~~~~~~~~~~~``", os_field["name"].astext == comparison.name)


Around here is where you'd convert the OsComparison to an actual DB operation. If we went with the logic I posted above, this next section could be something like this:

Suggested change

print("~~~~~~~~~~~~~~~~~~~``", os_field["name"].astext == comparison.name)

os_filters = [

os_field["name"].astext == comparison.name,

]

if comparison.major is not None:

os_filters.append(os_field["major"].astext.cast(Integer) == comparison.major)

if comparison.minor:

os_filters.append(os_field["minor"].astext.cast(Integer) == comparison.minor)

os_filter_list.append(and_(*os_filters))

Of course, you'll need to handle "neq" as well :)

kruai

Mostly looking good, got a few suggestions. Also, reminder that we'll need to update the documentation as well so people know how to use the filter in this way.

kruai · 2025-01-07T14:05:22Z

api/filtering/db_custom_filters.py

+            # case insensitive
+            if real_os_name.lower() not in (os_names := [name.lower() for name in _get_valid_os_names()]):
+                raise ValidationException(f"operating_system filter only supports these OS names: {os_names}.")
+            return [OsComparison(real_os_name, comparator, None)]


Adding a return statement here will make it so other provided OS filters are ignored. For instance, if the request happens to be ?filter[system_profile][operating_system][name][eq]=RHEL&filter[system_profile][operating_system][RHEL][version][gte]=8, it will no longer filter on the major version. Instead, the comparison should be appended to os_filter_list.

@kruai I guess I need to adjust the logic a little bit or maybe ?filter[system_profile][operating_system][name][eq]=RHEL&filter[system_profile][operating_system][RHEL][version][gte]=8 doesn't make sense. I'm saying this because the query came out like this:

AND (lower(((hbi.hosts.system_profile_facts["operating_system"]) ->> "name")) = "rhel" AND CAST(((hbi.hosts.system_profile_facts["operating_system"]) ->> "major") AS INTEGER) = "8" OR lower(((hbi.hosts.system_profile_facts["operating_system"]) ->> "name")) = "rhel")

If someone is looking for RHEL, and they want to specify a version, it's unnecessary to add filter[system_profile][operating_system][name][eq]=RHEL

this is with the return statement as it is. If I just append to the filter list the request returns 500

Yeah, it's for sure redundant and they shouldn't provide a query like that. But if they do, we should be able to handle it imo.
HTTP 500 just means there was an unhandled exception on our end; what does the exception say in the logs?

File "/home/fabriciadiniz/dev/insights-host-inventory/api/filtering/db_custom_filters.py", line 131, in separate_operating_system_filters if not isinstance(version_node := filter_param[os_name]["version"], dict): KeyError: 'version'

Makes sense - once you add the filter to the list, you don't want to run the rest of the code that generates the usual OS filter. You could just put the rest in an else, but I feel like we can also simplify it a bit. I'll play around with it a bit

@FabriciaDinizRH Suggestion for the code flow: FabriciaDinizRH#1
(Feel free to reject it or tweak as necessary ofc)

kruai · 2025-01-07T14:06:39Z

api/filtering/db_custom_filters.py

+            if real_os_name.lower() not in (os_names := [name.lower() for name in _get_valid_os_names()]):
+                raise ValidationException(f"operating_system filter only supports these OS names: {os_names}.")


DRY with the code directly below

for this I can either use pass instead of raising the validation in the first if and it will be caught in the second if (because "name" is not a supported OS name), or I can extract the raise ValidationException into a function and call it in both ifs. wdyt?

If using raise..., should there be a test asserting for this exception?

kruai · 2025-01-07T14:06:54Z

api/filtering/db_custom_filters.py

@@ -117,7 +118,15 @@ def separate_operating_system_filters(filter_param) -> list[OsComparison]:

    # filter_param is a dict
    for os_name in filter_param.keys():
-        if os_name not in (os_names := _get_valid_os_names()):
+        # [operating_system][name][eq]==real_os_name


Suggested change

# [operating_system][name][eq]==real_os_name

kruai · 2025-01-07T14:38:45Z

tests/test_api_hosts_get.py

+        ("[name][eq]=CentOS", True),
+        ("[name][eq]=centos", True),
+        ("[name][eq]=CENTOS", True),
+        ("[name][eq]=centos&filter[system_profile][operating_system][RHEL][version][eq][]=8", True),
+        ("[name][neq]=CENTOS", False),
+        ("[name][neq]=CentOS", False),


This could use another test to validate the scenario I mentioned above. For example, ?filter[system_profile][operating_system][name][eq]=RHEL&filter[system_profile][operating_system][RHEL][version][eq]=9 should not match either of the host data in this test

thearifismail · 2025-01-08T21:10:01Z

api/filtering/db_custom_filters.py

@@ -117,7 +118,15 @@ def separate_operating_system_filters(filter_param) -> list[OsComparison]:

    # filter_param is a dict
    for os_name in filter_param.keys():
-        if os_name not in (os_names := _get_valid_os_names()):
+        # [operating_system][name][eq]==real_os_name
+        if os_name == "name":


rename the variable os_name to key, which is what is being checked. A name like os_name causes confusion when a key is being checked NOT its value.

thearifismail · 2025-01-08T21:12:31Z

api/filtering/db_custom_filters.py

+            if real_os_name.lower() not in (os_names := [name.lower() for name in _get_valid_os_names()]):
+                raise ValidationException(f"operating_system filter only supports these OS names: {os_names}.")


If using raise..., should there be a test asserting for this exception?

OS filter suggestion

kruai · 2025-01-09T14:17:13Z

/retest

thearifismail

LGTM

FabriciaDinizRH force-pushed the refactor-host-list branch from e870dce to fde51a7 Compare December 18, 2024 20:01

refactor(RHINENG-5484): refactor operating_system filter

4f60130

FabriciaDinizRH force-pushed the refactor-host-list branch from fde51a7 to 4f60130 Compare December 18, 2024 20:17

kruai reviewed Dec 18, 2024

View reviewed changes

refactor(RHINENG-5484): refactor operating_system filter

d58cfc1

FabriciaDinizRH force-pushed the refactor-host-list branch from 0a82b53 to d58cfc1 Compare January 6, 2025 20:43

FabriciaDinizRH requested a review from kruai January 6, 2025 20:43

FabriciaDinizRH marked this pull request as ready for review January 6, 2025 20:44

FabriciaDinizRH requested a review from a team as a code owner January 6, 2025 20:44

FabriciaDinizRH and others added 4 commits January 6, 2025 20:44

Merge branch 'master' into refactor-host-list

75bab2e

refactor(RHINENG-5484): refactor operating_system filter

9614da3

Merge branch 'master' into refactor-host-list

325e8b1

Merge branch 'master' into refactor-host-list

a8f4bf0

kruai requested changes Jan 7, 2025

View reviewed changes

FabriciaDinizRH requested a review from kruai January 8, 2025 13:42

OS filter refactoring

435b6d1

thearifismail reviewed Jan 8, 2025

View reviewed changes

FabriciaDinizRH and others added 3 commits January 9, 2025 10:19

Merge branch 'master' into refactor-host-list

fd0a319

Merge pull request #1 from RedHatInsights/asa-suggestion-os-filter

60b38fd

OS filter suggestion

add test

deeb2d4

FabriciaDinizRH requested a review from thearifismail January 9, 2025 11:06

kruai previously approved these changes Jan 9, 2025

View reviewed changes

Merge branch 'master' into refactor-host-list

73d0fc2

FabriciaDinizRH dismissed kruai’s stale review via a781377 January 10, 2025 16:29

refactor OsComparison

1adf118

FabriciaDinizRH force-pushed the refactor-host-list branch from a781377 to 1adf118 Compare January 10, 2025 16:35

Merge branch 'master' into refactor-host-list

6360e96

thearifismail approved these changes Jan 10, 2025

View reviewed changes

Merge branch 'master' into refactor-host-list

f673a78

FabriciaDinizRH requested a review from kruai January 13, 2025 11:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(RHINENG-5484): refactor operating_system filter #2141

refactor(RHINENG-5484): refactor operating_system filter #2141

FabriciaDinizRH commented Dec 18, 2024 •

edited

Loading

FabriciaDinizRH commented Dec 18, 2024

kruai Dec 18, 2024

kruai Dec 18, 2024

kruai left a comment •

edited

Loading

kruai Jan 7, 2025

FabriciaDinizRH Jan 8, 2025 •

edited

Loading

kruai Jan 8, 2025

FabriciaDinizRH Jan 8, 2025 •

edited

Loading

kruai Jan 8, 2025

kruai Jan 8, 2025

FabriciaDinizRH Jan 9, 2025

kruai Jan 7, 2025

FabriciaDinizRH Jan 8, 2025

thearifismail Jan 8, 2025

kruai Jan 7, 2025

kruai Jan 7, 2025

thearifismail Jan 8, 2025

thearifismail Jan 8, 2025

kruai commented Jan 9, 2025

thearifismail left a comment

		for os_name in filter_param.keys(): # this doesn't account for "os_name" instead of os names
		if os_name not in (os_names := _get_valid_os_names()):

-            print("~~~~~~~~~~~~~~~~~~~``", os_field["name"].astext == comparison.name)
+            os_filters = [
+                os_field["name"].astext == comparison.name,
+            ]
+            if comparison.major is not None:
+                os_filters.append(os_field["major"].astext.cast(Integer) == comparison.major)
+                if comparison.minor:
+                    os_filters.append(os_field["minor"].astext.cast(Integer) == comparison.minor)
+            os_filter_list.append(and_(*os_filters))

		if real_os_name.lower() not in (os_names := [name.lower() for name in _get_valid_os_names()]):
		raise ValidationException(f"operating_system filter only supports these OS names: {os_names}.")

refactor(RHINENG-5484): refactor operating_system filter #2141

Are you sure you want to change the base?

refactor(RHINENG-5484): refactor operating_system filter #2141

Conversation

FabriciaDinizRH commented Dec 18, 2024 • edited Loading

Overview

PR Checklist

Secure Coding Practices Documentation Reference

Secure Coding Checklist

FabriciaDinizRH commented Dec 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kruai left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabriciaDinizRH Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabriciaDinizRH Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kruai commented Jan 9, 2025

thearifismail left a comment

Choose a reason for hiding this comment

FabriciaDinizRH commented Dec 18, 2024 •

edited

Loading

kruai left a comment •

edited

Loading

FabriciaDinizRH Jan 8, 2025 •

edited

Loading

FabriciaDinizRH Jan 8, 2025 •

edited

Loading