about testJSONMarshal's behavior #2699

exageraldo · 2023-03-06T20:58:39Z

When I was investigating issue #2679, I faced the following situation:

After changing the tags in the ListSCIMProvisionedIdentitiesOptions structure, from json to url, I was expecting the TestListSCIMProvisionedIdentitiesOptions_Marshal test to fail, but it didn't.

func TestListSCIMProvisionedIdentitiesOptions_Marshal(t *testing.T) {
	testJSONMarshal(t, &ListSCIMProvisionedIdentitiesOptions{}, `{}`)

	u := &ListSCIMProvisionedIdentitiesOptions{
		StartIndex: Int(1),
		Count:      Int(10),
		Filter:     String("test"),
	}

	want := `{
		"startIndex": 1,
		"count": 10,
	 	"filter": "test"
	}`

	testJSONMarshal(t, u, want)
}

It made sense that this test was passing before the change, because the structure was in accordance with the defined tags. However, after the change the tests kept passing.

// BEFORE
type ListSCIMProvisionedIdentitiesOptions struct {
	StartIndex *int    `json:"startIndex,omitempty"`
	Count      *int    `json:"count,omitempty"` 
	Filter     *string `json:"filter,omitempty"`
}

// AFTER
type ListSCIMProvisionedIdentitiesOptions struct {
	StartIndex *int    `url:"startIndex,omitempty"`
	Count      *int    `url:"count,omitempty"` 
	Filter     *string `url:"filter,omitempty"`
}

If we add a few more test cases, we can see that regardless of how the letters are (upper or lower case), the tests always pass (both using the json and url tags).

I created a temporary function with the test cases that should fail, the TestListSCIMProvisionedIdentitiesOptions_Marshal_MustFailed function, but all are passing so far.

func TestListSCIMProvisionedIdentitiesOptions_Marshal(t *testing.T) {
	// Case one: Empty struct
	testJSONMarshal(t, &ListSCIMProvisionedIdentitiesOptions{}, `{}`)

	// Case two: Happy case. The fields are as described in the structure tags
	u := &ListSCIMProvisionedIdentitiesOptions{
		StartIndex: Int(1),
		Count:      Int(10),
		Filter:     String("test two"),
	}

	want := `{
		"startIndex": 1,
		"count": 10,
	 	"filter": "test two"
	}`

	testJSONMarshal(t, u, want)
}

func TestListSCIMProvisionedIdentitiesOptions_Marshal_MustFailed(t *testing.T) {
	u := &ListSCIMProvisionedIdentitiesOptions{
		StartIndex: Int(1),
		Count:      Int(10),
	}

	// Case three: The json fields are like the field names in the struct
	u.Filter = String("test three")
	want := `{
		"StartIndex": 1,
		"Count": 10,
	 	"Filter": "test three"
	}`
	testJSONMarshal(t, u, want)

	// Case four: All the letters are in lower case.
	u.Filter = String("test four")
	want = `{
		"startindex": 1,
		"count": 10,
	 	"filter": "test four"
	}`
	testJSONMarshal(t, u, want)

	// Case five: All the letters are in upper case.
	u.Filter = String("test five")
	want = `{
		"STARTINDEX": 1,
		"COUNT": 10,
	 	"FILTER": "test five"
	}`
	testJSONMarshal(t, u, want)
}

To better understand this behavior, we need to see how the testJSONMarshal works.

// Test whether the marshaling of v produces JSON that corresponds
// to the want string.
func testJSONMarshal(t *testing.T, v interface{}, want string) {
	t.Helper()
	// Unmarshal the wanted JSON, to verify its correctness, and marshal it back
	// to sort the keys.
	u := reflect.New(reflect.TypeOf(v)).Interface()
	if err := json.Unmarshal([]byte(want), &u); err != nil {
		t.Errorf("Unable to unmarshal JSON for %v: %v", want, err)
	}
	w, err := json.Marshal(u)
	if err != nil {
		t.Errorf("Unable to marshal JSON for %#v", u)
	}

	// Marshal the target value.
	j, err := json.Marshal(v)
	if err != nil {
		t.Errorf("Unable to marshal JSON for %#v", v)
	}

	if string(w) != string(j) {
		t.Errorf("json.Marshal(%q) returned %s, want %s", v, j, w)
	}
}

As described in the comments in the code, these are the steps performed by this test:

Unmarshal the wanted JSON, to verify its correctness
Marshal it back to sort the keys
Marshal the target value
Compare the two strings

The value of want is used (directly) only once to parse the JSON (json.Unmarshal). After that it goes the other way (json.Marshal) to generate a new JSON (w []byte) and it is this new slice of byte that is used for comparison with the value of the passed structure (j []byte).

An important point in the json.Unmarshal documentation is:

To unmarshal JSON into a struct, Unmarshal matches incoming object keys to the keys used by Marshal (either the struct field name or its tag), preferring an exact match but also accepting a case-insensitive match.

That is, when we do json.Unmarshal([]byte(want), &u), we are building a structure, giving preference to exact match, but also accepting other cases, as the above cases show. This can become a problem, since the GitHub API is case sensitive. If we switch any case letters, the value sent will be ignored and the default value for that field will be kept.

To simplify it a bit more and also to have better control of the tests, I suggest that this function has two steps:

Marshal the target value
Compare the two slices of byte

My initial suggestion is something like this:

// ...
// Docs
// ...
func testJSONMarshal(t *testing.T, v interface{}, want string) {
	t.Helper()

	j, err := json.Marshal(v)
	if err != nil {
		t.Errorf("Unable to unmarshal JSON for %v: %v", v, err)
	}

	if !bytes.Equal(j, []byte(want)) {
		t.Errorf("json.Marshal(%+v) returned %s, want %s", v, j, want)
	}
}

Some comments about the change:

The function's return and signature will be kept
The only thing that will change a little is the value of the expected result (the "want" value)
- Must not contain extra spaces between key and value ({"key":"value"})
- Must be written on a single line ({"some_key":"value","other_key":"other_value"})
- The fields must be in the same order as they were defined in the structure that is being tested
This test will compare exactly the two slices of byte (bytes.Equal(j, []byte(want))). The idea is to compare the value generated by json.Marshal(v) is exactly equal to the expected value (want).

The way of writing the expected result may be a bit longer in some cases, but it will be much more explicit and predictable.

We can update our TestListSCIMProvisionedIdentitiesOptions_Marshal and TestListSCIMProvisionedIdentitiesOptions_Marshal_MustFailed tests. The only thing we need to do is put the expected value (want) on a single line, without extra spaces:

func TestListSCIMProvisionedIdentitiesOptions_Marshal(t *testing.T) {
	testJSONMarshal(t, &ListSCIMProvisionedIdentitiesOptions{}, `{}`)

	u := &ListSCIMProvisionedIdentitiesOptions{
		StartIndex: Int(1),
		Count:      Int(10),
		Filter:     String("test two"),
	}

	want := `{"startIndex":1,"count":10,"filter":"test two"}`
	testJSONMarshal(t, u, want)
}

func TestListSCIMProvisionedIdentitiesOptions_Marshal_MustFailed(t *testing.T) {
	u := &ListSCIMProvisionedIdentitiesOptions{
		StartIndex: Int(1),
		Count:      Int(10),
	}

	u.Filter = String("test three")
	want := `{"StartIndex":1,"Count":10,"Filter":"test three"}`
	testJSONMarshal(t, u, want)

	u.Filter = String("test four")
	want = `{"startindex":1,"count":10,"filter":"test four"}`
	testJSONMarshal(t, u, want)

	u.Filter = String("test five")
	want = `{"STARTINDEX":1,"COUNT":10,"FILTER":"test five"}`
	testJSONMarshal(t, u, want)
}

When we run the tests using the json tags in the structure, we notice that the cases inside TestListSCIMProvisionedIdentitiesOptions_Marshal pass successfully and the cases that should fail, actually fail. This is exactly what we expect!

OUTPUT:
--- FAIL: TestListSCIMProvisionedIdentitiesOptions_Marshal_MustFailed (0.00s)
    go-github/github/scim_test.go: json.Marshal(&{StartIndex:0xc000028878 Count:0xc000028880 Filter:0xc0001af160}) returned {"startIndex":1,"count":10,"filter":"test three"}, want {"StartIndex":1,"Count":10,"Filter":"test three"}
    go-github/github/scim_test.go: json.Marshal(&{StartIndex:0xc000028878 Count:0xc000028880 Filter:0xc0001af310}) returned {"startIndex":1,"count":10,"filter":"test four"}, want {"startindex":1,"count":10,"filter":"test four"}
    go-github/github/scim_test.go: json.Marshal(&{StartIndex:0xc000028878 Count:0xc000028880 Filter:0xc0001af350}) returned {"startIndex":1,"count":10,"filter":"test five"}, want {"STARTINDEX":1,"COUNT":10,"FILTER":"test five"}
FAIL
FAIL	github.com/google/go-github/v50/github	0.578s
FAIL

By changing the tags to url, we can see that all cases within TestListSCIMProvisionedIdentitiesOptions_Marshal fail, and this is a good sign.

OUTPUT:
--- FAIL: TestListSCIMProvisionedIdentitiesOptions_Marshal (0.00s)
    go-github/github/scim_test.go: json.Marshal(&{StartIndex:<nil> Count:<nil> Filter:<nil>}) returned {"StartIndex":null,"Count":null,"Filter":null}, want {}
    go-github/github/scim_test.go: json.Marshal(&{StartIndex:0xc0000a4888 Count:0xc0000a4890 Filter:0xc0001612c0}) returned {"StartIndex":1,"Count":10,"Filter":"test two"}, want {"startIndex":1,"count":10,"filter":"test two"}
FAIL
FAIL	github.com/google/go-github/v50/github	0.606s
FAIL

In TestListSCIMProvisionedIdentitiesOptions_Marshal_MustFailed we have one test case passing, which is the case when all fields in the json are equal to the fields in the structure. In this case this result is really expected.

OUTPUT:
--- FAIL: TestListSCIMProvisionedIdentitiesOptions_Marshal_MustFailed (0.00s)
    go-github/github/scim_test.go: json.Marshal(&{StartIndex:0xc0000a47e0 Count:0xc0000a47e8 Filter:0xc00015b2b0}) returned {"StartIndex":1,"Count":10,"Filter":"test four"}, want {"startindex":1,"count":10,"filter":"test four"}
    go-github/github/scim_test.go: json.Marshal(&{StartIndex:0xc0000a47e0 Count:0xc0000a47e8 Filter:0xc00015b2f0}) returned {"StartIndex":1,"Count":10,"Filter":"test five"}, want {"STARTINDEX":1,"COUNT":10,"FILTER":"test five"}
FAIL
FAIL	github.com/google/go-github/v50/github	0.286s
FAIL

Another curious case we can look at is the TestUpdateAttributeForSCIMUserOptions_Marshal. If we run our new version of testJSONMarshal, the test will break.

func TestUpdateAttributeForSCIMUserOptions_Marshal(t *testing.T) {
	testJSONMarshal(t, &UpdateAttributeForSCIMUserOptions{}, `{}`)

	u := &UpdateAttributeForSCIMUserOptions{
		Schemas: []string{"test", "schema"},
		Operations: UpdateAttributeForSCIMUserOperations{
			Op:   "TestOp",
			Path: String("path"),
		},
	}

	want := `{
		"schemas": ["test", "schema"],
		"operations": {
			"op": "TestOp",
			"path": "path"
		}
	}`

	testJSONMarshal(t, u, want)
}

We already expected the second case to fail, because we still need to put the whole structure on a single line, but the first case also failed.

OUTPUT:
--- FAIL: TestUpdateAttributeForSCIMUserOptions_Marshal (0.00s)
    go-github/github/scim_test.go:411: json.Marshal(&{Schemas:[] Operations:{Op: Path:<nil> Value:[]}}) returned {"operations":{"op":""}}, want {}
    go-github/github/scim_test.go:429: json.Marshal(&{Schemas:[test schema] Operations:{Op:TestOp Path:0xc00022b300 Value:[]}}) returned {"schemas":["test","schema"],"operations":{"op":"TestOp","path":"path"}}, want {
        		"schemas": ["test", "schema"],
        		"operations": {
        			"op": "TestOp",
        			"path": "path"
        		}
        	}
FAIL
FAIL	github.com/google/go-github/v50/github	0.867s
FAIL

We are expecting {} and the result is {"operations":{"op":""}}, but if we look at the UpdateAttributeForSCIMUserOptions and UpdateAttributeForSCIMUserOperations structures, it makes a lot of sense.

type UpdateAttributeForSCIMUserOptions struct {
	Schemas    []string                             `json:"schemas,omitempty"` // (Optional.)
	Operations UpdateAttributeForSCIMUserOperations `json:"operations"`        // Set of operations to be performed. (Required.)
}

type UpdateAttributeForSCIMUserOperations struct {
	Op    string          `json:"op"`              // (Required.)
	Path  *string         `json:"path,omitempty"`  // (Optional.)
	Value json.RawMessage `json:"value,omitempty"` // (Optional.)
}

The Op field, within UpdateAttributeForSCIMUserOperations, is a required field and will not be omitted if it is empty. Thus, we can rewrite our test as follows:

func TestUpdateAttributeForSCIMUserOptions_Marshal(t *testing.T) {
	testJSONMarshal(t, &UpdateAttributeForSCIMUserOptions{}, `{"operations":{"op":""}}`)

	u := &UpdateAttributeForSCIMUserOptions{
		Schemas: []string{"test", "schema"},
		Operations: UpdateAttributeForSCIMUserOperations{
			Op:   "TestOp",
			Path: String("path"),
		},
	}

	want := `{"schemas":["test","schema"],"operations":{"op":"TestOp","path":"path"}}`

	testJSONMarshal(t, u, want)
}

There are 410 test functions ending with _Marshal in 97 different files. Most likely all these tests will be affected by the change.

By making this change and running the tests, about 420 test cases failed. This is a large number, but the vast majority of them will be solved just by formatting the expected value.

I would like to open up these ideas for discussion.
What do you think about this?

I'd be very happy to contribute to this issue.

The text was updated successfully, but these errors were encountered:

gmlewis · 2023-03-07T19:32:14Z

OK, wow. Thanks for the detailed write-up, @exageraldo !

I believe I might be the original author of testJSONMarshal (this could be verified but is probably not a big deal either way)...
so I take full responsibility for this situation.

I actually thought I was being clever at the time and also thought:

it was easier on the test writers to order fields any way they thought made the most sense
it was easier on the test writers and maintainers to not have to futz with whitespace issues in the "want" section for exact matches

but never researched the behavior of json.Marshal that you have pointed out regarding case sensitivity.

So I'm now thinking that maybe my original ideas are not that great compared to the possibility of error creeping into the repo due to case sensitivity issues and the problems you have pointed out.

Therefore, I'm thinking that we should go ahead and fix this situation, and I'm happy for you to tackle it unless we hear from anyone else who has a different opinion.

exageraldo · 2023-03-08T02:13:44Z

That's great!
I've just started to implement this suggestion and will, whenever necessary, bring up relevant points about it.

exageraldo · 2023-03-08T22:00:57Z

I found one more observation that we should add. As we pass both structs and maps (like WorkflowRunBillMap and WorkflowBillMap) to the testJSONMarshal function, the order of writing the expected fields are a bit different.

From a StackOverflow answer:

The json package always orders keys when marshalling. Specifically:
- Maps have their keys sorted lexicographically
- Structs keys are marshalled in the order defined in the struct

exageraldo mentioned this issue Mar 6, 2023

Fix SCIM query params #2680

Merged

exageraldo linked a pull request Mar 15, 2023 that will close this issue

Change testJSONMarshal #2708

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

about testJSONMarshal's behavior #2699

about testJSONMarshal's behavior #2699

exageraldo commented Mar 6, 2023

gmlewis commented Mar 7, 2023

exageraldo commented Mar 8, 2023

exageraldo commented Mar 8, 2023

about testJSONMarshal's behavior #2699

about testJSONMarshal's behavior #2699

Comments

exageraldo commented Mar 6, 2023

gmlewis commented Mar 7, 2023

exageraldo commented Mar 8, 2023

exageraldo commented Mar 8, 2023