Skip to content

Improve error logging for 500/ISE scenarios #129

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
RahulRengeshOfficial wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Improve error logging for 500/ISE scenarios #129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e0e5787
Improve error logging for 500/ISE scenarios
Mar 12, 2026
7e5ef32
Update response.go
RahulRengeshOfficial Mar 23, 2026
be4f26f
Update webconfig_server.go
RahulRengeshOfficial Mar 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions dataapi/data_service_info.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"github.com/rdkcentral/xconfwebconfig/util"

"github.com/gorilla/mux"
log "github.com/sirupsen/logrus"
)

func GetInfoRefreshAllHandler(w http.ResponseWriter, r *http.Request) {
Expand All @@ -49,9 +50,11 @@ func GetInfoRefreshHandler(w http.ResponseWriter, r *http.Request) {
response, _ := util.JSONMarshal(stats)
xhttp.WriteXconfResponse(w, http.StatusOK, response)
} else {
log.Errorf("GetInfoRefreshHandler failed to get cache stats for table %s: %v", tableName, err)
xhttp.WriteXconfResponse(w, http.StatusInternalServerError, []byte(err.Error()))
}
} else {
log.Errorf("GetInfoRefreshHandler failed to refresh table %s: %v", tableName, err)
xhttp.WriteXconfResponse(w, http.StatusInternalServerError, []byte(err.Error()))
}
}
Expand Down
15 changes: 12 additions & 3 deletions dataapi/estb_firmware_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,10 @@ func GetEstbFirmwareSwuBseHandler(w http.ResponseWriter, r *http.Request) {
return
}
estbFirmwareRuleBase := dataef.NewEstbFirmwareRuleBaseDefault()
bseConfiguration, _ := estbFirmwareRuleBase.GetBseConfiguration(ip)
bseConfiguration, err := estbFirmwareRuleBase.GetBseConfiguration(ip)
if err != nil {
log.Errorf("GetEstbFirmwareSwuBseHandler failed to get BSE configuration: %v", err)
}
if bseConfiguration == nil {
xhttp.WriteXconfResponseAsText(w, 404, []byte("\"<h2>404 NOT FOUND</h2>\""))
return
Comment on lines +69 to 75

Copilot AI Mar 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If GetBseConfiguration returns an error (e.g., DB failure), this logs the error but then returns a 404 when bseConfiguration is nil. That conflates internal failures with ‘not found’. Consider returning 500 on err != nil (and only returning 404 when err == nil and the config is truly absent).

Copilot uses AI. Check for mistakes.
Expand Down Expand Up @@ -125,7 +128,10 @@ func GetEstbFirmwareSwuHandler(w http.ResponseWriter, r *http.Request) {
}

firmwareConfigResponse := sharedef.CreateFirmwareConfigFacadeResponse(*evaluationResult.FirmwareConfig)
response, _ := util.JSONMarshal(firmwareConfigResponse)
response, err := util.JSONMarshal(firmwareConfigResponse)
if err != nil {
log.WithFields(common.FilterLogFields(fields)).Errorf("GetEstbFirmwareSwuHandler failed to marshal firmware config response: %v", err)

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On marshal failure this still returns 200 with a likely empty/invalid body. Please return a 5xx error (and stop) when the response cannot be marshaled.

Suggested change
log.WithFields(common.FilterLogFields(fields)).Errorf("GetEstbFirmwareSwuHandler failed to marshal firmware config response: %v", err)
log.WithFields(common.FilterLogFields(fields)).Errorf("GetEstbFirmwareSwuHandler failed to marshal firmware config response: %v", err)
xhttp.Error(w, http.StatusInternalServerError, common.NotOK)
return

Copilot uses AI. Check for mistakes.
}
xhttp.WriteXconfResponse(w, 200, response)
} else {
xhttp.WriteXconfResponseAsText(w, status, response)
Expand Down Expand Up @@ -175,7 +181,10 @@ func GetFirmwareResponse(w http.ResponseWriter, r *http.Request, xw *xhttp.XResp
log.Debugf("GetEstbFirmwareSwuHandler call AddEstbFirmwareContext ... end contextMap %v", contextMap)
estbFirmwareRuleBase := dataef.NewEstbFirmwareRuleBaseDefault()
convertedContext := sharedef.GetContextConverted(contextMap)
evaluationResult, _ := estbFirmwareRuleBase.Eval(contextMap, convertedContext, contextMap[common.APPLICATION_TYPE], fields)
evaluationResult, err := estbFirmwareRuleBase.Eval(contextMap, convertedContext, contextMap[common.APPLICATION_TYPE], fields)
if err != nil {
log.WithFields(common.FilterLogFields(fields)).Errorf("GetFirmwareResponse firmware rule evaluation failed: %v", err)
}
explanation := GetExplanation(contextMap, evaluationResult)
if evaluationResult == nil || evaluationResult.Blocked || evaluationResult.FirmwareConfig == nil || evaluationResult.FirmwareConfig.Properties == nil {
return http.StatusNotFound, []byte(fmt.Sprintf("\"<h2>404 NOT FOUND</h2><div>%s<div>\"", explanation)), evaluationResult, convertedContext, explanation, contextMap
Comment on lines +184 to 190

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When Eval(...) returns an error, the function may still return 404 Not Found (if evaluationResult is nil/invalid), conflating internal evaluation failures with genuine 'not found' cases. If err != nil, consider returning 500 Internal Server Error (and an appropriate error body) to accurately reflect an evaluation failure.

Copilot uses AI. Check for mistakes.
Expand Down
22 changes: 18 additions & 4 deletions dataapi/estbfirmware/estb_evaluation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ import (
"github.com/rdkcentral/xconfwebconfig/shared"
coreef "github.com/rdkcentral/xconfwebconfig/shared/estbfirmware"
"github.com/rdkcentral/xconfwebconfig/shared/firmware"

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The import block formatting looks like it may not be gofmt-ed (extra blank line before the logrus import within the same group). Please run gofmt (or goimports) so imports are grouped/ordered consistently and CI styling checks (if any) don’t fail.

Suggested change

Copilot uses AI. Check for mistakes.
log "github.com/sirupsen/logrus"
)

// EvaluationResult ...
Expand Down Expand Up @@ -143,7 +145,10 @@ func DownloadLocationRoundRobinFilterContainsVersion(firmwareVersions string, co
* @return true if firmware output must be returned, false if must be blocked
*/
func PercentFilterfilter(evaluationResult *EvaluationResult, context *coreef.ConvertedContext) bool {
filterValue, _ := coreef.GetDefaultPercentFilterValueOneDB()
filterValue, err := coreef.GetDefaultPercentFilterValueOneDB()
if err != nil {
log.Errorf("PercentFilterfilter failed to get default percent filter value: %v", err)
}

Copilot AI Mar 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GetDefaultPercentFilterValueOneDB returns (nil, err) on DB failures. When that happens, filterValue is nil here, but the code immediately dereferences filterValue.EnvModelPercentages, filterValue.Whitelist, etc., which will panic. Add a nil guard (e.g., return false / treat as 0% / default behavior) when filterValue == nil after the call fails.

Suggested change
}
}
if filterValue == nil {
// On DB failure or missing default filter value, avoid panicking and
// fall back to allowing firmware output (no percent-based blocking).
return true
}

Copilot uses AI. Check for mistakes.
matchedEnvModelName := ""
if evaluationResult.MatchedRule != nil && firmware.ENV_MODEL_RULE == evaluationResult.MatchedRule.Type {
matchedEnvModelName = evaluationResult.MatchedRule.Name
Expand All @@ -166,13 +171,19 @@ func PercentFilterfilter(evaluationResult *EvaluationResult, context *coreef.Con
context.AddForceFiltersConverted(firmware.REBOOT_IMMEDIATELY_FILTER)
}
context.AddBypassFiltersConverted(firmware.TIME_FILTER)
config, _ := coreef.GetFirmwareConfigOneDB(envModelPercentage.IntermediateVersion)
config, err := coreef.GetFirmwareConfigOneDB(envModelPercentage.IntermediateVersion)
if err != nil {
log.Errorf("PercentFilterfilter failed to get intermediate version firmware config %s: %v", envModelPercentage.IntermediateVersion, err)
}
if config != nil && context.GetFirmwareVersionConverted() != config.FirmwareVersion {
// return IntermediateVersion firmware config
evaluationResult.FirmwareConfig = coreef.NewFirmwareConfigFacade(config)
evaluationResult.AppliedVersionInfo["firmwareVersionSource"] = "IV,doesntMeetMinCheck"
} else {
config, _ := coreef.GetFirmwareConfigOneDB(envModelPercentage.LastKnownGood)
config, err := coreef.GetFirmwareConfigOneDB(envModelPercentage.LastKnownGood)
if err != nil {
log.Errorf("PercentFilterfilter failed to get LKG firmware config %s: %v", envModelPercentage.LastKnownGood, err)
}
if config != nil {
// return LKG firmware config
evaluationResult.FirmwareConfig = coreef.NewFirmwareConfigFacade(config)
Expand All @@ -183,7 +194,10 @@ func PercentFilterfilter(evaluationResult *EvaluationResult, context *coreef.Con
}
result := fitsPercent(evaluationResult, context, whiteList, percentage)
if !result {
config, _ := coreef.GetFirmwareConfigOneDB(envModelPercentage.LastKnownGood)
config, err := coreef.GetFirmwareConfigOneDB(envModelPercentage.LastKnownGood)
if err != nil {
log.Errorf("PercentFilterfilter failed to get LKG firmware config %s: %v", envModelPercentage.LastKnownGood, err)
}
if config != nil && context.GetFirmwareVersionConverted() != config.FirmwareVersion {
// return LKG firmware config if versions are different
evaluationResult.FirmwareConfig = coreef.NewFirmwareConfigFacade(config)
Expand Down
15 changes: 12 additions & 3 deletions dataapi/estbfirmware/estb_firmware_rule_eval.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -361,13 +361,19 @@ func (e *EstbFirmwareRuleBase) ExtractConfigFromAction(context *coreef.Converted

context.AddBypassFiltersConverted(firmware.TIME_FILTER)

config, _ := coreef.GetFirmwareConfigOneDB(ruleAction.IntermediateVersion)
config, err := coreef.GetFirmwareConfigOneDB(ruleAction.IntermediateVersion)
if err != nil {
log.Errorf("PercentFilter failed to get intermediate version firmware config %s: %v", ruleAction.IntermediateVersion, err)
}
if config != nil && !strings.EqualFold(context.GetFirmwareVersionConverted(), config.FirmwareVersion) {
// return IntermediateVersion firmware config
appliedVersionInfo[FIRMWARE_SOURCE] = "IV,doesntMeetMinCheck"
return ruleAction.IntermediateVersion
} else {
config, _ = coreef.GetFirmwareConfigOneDB(ruleAction.ConfigId) // lkg config
config, err = coreef.GetFirmwareConfigOneDB(ruleAction.ConfigId) // lkg config
if err != nil {
log.Errorf("PercentFilter failed to get LKG firmware config %s: %v", ruleAction.ConfigId, err)
}
if config != nil {
// return LKG firmware config
appliedVersionInfo[FIRMWARE_SOURCE] = "LKG,doesntMeetMinCheck"
Expand All @@ -377,7 +383,10 @@ func (e *EstbFirmwareRuleBase) ExtractConfigFromAction(context *coreef.Converted
return e.ExtractAnyPresentConfig(ruleAction)
}

config, _ := coreef.GetFirmwareConfigOneDB(ruleAction.ConfigId)
config, err := coreef.GetFirmwareConfigOneDB(ruleAction.ConfigId)
if err != nil {
log.Errorf("PercentFilter failed to get firmware config %s: %v", ruleAction.ConfigId, err)
}
if config != nil {
appliedVersionInfo[FIRMWARE_SOURCE] = "LKG,meetMinCheck"
}
Expand Down
7 changes: 6 additions & 1 deletion dataapi/feature_control_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -247,6 +247,8 @@ func GetFeatureControlSettingsHandler(w http.ResponseWriter, r *http.Request) {
featureControl.FeatureResponses = append(featureControl.FeatureResponses, extraFeatureResponses...)
if bbytes, err := json.Marshal(extraFeatureResponses); err == nil {
rfcPostProc = string(bbytes)
} else {
log.WithFields(common.FilterLogFields(fields)).Errorf("GetFeatureControlSettingsHandler failed to marshal post-processing response: %v", err)
}
}
}
Expand Down Expand Up @@ -292,7 +294,10 @@ func GetFeatureControlSettingsHandler(w http.ResponseWriter, r *http.Request) {
featureControlMap := &map[string]rfc.FeatureControl{
"featureControl": *featureControl,
}
response, _ := util.XConfJSONMarshal(featureControlMap, true)
response, err := util.XConfJSONMarshal(featureControlMap, true)
if err != nil {
log.WithFields(common.FilterLogFields(fields)).Errorf("GetFeatureControlSettingsHandler failed to marshal feature control response: %v", err)

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If XConfJSONMarshal returns an error, the code still responds 200 OK and may send an empty/invalid JSON payload. Suggested fix: on err != nil, return an HTTP 500 response (and stop), since the handler cannot reliably produce a valid success body.

Suggested change
log.WithFields(common.FilterLogFields(fields)).Errorf("GetFeatureControlSettingsHandler failed to marshal feature control response: %v", err)
log.WithFields(common.FilterLogFields(fields)).Errorf("GetFeatureControlSettingsHandler failed to marshal feature control response: %v", err)
xhttp.WriteXconfResponseWithHeaders(w, headers, http.StatusInternalServerError, []byte(`{"error":"internal server error"}`))
return

Copilot uses AI. Check for mistakes.
}
xhttp.WriteXconfResponseWithHeaders(w, headers, http.StatusOK, []byte(response))
}

Expand Down
10 changes: 8 additions & 2 deletions dataapi/log_uploader_handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,10 @@ func GetLogUploaderSettings(w http.ResponseWriter, r *http.Request, isTelemetry2
if telemetryProfile == nil {
xhttp.WriteXconfResponseAsText(w, 404, []byte("\"<h2>404 NOT FOUND</h2><div> telemetry profile not found</div>\""))
} else {
response, _ := util.JSONMarshal(*telemetryProfile)
response, err := util.JSONMarshal(*telemetryProfile)
if err != nil {
log.WithFields(common.FilterLogFields(fields)).Errorf("GetLogUploaderSettings failed to marshal telemetry profile: %v", err)

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If util.JSONMarshal fails, the handler still responds 200 with a likely empty/invalid body (response may be nil/empty). This can mislead clients and masks a server-side failure. Recommended: on marshal error, return a 500 (or use the shared error responder) and stop processing instead of writing a success response.

Suggested change
log.WithFields(common.FilterLogFields(fields)).Errorf("GetLogUploaderSettings failed to marshal telemetry profile: %v", err)
log.WithFields(common.FilterLogFields(fields)).Errorf("GetLogUploaderSettings failed to marshal telemetry profile: %v", err)
xhttp.Error(w, http.StatusInternalServerError, common.NotOK)
return

Copilot uses AI. Check for mistakes.
}
xhttp.WriteXconfResponse(w, 200, response)
}
} else {
Expand Down Expand Up @@ -210,7 +213,10 @@ func GetLogUploaderSettings(w http.ResponseWriter, r *http.Request, isTelemetry2
}
LogResultSettings(result, telemetryRule, settingRules, fields)
settingsResponse := logupload.CreateSettingsResponseObject(result)
response, _ := util.JSONMarshal(settingsResponse)
response, err := util.JSONMarshal(settingsResponse)
if err != nil {
log.WithFields(common.FilterLogFields(fields)).Errorf("GetLogUploaderSettings failed to marshal settings response: %v", err)

Copilot AI Mar 23, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If marshaling the settings response fails, the handler still returns HTTP 200 with a potentially empty/invalid response body. Consider returning a 500 (and exiting) when util.JSONMarshal fails so clients don’t receive a successful status with an invalid payload.

Suggested change
log.WithFields(common.FilterLogFields(fields)).Errorf("GetLogUploaderSettings failed to marshal settings response: %v", err)
log.WithFields(common.FilterLogFields(fields)).Errorf("GetLogUploaderSettings failed to marshal settings response: %v", err)
xhttp.WriteXconfResponseAsText(w, http.StatusInternalServerError, []byte("\"Internal Server Error\""))
return

Copilot uses AI. Check for mistakes.
}
xhttp.WriteXconfResponse(w, 200, response)
Comment on lines +219 to 220

Copilot AI Mar 12, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same issue as above: a marshal failure still results in HTTP 200. Please return an error status (e.g., 500) and avoid emitting a successful response when serialization fails.

Suggested change
}
xhttp.WriteXconfResponse(w, 200, response)
xhttp.WriteXconfResponse(w, http.StatusInternalServerError, []byte("internal server error"))
return
}
xhttp.WriteXconfResponse(w, http.StatusOK, response)

Copilot uses AI. Check for mistakes.
}
}
Expand Down
Loading