fix: handle of NULL for groups#2237
Merged
jdobes merged 1 commit intoRedHatInsights:masterfrom Feb 24, 2026
Merged
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:
|
Reviewer's guide (collapsed on small PRs)Reviewer's GuideNormalize NULL system group values to an empty JSONB array ('[]') when reading from SystemGroupSet in CVE and system queries, ensuring consistent inventory_group data under the CYNDI_REPLICATION_REPLACEMENT feature flag. ER diagram for SystemGroupSet and InventoryHosts groups normalizationerDiagram
INVENTORY_HOSTS {
uuid id
jsonb groups
timestamp updated
uuid insights_id
}
SYSTEM_GROUP_SET {
uuid id
jsonb groups "nullable"
}
QUERY_RESULT {
jsonb inventory_group "never NULL when CYNDI_REPLICATION_REPLACEMENT is enabled"
}
INVENTORY_HOSTS ||--o{ SYSTEM_GROUP_SET : has_group_set
SYSTEM_GROUP_SET ||--o{ QUERY_RESULT : provides_normalized_groups
INVENTORY_HOSTS ||--o{ QUERY_RESULT : contributes_host_fields
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The
Case(None, ((SystemGroupSet.groups.is_null(True), SQL("'[]'::jsonb")),), SystemGroupSet.groups)expression is duplicated in multiple queries; consider extracting this into a small helper or constant to reduce repetition and keep theinventory_grouphandling consistent across call sites. - Instead of manually building JSONB with
SQL("'[]'::jsonb"), consider using a higher-level construct (e.g.,fn.CAST('[]', 'jsonb')or a framework-native JSON/JSONB literal) if available, to avoid raw SQL and make the intent clearer. - Double-check whether
SystemGroupSet.groups.is_null(True)is the idiomatic way in this query builder to expressIS NULL; if there is a clearer or more conventional null-check helper, using it would improve readability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `Case(None, ((SystemGroupSet.groups.is_null(True), SQL("'[]'::jsonb")),), SystemGroupSet.groups)` expression is duplicated in multiple queries; consider extracting this into a small helper or constant to reduce repetition and keep the `inventory_group` handling consistent across call sites.
- Instead of manually building JSONB with `SQL("'[]'::jsonb")`, consider using a higher-level construct (e.g., `fn.CAST('[]', 'jsonb')` or a framework-native JSON/JSONB literal) if available, to avoid raw SQL and make the intent clearer.
- Double-check whether `SystemGroupSet.groups.is_null(True)` is the idiomatic way in this query builder to express `IS NULL`; if there is a clearer or more conventional null-check helper, using it would improve readability.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
jdobes
reviewed
Feb 24, 2026
manager/cve_handler.py
Outdated
| InventoryHosts.insights_id, | ||
| ( | ||
| SystemGroupSet.groups.alias("inventory_group") | ||
| Case(None, ((SystemGroupSet.groups.is_null(True), SQL("'[]'::jsonb")),), SystemGroupSet.groups).alias("inventory_group") |
Member
There was a problem hiding this comment.
This is exact case for coalesce, it'll be shorter and more readable.
3837668 to
e6ecaa5
Compare
jdobes
reviewed
Feb 24, 2026
Member
jdobes
left a comment
There was a problem hiding this comment.
Is the SQL("'[]'::jsonb") needed? I think on other places we have only '[]' in coalesce and it works.
e6ecaa5 to
7b8a6a5
Compare
RHINENG-23541
7b8a6a5 to
1dd2df0
Compare
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.
RHINENG-23541
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Bug Fixes: