Repository: yarpc/yab
Pull request #332 (2021-04-01 05:06:50)

[yab] handle byte correctly based on thrift spec
Currently the check range for the byte is 0-256 and it is based on a Golang's byte. 
But [thrift spec](https://thrift.apache.org/docs/types) says it is a signed 8-bit integer between -128 and 127. 

Update the logic to reflect this.
```go
func main() {
	v := -8
	fmt.Printf("casted is %+v\n", byte(v))
}
// Output
casted is 248
```
Verified with a Java server that they will handle the overflown 248 correctly in Java (credits to @abhishekparwal)
```java
public class Main
{
	public static void main(String[] args) {
	    byte x = (byte)248;
		System.out.println(x);
	}
}
// Output
-8
```


  

codecov[bot]: # [Codecov](https://codecov.io/gh/yarpc/yab/pull/332?src=pr&el=h1) Report
> Merging [#332](https://codecov.io/gh/yarpc/yab/pull/332?src=pr&el=desc) (55884f2) into [dev](https://codecov.io/gh/yarpc/yab/commit/204b64a9f0ca4e75cbfbe4c4662fd80aebe19845?el=desc) (204b64a) will **decrease** coverage by `0.11%`.
> The diff coverage is `100.00%`.

[![Impacted file tree graph](https://codecov.io/gh/yarpc/yab/pull/332/graphs/tree.svg?width=650&height=150&src=pr&token=yEgrqUTCuh)](https://codecov.io/gh/yarpc/yab/pull/332?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##              dev     #332      +/-   ##
==========================================
- Coverage   90.41%   90.30%   -0.12%     
==========================================
  Files          54       54              
  Lines        2671     2671              
==========================================
- Hits         2415     2412       -3     
- Misses        180      182       +2     
- Partials       76       77       +1     
```


| [Impacted Files](https://codecov.io/gh/yarpc/yab/pull/332?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [thrift/types.go](https://codecov.io/gh/yarpc/yab/pull/332/diff?src=pr&el=tree#diff-dGhyaWZ0L3R5cGVzLmdv) | `100.00% <100.00%> (ø)` | |
| [handler.go](https://codecov.io/gh/yarpc/yab/pull/332/diff?src=pr&el=tree#diff-aGFuZGxlci5nbw==) | `80.89% <0.00%> (-1.69%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/yarpc/yab/pull/332?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/yarpc/yab/pull/332?src=pr&el=footer). Last update [fdb8727...55884f2](https://codecov.io/gh/yarpc/yab/pull/332?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).


Patch:
diff --git a/thrift/types.go b/thrift/types.go
index 51ebcd9..0c5b632 100644
--- a/thrift/types.go
+++ b/thrift/types.go
@@ -25,6 +25,7 @@ import (
 	"errors"
 	"fmt"
 	"io/ioutil"
+	"math"
 	"strconv"
 	"strings"
 )
@@ -115,11 +116,15 @@ func parseBinaryList(vl []interface{}) ([]byte, error) {
 	bs := make([]byte, 0, len(vl))
 	for _, v := range vl {
 		switch v := v.(type) {
+		// YAML uses int for all small values. For large values, it may use int64 or uint64
+		// but those are not valid bool values anyway, so we don't need to check them.
 		case int:
-			// YAML uses int for all small values. For large values, it may use int64 or uint64
-			// but those are not valid bool values anyway, so we don't need to check them.
-			if v < 0 || v >= (1<<8) {
-				return nil, fmt.Errorf("failed to parse list of bytes: %v is not a byte", v)
+			// from https://thrift.apache.org/docs/types, byte is an 8-bit signed integer
+			// In Go, byte is an unsigned int8 (uint8) which has a range [0, 255].
+			// In Java, byte is a signed int8 which has a range of [-128,127].
+			// To make both sides happy, check between [-128,255]
+			if v < math.MinInt8 || v > math.MaxUint8 {
+				return nil, fmt.Errorf("failed to parse list of bytes: %v is not a valid thrift byte", v)
 			}
 			bs = append(bs, byte(v))
 		case string:


Test patch:
diff --git a/thrift/types_test.go b/thrift/types_test.go
index 42c6bab..31e500e 100644
--- a/thrift/types_test.go
+++ b/thrift/types_test.go
@@ -272,6 +272,18 @@ func TestParseBinary(t *testing.T) {
 			value: []interface{}{65, 66},
 			want:  []byte("AB"),
 		},
+		{
+			value: []interface{}{1, -1},
+			want:  []byte{0x1, 0xff},
+		},
+		{
+			value: []interface{}{-128, 127, 255}, // boundary for diff int types
+			want:  []byte{0x80, 0x7f, 0xff},
+		},
+		{
+			value: []interface{}{-1, 255}, // overflow causes the same results
+			want:  []byte{0xff, 0xff},
+		},
 		{
 			value: []interface{}{104, "ello", "", " ", "world"},
 			want:  []byte("hello world"),
@@ -292,6 +304,10 @@ func TestParseBinary(t *testing.T) {
 			value:  []interface{}{256},
 			errMsg: "failed to parse list of bytes",
 		},
+		{
+			value:  []interface{}{-129},
+			errMsg: "failed to parse list of bytes",
+		},
 		{
 			value:  []interface{}{1.5},
 			errMsg: "can only parse list of bytes",