fix: Add correct http status codes

* We are always returning 200 for all responses * Set correct response codes based on actual response * Update tests Signed-off-by: Mahendra Paipuri <[email protected]>
mahendrapaipuri · Jan 5, 2024 · 866f946 · 866f946
1 parent dd91e1d
commit 866f946
Show file tree

Hide file tree

Showing 3 changed files with 14 additions and 6 deletions.
diff --git a/cmd/batchjob_stats_server/main_test.go b/cmd/batchjob_stats_server/main_test.go
@@ -35,7 +35,8 @@ func TestBatchjobStatsExecutable(t *testing.T) {
 	}
 
 	jobstats := exec.Command(
-		binary, "--path.data", tmpDir, "--slurm.sacct.path", tmpSacctPath,
+		binary, "--path.data", tmpDir,
+		"--slurm.sacct.path", tmpSacctPath,
 		"--batch.scheduler.slurm",
 		"--web.listen-address", address,
 	)

diff --git a/pkg/jobstats/cli/cli_test.go b/pkg/jobstats/cli/cli_test.go
@@ -9,8 +9,8 @@ import (
 	"time"
 )
 
-func queryExporter(address string) error {
-	resp, err := http.Get(fmt.Sprintf("http://%s/api/jobs", address))
+func queryServer(address string) error {
+	resp, err := http.Get(fmt.Sprintf("http://%s/api/health", address))
 	if err != nil {
 		return err
 	}
@@ -42,7 +42,7 @@ func TestBatchStatsServerMain(t *testing.T) {
 
 	// Query exporter
 	for i := 0; i < 10; i++ {
-		if err := queryExporter("localhost:9020"); err == nil {
+		if err := queryServer("localhost:9020"); err == nil {
 			break
 		}
 		time.Sleep(500 * time.Millisecond)

diff --git a/pkg/jobstats/server/server.go b/pkg/jobstats/server/server.go
@@ -185,12 +185,12 @@ func (s *JobstatsServer) setHeaders(w http.ResponseWriter) {
 func (s *JobstatsServer) accounts(w http.ResponseWriter, r *http.Request) {
 	var response base.AccountsResponse
 	s.setHeaders(w)
-	w.WriteHeader(http.StatusOK)
 
 	// Get current user from header
 	loggedUser, dashboardUser := s.getUser(r)
 	// If no user found, return empty response
 	if loggedUser == "" {
+		w.WriteHeader(http.StatusUnauthorized)
 		response = base.AccountsResponse{
 			Response: base.Response{
 				Status:    "error",
@@ -211,6 +211,7 @@ func (s *JobstatsServer) accounts(w http.ResponseWriter, r *http.Request) {
 	accounts, err := s.Accounts(s.dbConfig.JobstatsDBTable, dashboardUser, s.logger)
 	if err != nil {
 		level.Error(s.logger).Log("msg", "Failed to fetch accounts", "user", dashboardUser, "err", err)
+		w.WriteHeader(http.StatusInternalServerError)
 		response = base.AccountsResponse{
 			Response: base.Response{
 				Status:    "error",
@@ -228,6 +229,7 @@ func (s *JobstatsServer) accounts(w http.ResponseWriter, r *http.Request) {
 	}
 
 	// Write response
+	w.WriteHeader(http.StatusOK)
 	response = base.AccountsResponse{
 		Response: base.Response{
 			Status: "success",
@@ -264,7 +266,6 @@ func (s *JobstatsServer) jobs(w http.ResponseWriter, r *http.Request) {
 	var fromTime, toTime time.Time
 	var response base.JobsResponse
 	s.setHeaders(w)
-	w.WriteHeader(http.StatusOK)
 
 	// Initialise utility vars
 	checkQueryWindow := true                             // Check query window size
@@ -274,6 +275,7 @@ func (s *JobstatsServer) jobs(w http.ResponseWriter, r *http.Request) {
 	loggedUser, dashboardUser := s.getUser(r)
 	// If no user found, return empty response
 	if loggedUser == "" {
+		w.WriteHeader(http.StatusUnauthorized)
 		s.jobsErrorResponse("User Error", "No user identified", w)
 		return
 	}
@@ -328,6 +330,7 @@ func (s *JobstatsServer) jobs(w http.ResponseWriter, r *http.Request) {
 		// Return error response if from is not a timestamp
 		if ts, err := strconv.Atoi(f); err != nil {
 			level.Error(s.logger).Log("msg", "Failed to parse from timestamp", "from", f, "err", err)
+			w.WriteHeader(http.StatusBadRequest)
 			s.jobsErrorResponse("Internal server error", "Malformed 'from' timestamp", w)
 			return
 		} else {
@@ -341,6 +344,7 @@ func (s *JobstatsServer) jobs(w http.ResponseWriter, r *http.Request) {
 		// Return error response if to is not a timestamp
 		if ts, err := strconv.Atoi(t); err != nil {
 			level.Error(s.logger).Log("msg", "Failed to parse to timestamp", "to", t, "err", err)
+			w.WriteHeader(http.StatusBadRequest)
 			s.jobsErrorResponse("Internal server error", "Malformed 'to' timestamp", w)
 			return
 		} else {
@@ -357,6 +361,7 @@ func (s *JobstatsServer) jobs(w http.ResponseWriter, r *http.Request) {
 			"from", fromTime.Format(time.DateTime), "to", toTime.Format(time.DateTime),
 			"queryWindow", toTime.Sub(fromTime).String(),
 		)
+		w.WriteHeader(http.StatusBadRequest)
 		s.jobsErrorResponse("Internal server error", "Maximum query window exceeded", w)
 		return
 	}
@@ -373,11 +378,13 @@ queryJobs:
 	jobs, err := s.Jobs(q, s.logger)
 	if err != nil {
 		level.Error(s.logger).Log("msg", "Failed to fetch jobs", "loggedUser", loggedUser, "err", err)
+		w.WriteHeader(http.StatusInternalServerError)
 		s.jobsErrorResponse("Internal server error", "Failed to fetch user jobs", w)
 		return
 	}
 
 	// Write response
+	w.WriteHeader(http.StatusOK)
 	response = base.JobsResponse{
 		Response: base.Response{
 			Status: "success",