Repository: apache/dubbo-go-hessian2
Pull request #167 (2020-03-17 10:09:14)

Fix hessian codec bugs
<!--  Thanks for sending a pull request! 
-->

**What this PR does**:

**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes https://github.com/apache/dubbo-go-hessian2/issues/164
Fixes https://github.com/apache/dubbo-go-hessian2/issues/168

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note

```

zonghaishang: @wongoo Please help check TestList test case, local run is passed

https://travis-ci.org/github/apache/dubbo-go-hessian2/builds/663769046?utm_source=github_status&utm_medium=notification

error stack:
```
=== RUN   TestList
--- FAIL: TestList (0.00s)
panic: assignment to entry in nil map [recovered]
	panic: assignment to entry in nil map
goroutine 484 [running]:
testing.tRunner.func1(0xc00039c500)
	/home/travis/.gimme/versions/go1.11.1.linux.amd64/src/testing/testing.go:792 +0x6a7
panic(0x88d280, 0x95fe00)
	/home/travis/.gimme/versions/go1.11.1.linux.amd64/src/runtime/panic.go:513 +0x1b9
github.com/apache/dubbo-go-hessian2.RegisterPOJO(0x960880, 0xc10c20, 0x0)
	/home/travis/build/apache/dubbo-go-hessian2/pojo.go:149 +0x2f9
github.com/apache/dubbo-go-hessian2.RegisterPOJOs(0xc0001b9f38, 0x3, 0x3, 0xc0000a4cc0, 0x871c00, 0x95e650)
	/home/travis/build/apache/dubbo-go-hessian2/pojo.go:218 +0xf6
github.com/apache/dubbo-go-hessian2.TestList(0xc00039c500)
	/home/travis/build/apache/dubbo-go-hessian2/list_test.go:75 +0x125
testing.tRunner(0xc00039c500, 0x906fc8)
	/home/travis/.gimme/versions/go1.11.1.linux.amd64/src/testing/testing.go:827 +0x163
created by testing.(*T).Run
	/home/travis/.gimme/versions/go1.11.1.linux.amd64/src/testing/testing.go:878 +0x651
exit status 2
FAIL	github.com/apache/dubbo-go-hessian2	30.313s
```
codecov-io: # [Codecov](https://codecov.io/gh/apache/dubbo-go-hessian2/pull/167?src=pr&el=h1) Report
> Merging [#167](https://codecov.io/gh/apache/dubbo-go-hessian2/pull/167?src=pr&el=desc) into [master](https://codecov.io/gh/apache/dubbo-go-hessian2/commit/a1d166004ebaa854076e9a200c13b6b6a7257b96&el=desc) will **increase** coverage by `0.96%`.
> The diff coverage is `87.87%`.

[![Impacted file tree graph](https://codecov.io/gh/apache/dubbo-go-hessian2/pull/167/graphs/tree.svg?width=650&height=150&src=pr&token=Yh82j7lS1W)](https://codecov.io/gh/apache/dubbo-go-hessian2/pull/167?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #167      +/-   ##
==========================================
+ Coverage   66.45%   67.42%   +0.96%     
==========================================
  Files          21       22       +1     
  Lines        2573     2671      +98     
==========================================
+ Hits         1710     1801      +91     
+ Misses        667      666       -1     
- Partials      196      204       +8     
```


| [Impacted Files](https://codecov.io/gh/apache/dubbo-go-hessian2/pull/167?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [object.go](https://codecov.io/gh/apache/dubbo-go-hessian2/pull/167/diff?src=pr&el=tree#diff-b2JqZWN0Lmdv) | `64.61% <66.66%> (+1.00%)` | :arrow_up: |
| [pojo.go](https://codecov.io/gh/apache/dubbo-go-hessian2/pull/167/diff?src=pr&el=tree#diff-cG9qby5nbw==) | `60.10% <91.66%> (+3.56%)` | :arrow_up: |
| [response.go](https://codecov.io/gh/apache/dubbo-go-hessian2/pull/167/diff?src=pr&el=tree#diff-cmVzcG9uc2UuZ28=) | `56.37% <100.00%> (+7.11%)` | :arrow_up: |
| [java\_collection.go](https://codecov.io/gh/apache/dubbo-go-hessian2/pull/167/diff?src=pr&el=tree#diff-amF2YV9jb2xsZWN0aW9uLmdv) | `54.66% <0.00%> (ø)` | |
| [list.go](https://codecov.io/gh/apache/dubbo-go-hessian2/pull/167/diff?src=pr&el=tree#diff-bGlzdC5nbw==) | `85.38% <0.00%> (+0.27%)` | :arrow_up: |
| [codec.go](https://codecov.io/gh/apache/dubbo-go-hessian2/pull/167/diff?src=pr&el=tree#diff-Y29kZWMuZ28=) | `65.83% <0.00%> (+0.83%)` | :arrow_up: |
| [hessian.go](https://codecov.io/gh/apache/dubbo-go-hessian2/pull/167/diff?src=pr&el=tree#diff-aGVzc2lhbi5nbw==) | `48.14% <0.00%> (+8.33%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/apache/dubbo-go-hessian2/pull/167?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/apache/dubbo-go-hessian2/pull/167?src=pr&el=footer). Last update [a1d1660...34e6319](https://codecov.io/gh/apache/dubbo-go-hessian2/pull/167?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

wongoo: LGTM

Patch:
diff --git a/object.go b/object.go
index 857ae05..4025dac 100644
--- a/object.go
+++ b/object.go
@@ -541,11 +541,19 @@ func (d *Decoder) decEnum(javaName string, flag int32) (JavaEnum, error) {
 
 // skip this object
 func (d *Decoder) skip(cls classInfo) error {
-	if len(cls.fieldNameList) < 1 {
+	len := len(cls.fieldNameList)
+	if len < 1 {
 		return nil
 	}
-	_, err := d.DecodeValue()
-	return err
+
+	for i := 0; i < len; i++ {
+		// skip class fields.
+		if _, err := d.DecodeValue(); err != nil {
+			return err
+		}
+	}
+
+	return nil
 }
 
 func (d *Decoder) decObject(flag int32) (interface{}, error) {
diff --git a/pojo.go b/pojo.go
index ed5ab8b..0c9f9aa 100644
--- a/pojo.go
+++ b/pojo.go
@@ -130,18 +130,9 @@ func RegisterPOJO(o POJO) int {
 		fieldList  []string
 		structInfo structInfo
 		clsDef     classInfo
-		v          reflect.Value
 	)
 
-	v = reflect.ValueOf(o)
-	switch v.Kind() {
-	case reflect.Struct:
-		structInfo.typ = v.Type()
-	case reflect.Ptr:
-		structInfo.typ = v.Elem().Type()
-	default:
-		structInfo.typ = reflect.TypeOf(o)
-	}
+	structInfo.typ = obtainValueType(o)
 
 	structInfo.goName = structInfo.typ.String()
 	structInfo.javaName = o.JavaClassName()
@@ -210,6 +201,47 @@ func RegisterPOJO(o POJO) int {
 	return structInfo.index
 }
 
+// easy for test
+func unRegisterPOJOs(os ...POJO) []int {
+	arr := make([]int, len(os))
+	for i := range os {
+		arr[i] = unRegisterPOJO(os[i])
+	}
+
+	return arr
+}
+
+func unRegisterPOJO(o POJO) int {
+	pojoRegistry.Lock()
+	defer pojoRegistry.Unlock()
+
+	goName := obtainValueType(o).String()
+
+	if structInfo, ok := pojoRegistry.registry[goName]; ok {
+		delete(pojoRegistry.j2g, structInfo.javaName)
+		listTypeNameMapper.Delete(structInfo.goName)
+		// remove registry cache.
+		delete(pojoRegistry.registry, structInfo.goName)
+		// don't remove registry classInfoList,
+		// indexes of registered pojo may be affected.
+		return structInfo.index
+	}
+
+	return -1
+}
+
+func obtainValueType(o POJO) reflect.Type {
+	v := reflect.ValueOf(o)
+	switch v.Kind() {
+	case reflect.Struct:
+		return v.Type()
+	case reflect.Ptr:
+		return v.Elem().Type()
+	}
+
+	return reflect.TypeOf(o)
+}
+
 // RegisterPOJOs register a POJO instance arr @os. The return value is @os's
 // mathching index array, in which "-1" means its matching POJO has been registered.
 func RegisterPOJOs(os ...POJO) []int {
diff --git a/response.go b/response.go
index 4ba9e39..1fede8e 100644
--- a/response.go
+++ b/response.go
@@ -199,13 +199,18 @@ func unpackResponseBody(decoder *Decoder, resp interface{}) error {
 				return perrors.WithStack(err)
 			}
 			if v, ok := attachments.(map[interface{}]interface{}); ok {
-				atta := ToMapStringString(v)
-				response.Attachments = atta
+				response.Attachments = ToMapStringString(v)
 			} else {
 				return perrors.Errorf("get wrong attachments: %+v", attachments)
 			}
 		}
 
+		// If the return value is nil,
+		// we should consider it normal
+		if rsp == nil {
+			return nil
+		}
+
 		return perrors.WithStack(ReflectResponse(rsp, response.RspObj))
 
 	case RESPONSE_NULL_VALUE, RESPONSE_NULL_VALUE_WITH_ATTACHMENTS:


Test patch:
diff --git a/hessian_test.go b/hessian_test.go
index fa74abe..e466cf2 100644
--- a/hessian_test.go
+++ b/hessian_test.go
@@ -34,8 +34,27 @@ type Case struct {
 	B int
 }
 
+type CaseA struct {
+	A string
+	B int
+	C Case
+}
+
+type CaseB struct {
+	A string
+	B CaseA
+}
+
+func (c *CaseB) JavaClassName() string {
+	return "com.test.caseb"
+}
+
+func (c CaseA) JavaClassName() string {
+	return "com.test.casea"
+}
+
 //JavaClassName  java fully qualified path
-func (c *Case) JavaClassName() string {
+func (c Case) JavaClassName() string {
 	return "com.test.case"
 }
 
@@ -171,3 +190,30 @@ func TestRequest(t *testing.T) {
 	doTestRequest(t, PackageRequest, Zero, []interface{}{"a", 3, true, []*Case{{A: "a", B: 3}}})
 	doTestRequest(t, PackageRequest, Zero, []interface{}{map[string][]*Case{"key": {{A: "a", B: 3}}}})
 }
+
+func TestHessianCodec_ReadAttachments(t *testing.T) {
+	body := &Response{
+		RspObj:      &CaseB{A: "A", B: CaseA{A: "a", B: 1, C: Case{A: "c", B: 2}}},
+		Exception:   nil,
+		Attachments: map[string]string{DUBBO_VERSION_KEY: "2.6.4"},
+	}
+	resp, err := doTestHessianEncodeHeader(t, PackageResponse, Response_OK, body)
+	assert.NoError(t, err)
+
+	unRegisterPOJOs(&CaseB{}, &CaseA{})
+	codecR1 := NewHessianCodec(bufio.NewReader(bytes.NewReader(resp)))
+	codecR2 := NewHessianCodec(bufio.NewReader(bytes.NewReader(resp)))
+	h := &DubboHeader{}
+	assert.NoError(t, codecR1.ReadHeader(h))
+	t.Log(h)
+	assert.NoError(t, codecR2.ReadHeader(h))
+	t.Log(h)
+
+	err = codecR1.ReadBody(body)
+	assert.Equal(t, "can not find go type name com.test.caseb in registry", err.Error())
+	attrs, err := codecR2.ReadAttachments()
+	assert.NoError(t, err)
+	assert.Equal(t, "2.6.4", attrs[DUBBO_VERSION_KEY])
+
+	t.Log(attrs)
+}