From 1a00a4b14f930c4588ede207c18c341336a760fd Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E7=BD=97=E6=B3=BD=E8=BD=A9?= <spacewanderlzx@gmail.com>
Date: Mon, 14 Feb 2022 10:15:33 +0800
Subject: [PATCH] fix: write response header should not break req (#65)

---
 docs/en/latest/getting-started.md |  7 ++++--
 internal/http/response.go         |  4 ++-
 internal/plugin/conf.go           |  4 +--
 internal/plugin/plugin_test.go    | 42 +++++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+), 5 deletions(-)

diff --git a/docs/en/latest/getting-started.md b/docs/en/latest/getting-started.md
index 78bc87a..ca6e6e1 100644
--- a/docs/en/latest/getting-started.md
+++ b/docs/en/latest/getting-started.md
@@ -128,9 +128,12 @@ func (p *Say) Filter(conf interface{}, w http.ResponseWriter, r pkgHTTP.Request)
 }
 ```
 
-We can see that the Filter takes the value of the body set in the configuration as the response body. If we respond directly in the plugin, it will response directly in the APISIX without touching the upstream.
+We can see that the Filter takes the value of the body set in the configuration as the response body. If we call `Write` or `WriteHeader` of the `http.ResponseWriter`
+(respond directly in the plugin), it will response directly in the APISIX without touching the upstream. As we only support setting headers of local response,
+calling `Header` won't take any effects without calling `Write` or `WriteHeader`. (TODO: support setting response header separately)
 
-Here you can read the API documentation provided by the Go Runner SDK: https://pkg.go.dev/github.com/apache/apisix-go-plugin-runner
+For the `pkgHTTP.Request`, you can refer to the API documentation provided by the Go Runner SDK:
+https://pkg.go.dev/github.com/apache/apisix-go-plugin-runner
 
 After building the application (`make build` in the example), we need to set some environment variables at runtime:
 
diff --git a/internal/http/response.go b/internal/http/response.go
index acf349e..dcf120a 100644
--- a/internal/http/response.go
+++ b/internal/http/response.go
@@ -45,6 +45,8 @@ func (r *Response) Write(b []byte) (int, error) {
 		r.body = &bytes.Buffer{}
 	}
 
+	// APISIX will convert code 0 to 200, so we don't need to WriteHeader(http.StatusOK)
+	// before writing the data
 	return r.body.Write(b)
 }
 
@@ -64,7 +66,7 @@ func (r *Response) Reset() {
 }
 
 func (r *Response) HasChange() bool {
-	return !(r.body == nil && r.code == 0 && len(r.hdr) == 0)
+	return !(r.body == nil && r.code == 0)
 }
 
 func (r *Response) FetchChanges(id uint32, builder *flatbuffers.Builder) bool {
diff --git a/internal/plugin/conf.go b/internal/plugin/conf.go
index 5758f0a..7c2cc6c 100644
--- a/internal/plugin/conf.go
+++ b/internal/plugin/conf.go
@@ -101,8 +101,8 @@ func (cc *ConfCache) Set(req *pc.Req) (uint32, error) {
 			conf, err := plugin.ParseConf(v)
 			if err != nil {
 				log.Errorf(
-					"failed to parse configuration for plugin %s, configuration: %s",
-					name, string(v))
+					"failed to parse configuration for plugin %s, configuration: %s, err: %v",
+					name, string(v), err)
 				continue
 			}
 
diff --git a/internal/plugin/plugin_test.go b/internal/plugin/plugin_test.go
index 9016360..9893fb5 100644
--- a/internal/plugin/plugin_test.go
+++ b/internal/plugin/plugin_test.go
@@ -289,6 +289,7 @@ func TestFilter(t *testing.T) {
 	}
 	fooFilter := func(conf interface{}, w http.ResponseWriter, r pkgHTTP.Request) {
 		w.Header().Add("foo", "bar")
+		w.WriteHeader(200)
 		assert.Equal(t, "foo", conf.(string))
 	}
 	barParseConf := func(in []byte) (conf interface{}, err error) {
@@ -333,3 +334,44 @@ func TestFilter(t *testing.T) {
 	assert.Equal(t, "bar", resp.Header().Get("foo"))
 	assert.Equal(t, "bar", req.Header().Get("foo"))
 }
+
+func TestFilter_SetRespHeaderDoNotBreakReq(t *testing.T) {
+	InitConfCache(1 * time.Millisecond)
+
+	barParseConf := func(in []byte) (conf interface{}, err error) {
+		return "", nil
+	}
+	barFilter := func(conf interface{}, w http.ResponseWriter, r pkgHTTP.Request) {
+		r.Header().Set("foo", "bar")
+	}
+	filterSetRespParseConf := func(in []byte) (conf interface{}, err error) {
+		return "", nil
+	}
+	filterSetRespFilter := func(conf interface{}, w http.ResponseWriter, r pkgHTTP.Request) {
+		w.Header().Add("foo", "baz")
+	}
+	RegisterPlugin("bar", barParseConf, barFilter)
+	RegisterPlugin("filterSetResp", filterSetRespParseConf, filterSetRespFilter)
+
+	builder := flatbuffers.NewBuilder(1024)
+	barName := builder.CreateString("bar")
+	barConf := builder.CreateString("a")
+	filterSetRespName := builder.CreateString("filterSetResp")
+	filterSetRespConf := builder.CreateString("a")
+	prepareConfWithData(builder, filterSetRespName, filterSetRespConf, barName, barConf)
+
+	res, _ := GetRuleConf(1)
+	hrc.ReqStart(builder)
+	hrc.ReqAddId(builder, 233)
+	hrc.ReqAddConfToken(builder, 1)
+	r := hrc.ReqEnd(builder)
+	builder.Finish(r)
+	out := builder.FinishedBytes()
+
+	req := inHTTP.CreateRequest(out)
+	resp := inHTTP.CreateResponse()
+	filter(res, resp, req)
+
+	assert.Equal(t, "bar", req.Header().Get("foo"))
+	assert.Equal(t, "baz", resp.Header().Get("foo"))
+}