Skip to content

Commit c017ed0

Browse files
prkhrkatprkhrkat
authored
Merge pull request #6797 from devtron-labs/fix-api-responses
fix: api responses enhancements
2 parents d9c6787 + be79d8a commit c017ed0

File tree

7 files changed

+70
-22
lines changed

7 files changed

+70
-22
lines changed

‎api/appStore/deployment/CommonDeploymentRestHandler.go‎

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -87,12 +87,12 @@ func (handler *CommonDeploymentRestHandlerImpl) getAppOfferingMode(installedAppI
8787
if len(appId) > 0 {
8888
appIdentifier, err := handler.helmAppService.DecodeAppId(appId)
8989
if err != nil {
90-
err = &util.ApiError{HttpStatusCode: http.StatusBadRequest, UserMessage: "invalid app id"}
90+
// err = &util.ApiError{HttpStatusCode: http.StatusBadRequest, UserMessage: "invalid app id"}
9191
return appOfferingMode, installedAppDto, err
9292
}
9393
installedAppDto, err = handler.installedAppService.GetInstalledAppByClusterNamespaceAndName(appIdentifier)
9494
if err != nil {
95-
err = &util.ApiError{HttpStatusCode: http.StatusBadRequest, UserMessage: "unable to find app in database"}
95+
// err = &util.ApiError{HttpStatusCode: http.StatusBadRequest, UserMessage: "unable to find app in database"}
9696
return appOfferingMode, installedAppDto, err
9797
}
9898
// this is the case when hyperion apps does not linked yet
@@ -103,7 +103,7 @@ func (handler *CommonDeploymentRestHandlerImpl) getAppOfferingMode(installedAppI
103103
installedAppDto.AppOfferingMode = appOfferingMode
104104
appIdentifier, err := handler.helmAppService.DecodeAppId(appId)
105105
if err != nil {
106-
err = &util.ApiError{HttpStatusCode: http.StatusBadRequest, UserMessage: "invalid app id"}
106+
// err = &util.ApiError{HttpStatusCode: http.StatusBadRequest, UserMessage: "invalid app id, expected format clusterId|namespace|releaseName"}
107107
return appOfferingMode, installedAppDto, err
108108
}
109109
installedAppDto.ClusterId = appIdentifier.ClusterId
@@ -113,16 +113,17 @@ func (handler *CommonDeploymentRestHandlerImpl) getAppOfferingMode(installedAppI
113113
} else if len(installedAppId) > 0 {
114114
installedAppId, err := strconv.Atoi(installedAppId)
115115
if err != nil {
116-
err = &util.ApiError{HttpStatusCode: http.StatusBadRequest, UserMessage: "invalid installed app id"}
116+
handler.Logger.Errorw("Invalid installedAppId expected int value", "installedAppId", installedAppId, "err", err)
117117
return appOfferingMode, installedAppDto, err
118118
}
119119
installedAppDto, err = handler.installedAppService.GetInstalledAppByInstalledAppId(installedAppId)
120120
if err != nil {
121-
err = &util.ApiError{HttpStatusCode: http.StatusBadRequest, UserMessage: "unable to find app in database"}
121+
// err = &util.ApiError{HttpStatusCode: http.StatusBadRequest, UserMessage: "unable to find app in database"}
122122
return appOfferingMode, installedAppDto, err
123123
}
124124
} else {
125125
err := &util.ApiError{HttpStatusCode: http.StatusBadRequest, UserMessage: "app id missing in request"}
126+
handler.Logger.Errorw("appId is missing and is a required field", "appId", appId, "err", err)
126127
return appOfferingMode, installedAppDto, err
127128
}
128129
if installedAppDto != nil && installedAppDto.InstalledAppId > 0 {
@@ -188,6 +189,7 @@ func (handler *CommonDeploymentRestHandlerImpl) GetDeploymentHistoryValues(w htt
188189
v := r.URL.Query()
189190
installedAppId := v.Get("installedAppId")
190191
appId := v.Get("appId")
192+
191193
appOfferingMode, installedAppDto, err := handler.getAppOfferingMode(installedAppId, appId)
192194
if err != nil {
193195
common.WriteJsonResp(w, err, "bad request", http.StatusBadRequest)

‎api/cluster/EnvironmentRestHandler.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -447,11 +447,15 @@ func (impl EnvironmentRestHandlerImpl) GetCombinedEnvironmentListForDropDownByCl
447447
id, err := strconv.Atoi(clusterId)
448448
if err != nil {
449449
impl.logger.Errorw("request err, GetCombinedEnvironmentListForDropDownByClusterIds", "err", err, "clusterIdString", clusterIdString)
450-
common.WriteJsonResp(w, err, "please send valid cluster Ids", http.StatusBadRequest)
450+
common.HandleParameterError(w, r, "ids", clusterIdString)
451451
return
452452
}
453453
clusterIds = append(clusterIds, id)
454454
}
455+
} else {
456+
impl.logger.Errorw("request err empty query param, GetCombinedEnvironmentListForDropDownByClusterIds", "err", err, "clusterIdString", clusterIdString)
457+
common.HandleParameterError(w, r, "ids", clusterIdString)
458+
return
455459
}
456460
token := r.Header.Get("token")
457461
clusters, err := impl.environmentClusterMappingsService.GetCombinedEnvironmentListForDropDownByClusterIds(token, clusterIds, impl.rbacEnforcementUtil.CheckAuthorizationForGlobalEnvironment)

‎api/devtronResource/DevtronResourceHistoryRestHandler.go‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,9 +54,16 @@ func (handler *HistoryRestHandlerImpl) GetDeploymentHistory(w http.ResponseWrite
5454
queryParams := apiBean.GetHistoryQueryParams{}
5555
err := decoder.Decode(&queryParams, v)
5656
if err != nil {
57+
handler.logger.Errorw("error in decoding query parameters", "queryParams", v, "err", err)
5758
common.WriteJsonResp(w, err, nil, http.StatusBadRequest)
5859
return
5960
}
61+
// Validating that filterCriteria is provided
62+
if len(queryParams.FilterCriteria) == 0 {
63+
handler.logger.Errorw("missing required filterCriteria parameter")
64+
common.HandleParameterError(w, r, "filterCriteria", "")
65+
return
66+
}
6067
decodedReqBean, err := handler.apiReqDecoderService.GetFilterCriteriaParamsForDeploymentHistory(queryParams.FilterCriteria)
6168
if err != nil {
6269
handler.logger.Errorw("error in getting filter criteria params", "err", err, "filterCriteria", queryParams.FilterCriteria)

‎api/helm-app/service/helper.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ import (
2929
func DecodeExternalAppAppId(appId string) (*bean.AppIdentifier, error) {
3030
component := strings.Split(appId, "|")
3131
if len(component) != 3 {
32-
return nil, fmt.Errorf("malformed app id %s", appId)
32+
return nil, fmt.Errorf("malformed appId, appId: %s, expected format clusterId|namespace|releaseName", appId)
3333
}
3434
clusterId, err := strconv.Atoi(component[0])
3535
if err != nil {

‎api/restHandler/app/pipeline/AutoCompleteRestHandler.go‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -136,7 +136,11 @@ func (handler DevtronAppAutoCompleteRestHandlerImpl) GetAppListForAutocomplete(w
136136
return
137137
}
138138
} else {
139-
teamIdInt, err = strconv.Atoi(teamId)
139+
teamIdInt, err = common.ExtractIntPathParamWithContext(w, r, "teamId", teamId+" team")
140+
if err != nil {
141+
// Error already written by ExtractIntPathParamWithContext
142+
return
143+
}
140144
if err != nil {
141145
common.WriteJsonResp(w, err, nil, http.StatusBadRequest)
142146
return

‎api/restHandler/app/pipeline/configure/BuildPipelineRestHandler.go‎

Lines changed: 27 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -1209,15 +1209,21 @@ func (handler *PipelineConfigRestHandlerImpl) FetchMaterialInfo(w http.ResponseW
12091209

12101210
func (handler *PipelineConfigRestHandlerImpl) GetCIPipelineById(w http.ResponseWriter, r *http.Request) {
12111211
token := r.Header.Get("token")
1212-
vars := mux.Vars(r)
1213-
appId, err := strconv.Atoi(vars["appId"])
1212+
// vars := mux.Vars(r)
1213+
//appId, err := strconv.Atoi(vars["appId"])
1214+
appId, err := common.ExtractIntPathParamWithContext(w, r, "appId", "app")
12141215
if err != nil {
1215-
common.WriteJsonResp(w, err, nil, http.StatusBadRequest)
1216+
// response already written by ExtractIntPathParamWithContext
1217+
//common.WriteJsonResp(w, err, nil, http.StatusBadRequest)
1218+
handler.Logger.Error("request err getting , GetCIPipelineById", "appId", appId, "err", err)
12161219
return
12171220
}
1218-
pipelineId, err := strconv.Atoi(vars["pipelineId"])
1221+
//pipelineId, err := strconv.Atoi(vars["pipelineId"])
1222+
pipelineId, err := common.ExtractIntPathParamWithContext(w, r, "pipelineId", "pipeline")
12191223
if err != nil {
1220-
common.WriteJsonResp(w, err, nil, http.StatusBadRequest)
1224+
// response already written by ExtractIntPathParamWithContext
1225+
// common.WriteJsonResp(w, err, nil, http.StatusBadRequest)
1226+
handler.Logger.Error("request err getting , GetCIPipelineById", "pipelineId", pipelineId, "err", err)
12211227
return
12221228
}
12231229

@@ -1711,20 +1717,29 @@ func (handler *PipelineConfigRestHandlerImpl) FetchWorkflowDetails(w http.Respon
17111717
common.HandleUnauthorized(w, r)
17121718
return
17131719
}
1714-
vars := mux.Vars(r)
1715-
appId, err := strconv.Atoi(vars["appId"])
1720+
// vars := mux.Vars(r)
1721+
// appId, err = strconv.Atoi(vars["appId"])
1722+
appId, err := common.ExtractIntPathParamWithContext(w, r, "appId", "app")
17161723
if err != nil {
1717-
common.WriteJsonResp(w, err, nil, http.StatusBadRequest)
1724+
// response already written by ExtractIntPathParamWithContext
1725+
// common.WriteJsonResp(w, err, nil, http.StatusBadRequest)
1726+
handler.Logger.Error("request err getting , FetchWorkflowDetails", "appId", appId, "err", err)
17181727
return
17191728
}
1720-
pipelineId, err := strconv.Atoi(vars["pipelineId"])
1729+
// pipelineId, err := strconv.Atoi(vars["pipelineId"])
1730+
pipelineId, err := common.ExtractIntPathParamWithContext(w, r, "pipelineId", "pipeline")
17211731
if err != nil {
1722-
common.WriteJsonResp(w, err, nil, http.StatusBadRequest)
1732+
// response already written by ExtractIntPathParamWithContext
1733+
// common.WriteJsonResp(w, err, nil, http.StatusBadRequest)
1734+
handler.Logger.Error("request err getting , FetchWorkflowDetails", "pipelineId", pipelineId, "err", err)
17231735
return
17241736
}
1725-
buildId, err := strconv.Atoi(vars["workflowId"])
1737+
// buildId, err := strconv.Atoi(vars["workflowId"])
1738+
buildId, err := common.ExtractIntPathParamWithContext(w, r, "workflowId", "workflow")
17261739
if err != nil || buildId == 0 {
1727-
common.WriteJsonResp(w, err, nil, http.StatusBadRequest)
1740+
// response already written by ExtractIntPathParamWithContext
1741+
// common.WriteJsonResp(w, err, nil, http.StatusBadRequest)
1742+
handler.Logger.Error("request err getting , FetchWorkflowDetails", "buildId", buildId, "err", err)
17281743
return
17291744
}
17301745
handler.Logger.Infow("request payload, FetchWorkflowDetails", "appId", appId, "pipelineId", pipelineId, "buildId", buildId, "buildId", buildId)

‎pkg/devtronResource/APIReqDecoderService.go‎

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"go.uber.org/zap"
99
"net/http"
1010
"strconv"
11+
"strings"
1112
)
1213

1314
// APIReqDecoderService is common service used for getting decoded devtronResource and history related api params.
@@ -52,11 +53,26 @@ func (impl *APIReqDecoderServiceImpl) GetFilterCriteriaParamsForDeploymentHistor
5253
return nil, &util2.ApiError{
5354
HttpStatusCode: http.StatusBadRequest,
5455
Code: "400",
55-
InternalMessage: "invalid format filter criteria!",
56-
UserMessage: "invalid format filter criteria!",
56+
InternalMessage: "missing required identifiers: either (appId and envId) or pipelineId must be provided to fetch history",
57+
UserMessage: "Please provide either both App ID and Env ID, or a Pipeline ID to view history.",
5758
}
5859
}
5960
if resp.PipelineId == 0 {
61+
missingResources := []string{}
62+
if resp.AppId == 0 {
63+
missingResources = append(missingResources, "appId")
64+
}
65+
if resp.EnvId == 0 {
66+
missingResources = append(missingResources, "EnvId")
67+
}
68+
if len(missingResources) > 0 {
69+
return nil, &util2.ApiError{
70+
HttpStatusCode: http.StatusBadRequest,
71+
Code: "400",
72+
InternalMessage: "missing required resources: " + strings.Join(missingResources, ", "),
73+
UserMessage: "missing required resources: " + strings.Join(missingResources, ", "),
74+
}
75+
}
6076
pipelineObj, err := impl.pipelineRepository.FindActiveByAppIdAndEnvId(resp.AppId, resp.EnvId)
6177
if err != nil {
6278
impl.logger.Errorw("error in getting pipeline", "appId", resp.AppId, "envId", resp.EnvId, "err", err)

0 commit comments

Comments
 (0)