RHINENG-21786: add severity name to advisory related APIs#2098
RHINENG-21786: add severity name to advisory related APIs#2098MichaelMraka wants to merge 6 commits intoRedHatInsights:masterfrom
Conversation
Reviewer's GuideAdds severity name support and JSONB-backed list fields to advisory-related APIs, simplifying DB models and queries while exposing new filtering/export capabilities and cleaning up legacy parsing logic. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
SC Environment Impact AssessmentOverall Impact: ⚪ NONE No SC Environment-specific impacts detected in this PR. What was checkedThis PR was automatically scanned for:
|
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
AdvisoryDetailAttributes, fieldsTopicandSeverityare populated viaam.*but their names don’t match the underlying column names (summary,severity_id), so you should add explicitgorm:"column:summary"andgorm:"column:severity_id"tags (otherwise they’ll always be zero-valued). - In
buildSystemAdvisoriesQueryyou joinadvisory_severitywithLEFT JOIN advisory_severity sev ON sa.advisory_id = sev.id, which looks incorrect – this should likely join on the advisory’s severity ID (e.g.am.severity_id = sev.id) rather than the advisory ID, otherwise the severity name will be wrong or null.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `AdvisoryDetailAttributes`, fields `Topic` and `Severity` are populated via `am.*` but their names don’t match the underlying column names (`summary`, `severity_id`), so you should add explicit `gorm:"column:summary"` and `gorm:"column:severity_id"` tags (otherwise they’ll always be zero-valued).
- In `buildSystemAdvisoriesQuery` you join `advisory_severity` with `LEFT JOIN advisory_severity sev ON sa.advisory_id = sev.id`, which looks incorrect – this should likely join on the advisory’s severity ID (e.g. `am.severity_id = sev.id`) rather than the advisory ID, otherwise the severity name will be wrong or null.
## Individual Comments
### Comment 1
<location path="manager/controllers/advisory_detail.go" line_range="36-40" />
<code_context>
+ Description string `json:"description"`
+ ModifiedDate *time.Time `json:"modified_date"`
+ PublicDate *time.Time `json:"public_date"`
+ Topic string `json:"topic" query:"summary"`
+ Synopsis string `json:"synopsis"`
+ Solution *string `json:"solution"`
+ AdvisoryTypeName string `json:"advisory_type_name"`
+ Severity *int `json:"severity" query:"severity_id"`
+ SeverityName *string `json:"severity_name,omitempty"`
+ Fixes *string `json:"fixes"`
</code_context>
<issue_to_address>
**issue (bug_risk):** The `Topic` and `Severity` fields will not be populated correctly from `am.*` due to column name mismatches.
In `getAdvisoryFromDB`, selecting `am.*` into `AdvisoryDetailAttributes` relies on GORM’s default snake_case mapping. Since the DB columns are `summary` and `severity_id` but the struct fields are `Topic` and `Severity`, GORM will look for `topic` and `severity` columns instead, leaving `summary`/`severity_id` unused and changing behavior from the previous explicit mapping.
To preserve correct values, add explicit column tags, e.g.:
```go
Topic string `json:"topic" query:"summary" gorm:"column:summary"`
Severity *int `json:"severity" query:"severity_id" gorm:"column:severity_id"`
```
or rename the fields to match the DB columns.
</issue_to_address>
### Comment 2
<location path="manager/controllers/system_advisories.go" line_range="29-32" />
<code_context>
}
-type RelList []string
+type RelList datatypes.JSONSlice[string]
type SystemAdvisoriesDBLookup struct {
</code_context>
<issue_to_address>
**issue (bug_risk):** RelList.String currently won’t compile because it passes a custom type to `strings.Join` without conversion.
With `RelList` now defined as `datatypes.JSONSlice[string]`, it’s no longer directly assignable to `[]string`, so `strings.Join(v, ",")` won’t compile. Convert explicitly:
```go
func (v RelList) String() string {
return strings.Join([]string(v), ",")
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2098 +/- ##
==========================================
- Coverage 59.40% 59.13% -0.27%
==========================================
Files 134 134
Lines 8707 8640 -67
==========================================
- Hits 5172 5109 -63
- Misses 2989 2996 +7
+ Partials 546 535 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
24dd3c0 to
49c2b32
Compare
ae1b52c to
67b910f
Compare
and remove lot of custom json column parsing function
so we can get rid of special handling of AdvisoryTypes
Dugowitch
left a comment
There was a problem hiding this comment.
Apart from the mysql driver, looks good to me 👍
|
/retest |
1 similar comment
|
/retest |
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist
Summary by Sourcery
Add severity name support and JSONB-backed list fields to advisory-related APIs while simplifying advisory metadata lookups and exports.
New Features:
Enhancements:
Build:
Tests: