chore: replace old queries due to cyndi migration(tag fiels)#2239
chore: replace old queries due to cyndi migration(tag fiels)#2239RostyslavKachan wants to merge 2 commits intoRedHatInsights:masterfrom
Conversation
Reviewer's GuideThis PR updates CVE and system queries to use the new Cyndi-backed SystemTagSet tags (with a feature flag fallback to InventoryHosts.tags), and wires tag-based filters and joins to the new tag source when the CYNDI_REPLICATION_REPLACEMENT Unleash flag is enabled. Updated class diagram for query models and handlers using SystemTagSetclassDiagram
class UNLEASH {
+is_enabled(flag_name)
}
class CYNDI_REPLICATION_REPLACEMENT
class InventoryHosts {
+tags
+updated
+insights_id
}
class SystemTagSet {
+tags
}
class SystemPlatform {
+culled_timestamp
+cve_count_cache
+opt_out
+host_type
+display_name
+last_upload
+stale
}
class SystemGroupSet {
+groups
}
class SystemVulnerabilities {
+status_id
+rule_id
+mitigation_reason
+remediation_type_id
}
class InsightsRule {
+description
+reason
+resolution
+reboot_required
+resolution_text
+kbase_node_id
+more_info_text
}
class CveHandler {
-_full_query(rh_account_id, synopsis, parsed_args, filters, remediation_filter)
-_unpatched_full_query(rh_account_id, synopsis, parsed_args, filters)
-_id_query(rh_account_id, synopsis, parsed_args, filters, remediation_filter)
-_unpatched_id_query(rh_account_id, synopsis, parsed_args, filters)
}
class SystemHandler {
-_full_query(rh_account_id)
+handle_get(kwargs)
}
class Filters {
-_filter_system_by_tags(query, args, kwargs)
}
class BaseManager {
+cyndi_join(query)
}
UNLEASH --> CYNDI_REPLICATION_REPLACEMENT : checks
CveHandler --> InventoryHosts : selects
CveHandler --> SystemTagSet : selects when flag enabled
CveHandler --> SystemPlatform : joins
CveHandler --> SystemVulnerabilities : joins
CveHandler --> InsightsRule : joins
SystemHandler --> InventoryHosts : selects
SystemHandler --> SystemTagSet : selects when flag enabled
SystemHandler --> SystemPlatform : joins
Filters --> InventoryHosts : tags filter when flag disabled
Filters --> SystemTagSet : tags filter when flag enabled
BaseManager --> SystemPlatform : switch for joins
BaseManager --> SystemGroupSet : joins when flag enabled
BaseManager --> SystemTagSet : joins when flag enabled
BaseManager --> InventoryHosts : legacy tag source when flag disabled
Flow diagram for tag source selection via CYNDI_REPLICATION_REPLACEMENTflowchart TD
A["Build query needing tags"] --> B["Check UNLEASH.is_enabled(CYNDI_REPLICATION_REPLACEMENT)"]
B -->|"true"| C["Use SystemTagSet.tags with COALESCE(SystemTagSet.tags, '[]') as tags"]
B -->|"false"| D["Use InventoryHosts.tags as tags"]
C --> E["Return query with tags column from SystemTagSet"]
D --> E["Return query with tags column from InventoryHosts"]
Flow diagram for tag filtering with CYNDI_REPLICATION_REPLACEMENTflowchart TD
A["_filter_system_by_tags called"] --> B["Are tags provided in args?"]
B -->|"no"| G["Return original query"]
B -->|"yes"| C["Iterate over each tag in args['tags']"]
C --> D["Check UNLEASH.is_enabled(CYNDI_REPLICATION_REPLACEMENT)"]
D -->|"true"| E["Add where(SystemTagSet.tags.contains([tag]))"]
D -->|"false"| F["Add where(InventoryHosts.tags.contains([tag]))"]
E --> H["Next tag or finish"]
F --> H["Next tag or finish"]
H --> I["Return filtered query"]
Flow diagram for cyndi_join behavior with CYNDI_REPLICATION_REPLACEMENTflowchart TD
A["cyndi_join(query)"] --> B["Check UNLEASH.is_enabled(CYNDI_REPLICATION_REPLACEMENT)"]
B -->|"true"| C["query.switch(SystemPlatform).join(SystemGroupSet, LEFT_OUTER)"]
C --> D["query.switch(SystemPlatform).join(SystemTagSet, LEFT_OUTER)"]
D --> E["filter_allowed_groups(query, SystemGroupSet.groups)"]
E --> F["filter_kessel_workspace_opt_out(query, SystemGroupSet.groups)"]
F --> G["Return joined query (Cyndi-backed groups and tags)"]
B -->|"false"| H["Legacy join path using InventoryHosts.tags (unchanged by this PR)"]
H --> I["Return legacy joined query"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
SC Environment Impact AssessmentOverall Impact: 🟢 LOW View full reportSummary
Detailed Findings🟢 LOW ImpactFeature flag change detected
Feature flag change detected
Feature 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. |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The conditional selection between
SystemTagSet.tagsandInventoryHosts.tagsis duplicated in multiple queries; consider extracting a small helper (or column expression factory) to centralize this logic and avoid divergence in future changes. - Now that
SystemTagSetis used in filters and selects, double-check that every query path usingSystemTagSet.tagsalways goes throughcyndi_join(or otherwise joinsSystemTagSet) to prevent runtime errors from missing joins.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The conditional selection between `SystemTagSet.tags` and `InventoryHosts.tags` is duplicated in multiple queries; consider extracting a small helper (or column expression factory) to centralize this logic and avoid divergence in future changes.
- Now that `SystemTagSet` is used in filters and selects, double-check that every query path using `SystemTagSet.tags` always goes through `cyndi_join` (or otherwise joins `SystemTagSet`) to prevent runtime errors from missing joins.
## Individual Comments
### Comment 1
<location path="manager/filters.py" line_range="358-357" />
<code_context>
if UNLEASH.is_enabled(CYNDI_REPLICATION_REPLACEMENT):
query = (query.switch(SystemPlatform).join(SystemGroupSet, JOIN.LEFT_OUTER))
+ query = (query.switch(SystemPlatform).join(SystemTagSet, JOIN.LEFT_OUTER))
</code_context>
<issue_to_address>
**suggestion (performance):** Move the feature-flag check outside the tag loop to avoid repeated evaluation.
`UNLEASH.is_enabled(CYNDI_REPLICATION_REPLACEMENT)` is currently evaluated once per tag in `args["tags"]`, which can cause many redundant feature-flag checks for large tag lists. Compute a single `use_system_tag_set` (or similar) boolean before the loop and branch on it once, then apply the `contains` condition inside the loop using the selected column.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
0469e99 to
b9bc8a3
Compare
b9bc8a3 to
98b4351
Compare
98b4351 to
09b716a
Compare
manager/base.py
Outdated
|
|
||
| def tag_set_join(query): | ||
| """Join SystemTagSet for tag data.""" | ||
| return query.switch(SystemPlatform).join(SystemTagSet, JOIN.LEFT_OUTER) |
There was a problem hiding this comment.
Now without the condition, what's a purpose of oneline function? just join it directly
09b716a to
a485d34
Compare
… mssql fiels) RHINENG-23543
e54d337 to
6ed8e90
Compare
RHINENG-23542
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Update system and CVE queries to use the new SystemTagSet tags source when the Cyndi replication replacement feature flag is enabled.
Enhancements: