Repository: swaggest/rest
Pull request #156 (2023-04-26 20:35:06)

Fix: Setting the HTTP status during the Encoder setup is insufficient
Sorry, my previous PR had a bug 😅 

Setting the HTTP status during the Encoder setup is insufficient: we must also set it when actually encoding the output (as concrete output values might have different statuses).

vearutop: Could you also add a test for this fix (original bug reproducer)? 
codecov[bot]: ## [Codecov](https://codecov.io/gh/swaggest/rest/pull/156?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=swaggest) Report
> Merging [#156](https://codecov.io/gh/swaggest/rest/pull/156?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=swaggest) (4e140b0) into [master](https://codecov.io/gh/swaggest/rest/commit/558c9adda36dec52270a6078af40891c01f3148f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=swaggest) (558c9ad) will **not change** coverage.
> The diff coverage is `50.00%`.

```diff
@@           Coverage Diff           @@
##           master     #156   +/-   ##
=======================================
  Coverage   77.67%   77.67%           
=======================================
  Files          28       28           
  Lines        1608     1608           
=======================================
  Hits         1249     1249           
  Misses        236      236           
  Partials      123      123           
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `77.67% <50.00%> (ø)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=swaggest#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/swaggest/rest/pull/156?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=swaggest) | Coverage Δ | |
|---|---|---|
| [response/encoder.go](https://codecov.io/gh/swaggest/rest/pull/156?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=swaggest#diff-cmVzcG9uc2UvZW5jb2Rlci5nbw==) | `66.94% <50.00%> (ø)` | |

:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=swaggest)

haimgel: @vearutop I added an explicit test for this.

I don't think this linter complaint is valid, though. This is something that is fixed in golangci-lint 1.51.2...

Patch:
diff --git a/response/encoder.go b/response/encoder.go
index 8b0cdb3..ceeda6f 100644
--- a/response/encoder.go
+++ b/response/encoder.go
@@ -191,14 +191,16 @@ func (h *Encoder) SetupOutput(output interface{}, ht *rest.HandlerTrait) {
 		return
 	}
 
+	ht.SuccessStatus = h.successStatus(output)
+}
+
+func (h *Encoder) successStatus(output interface{}) int {
 	if outputWithStatus, ok := output.(rest.OutputWithHTTPStatus); ok {
-		ht.SuccessStatus = outputWithStatus.HTTPStatus()
+		return outputWithStatus.HTTPStatus()
+	} else if h.skipRendering && !h.outputWithWriter {
+		return http.StatusNoContent
 	} else {
-		if h.skipRendering && !h.outputWithWriter {
-			ht.SuccessStatus = http.StatusNoContent
-		} else {
-			ht.SuccessStatus = http.StatusOK
-		}
+		return http.StatusOK
 	}
 }
 
@@ -353,7 +355,7 @@ func (h *Encoder) WriteSuccessfulResponse(
 	}
 
 	if ht.SuccessStatus == 0 {
-		ht.SuccessStatus = http.StatusOK
+		ht.SuccessStatus = h.successStatus(output)
 	}
 
 	if skipRendering {


Test patch:
diff --git a/response/encoder_test.go b/response/encoder_test.go
index 3035ddf..6f16960 100644
--- a/response/encoder_test.go
+++ b/response/encoder_test.go
@@ -221,12 +221,18 @@ func TestEncoder_SetupOutput_httpStatus(t *testing.T) {
 	e := response.Encoder{}
 	ht := rest.HandlerTrait{}
 	e.SetupOutput(outputWithHTTPStatuses{}, &ht)
+	assert.Equal(t, http.StatusCreated, ht.SuccessStatus)
+}
+
+func TestEncoder_Writer_httpStatus(t *testing.T) {
+	e := response.Encoder{}
+	e.SetupOutput(outputWithHTTPStatuses{}, &rest.HandlerTrait{})
 
 	r, err := http.NewRequest(http.MethodPost, "/", nil)
 	require.NoError(t, err)
 
 	w := httptest.NewRecorder()
-	output := e.MakeOutput(w, ht)
-	e.WriteSuccessfulResponse(w, r, output, ht)
+	output := e.MakeOutput(w, rest.HandlerTrait{})
+	e.WriteSuccessfulResponse(w, r, output, rest.HandlerTrait{})
 	assert.Equal(t, http.StatusCreated, w.Code)
 }