Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Not 100% compatibility with encoding/json #4

Closed
wangkechun opened this issue May 31, 2021 · 10 comments
Closed

Not 100% compatibility with encoding/json #4

wangkechun opened this issue May 31, 2021 · 10 comments
Assignees
@chenzhuoyu
Labels
question Further information is requested

Comments

@wangkechun
Copy link 

func TestCompatibility(t *testing.T) {
	inputs := []interface{}{`"<&>"`, `"\"<&>\""`, "\b", float64(-0), float32(-0), map[string]int{"3": 3, "2": 2, "1": 1}}
	for _, input := range inputs {
		t.Run(fmt.Sprintf("case %v", input), func(t *testing.T) {
			buf1, err1 := json.Marshal(input)
			buf2, err2 := Marshal(input)
			require.Nil(t, err1)
			require.Nil(t, err2)
			require.Equal(t, string(buf1), string(buf2))
		})
	}
}

--- FAIL: TestCompatibility (0.00s)
    --- FAIL: TestCompatibility/case_"<&>" (0.00s)
        fuzz_test.go:112: 
                Error Trace:    fuzz_test.go:112
                Error:          Not equal: 
                                expected: "\"\\\"\\u003c\\u0026\\u003e\\\"\""
                                actual  : "\"\\\"<&>\\\"\""
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -"\"\u003c\u0026\u003e\""
                                +"\"<&>\""
                Test:           TestCompatibility/case_"<&>"
    --- FAIL: TestCompatibility/case_"\"<&>\"" (0.00s)
        fuzz_test.go:112: 
                Error Trace:    fuzz_test.go:112
                Error:          Not equal: 
                                expected: "\"\\\"\\\\\\\"\\u003c\\u0026\\u003e\\\\\\\"\\\"\""
                                actual  : "\"\\\"\\\\\\\"<&>\\\\\\\"\\\"\""
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -"\"\\\"\u003c\u0026\u003e\\\"\""
                                +"\"\\\"<&>\\\"\""
                Test:           TestCompatibility/case_"\"<&>\""
    --- FAIL: TestCompatibility/case_\b (0.00s)
        fuzz_test.go:112: 
                Error Trace:    fuzz_test.go:112
                Error:          Not equal: 
                                expected: "\"\\u0008\""
                                actual  : "\"\\b\""
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -"\u0008"
                                +"\b"
                Test:           TestCompatibility/case_\b
    --- FAIL: TestCompatibility/case_map[1:1_2:2_3:3] (0.00s)
        fuzz_test.go:112: 
                Error Trace:    fuzz_test.go:112
                Error:          Not equal: 
                                expected: "{\"1\":1,\"2\":2,\"3\":3}"
                                actual  : "{\"2\":2,\"1\":1,\"3\":3}"
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -{"1":1,"2":2,"3":3}
                                +{"2":2,"1":1,"3":3}
                Test:           TestCompatibility/case_map[1:1_2:2_3:3]
FAIL
exit status 1
FAIL    github.com/bytedance/sonic      0.144s
@wangkechun
Copy link
Author 

func TestPrefilledArr(t *testing.T) {
	a := &[...]int{1, 2}
	b := &[...]int{1, 2}
	err1 := json.Unmarshal([]byte(`[3]`), a)
	err2 := Unmarshal([]byte(`[3]`), b)
	require.Equal(t, err1, err2)
	require.Equal(t, a, b)
}

=== RUN   TestPrefilledArr
    fuzz_test.go:109: 
                Error Trace:    fuzz_test.go:109
                Error:          Not equal: 
                                expected: &[2]int{3, 0}
                                actual  : &[2]int{3, 2}
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -2,3 +2,3 @@
                                  (int) 3,
                                - (int) 0
                                + (int) 2
                                 })
                Test:           TestPrefilledArr
--- FAIL: TestPrefilledArr (0.00s)
FAIL
exit status 1

@chenzhuoyu
Copy link
Member 

func TestPrefilledArr(t *testing.T) {
	a := &[...]int{1, 2}
	b := &[...]int{1, 2}
	err1 := json.Unmarshal([]byte(`[3]`), a)
	err2 := Unmarshal([]byte(`[3]`), b)
	require.Equal(t, err1, err2)
	require.Equal(t, a, b)
}

=== RUN   TestPrefilledArr
    fuzz_test.go:109: 
                Error Trace:    fuzz_test.go:109
                Error:          Not equal: 
                                expected: &[2]int{3, 0}
                                actual  : &[2]int{3, 2}
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -2,3 +2,3 @@
                                  (int) 3,
                                - (int) 0
                                + (int) 2
                                 })
                Test:           TestPrefilledArr
--- FAIL: TestPrefilledArr (0.00s)
FAIL
exit status 1

@wangkechun Please move this into a separate issue. Thanks.

@chenzhuoyu
Copy link
Member 

func TestCompatibility(t *testing.T) {
	inputs := []interface{}{`"<&>"`, `"\"<&>\""`, "\b", float64(-0), float32(-0), map[string]int{"3": 3, "2": 2, "1": 1}}
	for _, input := range inputs {
		t.Run(fmt.Sprintf("case %v", input), func(t *testing.T) {
			buf1, err1 := json.Marshal(input)
			buf2, err2 := Marshal(input)
			require.Nil(t, err1)
			require.Nil(t, err2)
			require.Equal(t, string(buf1), string(buf2))
		})
	}
}

--- FAIL: TestCompatibility (0.00s)
    --- FAIL: TestCompatibility/case_"<&>" (0.00s)
        fuzz_test.go:112: 
                Error Trace:    fuzz_test.go:112
                Error:          Not equal: 
                                expected: "\"\\\"\\u003c\\u0026\\u003e\\\"\""
                                actual  : "\"\\\"<&>\\\"\""
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -"\"\u003c\u0026\u003e\""
                                +"\"<&>\""
                Test:           TestCompatibility/case_"<&>"
    --- FAIL: TestCompatibility/case_"\"<&>\"" (0.00s)
        fuzz_test.go:112: 
                Error Trace:    fuzz_test.go:112
                Error:          Not equal: 
                                expected: "\"\\\"\\\\\\\"\\u003c\\u0026\\u003e\\\\\\\"\\\"\""
                                actual  : "\"\\\"\\\\\\\"<&>\\\\\\\"\\\"\""
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -"\"\\\"\u003c\u0026\u003e\\\"\""
                                +"\"\\\"<&>\\\"\""
                Test:           TestCompatibility/case_"\"<&>\""
    --- FAIL: TestCompatibility/case_\b (0.00s)
        fuzz_test.go:112: 
                Error Trace:    fuzz_test.go:112
                Error:          Not equal: 
                                expected: "\"\\u0008\""
                                actual  : "\"\\b\""
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -"\u0008"
                                +"\b"
                Test:           TestCompatibility/case_\b
    --- FAIL: TestCompatibility/case_map[1:1_2:2_3:3] (0.00s)
        fuzz_test.go:112: 
                Error Trace:    fuzz_test.go:112
                Error:          Not equal: 
                                expected: "{\"1\":1,\"2\":2,\"3\":3}"
                                actual  : "{\"2\":2,\"1\":1,\"3\":3}"
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -1 +1 @@
                                -{"1":1,"2":2,"3":3}
                                +{"2":2,"1":1,"3":3}
                Test:           TestCompatibility/case_map[1:1_2:2_3:3]
FAIL
exit status 1
FAIL    github.com/bytedance/sonic      0.144s

This IS compatible with stdlib. Compatibility does not mean it should operates exactly like stdlib, minor diffs does not affect compatibility.

The \b escape

According to RFC8259, \b is a valid escape sequence, and the stdlib does not explain why it don't escape to \b. So I consider it's a bug in the stdlib.

HTMLEscape

Should we support this? @PureWhiteWu

The key order of map

maps in Go are intrinsically unordered, the Go Spec also says you should NOT rely on the traversal order. And for performance reason, the key is not ordered by design.

@chenzhuoyu chenzhuoyu added help wanted Extra attention is needed question Further information is requested and removed help wanted Extra attention is needed labels May 31, 2021
@PureWhiteWu
Copy link
Collaborator

Do you use HTMLEscape in your production code? @wangkechun

@wangkechun
Copy link
Author

Do you use HTMLEscape in your production code? @wangkechun

Never, but I want it to be configurable, like https://pkg.go.dev/github.com/json-iterator/go#pkg-variables

@wangkechun
Copy link
Author 

func TestPrefilledArr(t *testing.T) {
	a := &[...]int{1, 2}
	b := &[...]int{1, 2}
	err1 := json.Unmarshal([]byte(`[3]`), a)
	err2 := Unmarshal([]byte(`[3]`), b)
	require.Equal(t, err1, err2)
	require.Equal(t, a, b)
}

=== RUN   TestPrefilledArr
    fuzz_test.go:109: 
                Error Trace:    fuzz_test.go:109
                Error:          Not equal: 
                                expected: &[2]int{3, 0}
                                actual  : &[2]int{3, 2}
                            
                                Diff:
                                --- Expected
                                +++ Actual
                                @@ -2,3 +2,3 @@
                                  (int) 3,
                                - (int) 0
                                + (int) 2
                                 })
                Test:           TestPrefilledArr
--- FAIL: TestPrefilledArr (0.00s)
FAIL
exit status 1

@wangkechun Please move this into a separate issue. Thanks.

done #7

@wangkechun
Copy link
Author

The key order of map

maps in Go are intrinsically unordered, the Go Spec also says you should NOT rely on the traversal order. And for performance reason, the key is not ordered by design.

For unit test, reproducible serialization results are very important.

@PureWhiteWu
Copy link
Collaborator

Do you use HTMLEscape in your production code? @wangkechun

Never, but I want it to be configurable, like https://pkg.go.dev/github.com/json-iterator/go#pkg-variables

I think we can support it if there's real needs for it, but in our investigation, this function is rarely used.
Maybe you can submit another issue if you really need it in your production code.

@PureWhiteWu
Copy link
Collaborator

The key order of map

maps in Go are intrinsically unordered, the Go Spec also says you should NOT rely on the traversal order. And for performance reason, the key is not ordered by design.

For unit test, reproducible serialization results are very important.

I think you should not rely on the undefined behaviour in your unit tests. Instead, you can test your code by marshal and unmarshal it and compare to the original value.
So I think this is not really an issue.

@PureWhiteWu
Copy link
Collaborator

Closed since there's no more replies.
If you still have questions, feel free to comment or open another issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@chenzhuoyu chenzhuoyu

Labels
question Further information is requested
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@PureWhiteWu @wangkechun @chenzhuoyu