tests/fields: False positive for fields with null values, reported as "missing" incorrectly.

[dmitshur]

There appears to be a bug in the ./tests/fields test/utility, causing some fields to be falsely reported as "missing".

For example, in the following test case:

{"repos/google/go-github/issues/1", &github.Issue{}},

The real GitHub API response from https://api.github.com/repos/google/go-github/issues/1 includes:

...
    "site_admin": false
    }
  ],
  "milestone": null,
  "comments": 2,
  "created_at": "2013-05-30T18:35:59Z",
  "updated_at": "2013-08-20T22:47:14Z",
...

Note that the value of milestone is null.

The corresponding struct field Issue.Milestone exists:

https://github.com/google/go-github/blob/d9c241b9c95cbd5996418f70223328a37cfb9bb3/github/issues.go#L42

But the output from the fields test incorrectly reports:

...
*github.Issue missing field for key: milestone (example value: <nil>)
...

I think it's because the null value gets dropped when JSON marshalling the Issue struct, because of ,omitempty option.

[dmitshur]

There are multiple approaches to solving the issue. I can't think of one without problems.

Essentially, we want to either modify the struct being marshaled into JSON to strip its "omitempty" JSON options. Alternatively, we can use reflection and (re)implement encoding/json rules for mapping struct fields to JSON keys.

I've prototyped the latter, and it works (milestone key is no longer incorrectly reported as missing), but it's not cleaned up and not finished. Here is a WIP patch for reference. See TODOs for limitations.

commit 6883f03201546121013b6d67674a7451f87fe639
Author: Dmitri Shuralyov <[email protected]>
Date:   Sat Mar 4 22:26:16 2017 -0500

    WIP: tests/fields: Fix #576.
    
    TODO: Decide on the high-level approach, clean up, finish.

diff --git a/tests/fields/fields.go b/tests/fields/fields.go
index bd1463e..bc6d417 100644
--- a/tests/fields/fields.go
+++ b/tests/fields/fields.go
@@ -59,12 +59,13 @@ func main() {
 		typ interface{}
 	}{
 		//{"rate_limit", &github.RateLimits{}},
-		{"users/octocat", &github.User{}},
-		{"user", &github.User{}},
-		{"users/willnorris/keys", &[]github.Key{}},
-		{"orgs/google-test", &github.Organization{}},
-		{"repos/google/go-github", &github.Repository{}},
-		{"/gists/9257657", &github.Gist{}},
+		//{"users/octocat", &github.User{}},
+		//{"user", &github.User{}},
+		//{"users/willnorris/keys", &[]github.Key{}},
+		//{"orgs/google-test", &github.Organization{}},
+		//{"repos/google/go-github", &github.Repository{}},
+		{"repos/google/go-github/issues/1", &github.Issue{}},
+		//{"/gists/9257657", &github.Gist{}},
 	} {
 		err := testType(tt.url, tt.typ)
 		if err != nil {
@@ -106,31 +107,25 @@ func testType(urlStr string, typ interface{}) error {
 		}
 	}
 
-	// unmarshal to typ first, then re-marshal and unmarshal to a map
-	err = json.Unmarshal(*raw, typ)
-	if err != nil {
-		return err
-	}
-
-	var byt []byte
-	if slice {
-		// use first item in slice
+	// Populate m2 with all fields from typ,
+	// using encoding/json rules for mapping struct fields to JSON key names.
+	var m2 = make(map[string]struct{})
+	{
 		v := reflect.Indirect(reflect.ValueOf(typ))
-		byt, err = json.Marshal(v.Index(0).Interface())
-		if err != nil {
-			return err
+		if slice {
+			v = v.Index(0)
 		}
-	} else {
-		byt, err = json.Marshal(typ)
-		if err != nil {
-			return err
+		t := v.Type()
+		if t.Kind() != reflect.Struct {
+			return fmt.Errorf("typ is %v (%T), expected a struct", typ, typ)
+		}
+		for i := 0; i < t.NumField(); i++ {
+			name, ok := structFieldToJSONKeyName(t.Field(i))
+			if !ok {
+				continue
+			}
+			m2[name] = struct{}{}
 		}
-	}
-
-	var m2 map[string]interface{}
-	err = json.Unmarshal(byt, &m2)
-	if err != nil {
-		return err
 	}
 
 	// now compare the two maps
@@ -145,3 +140,29 @@ func testType(urlStr string, typ interface{}) error {
 
 	return nil
 }
+
+// structFieldToJSONKeyName returns JSON key name that the struct field maps to, if any,
+// according to encoding/json rules.
+// TODO: This doesn't fully include embedded structs...
+// TODO: This also doesn't fully implement all encoding/json rules. For example,
+//       there's no valid tag check (isValidTag), etc. If encoding/json rules change,
+//       this code will be out of date, since it duplicates its private functionality.
+//       However, this might be sufficient (good enough) for the limited purposes of this tool.
+func structFieldToJSONKeyName(field reflect.StructField) (string, bool) {
+	if field.PkgPath != "" && !field.Anonymous { // unexported
+		// json.Unmarshal will only set exported fields of the struct.
+		return "", false
+	}
+	tag, ok := field.Tag.Lookup("json")
+	if !ok {
+		return field.Name, true
+	}
+	if tag == "-" {
+		return "", false
+	}
+	name := strings.Split(tag, ",")[0]
+	if name == "" {
+		return "", false
+	}
+	return name, true
+}

[FenwickElliott]

I'm new to go, have been using this project and I'd like to contribute. Can I help with finishing off this issue?

Thanks,

[dmitshur]

You're certainly welcome to, thanks!

This is a hard issue. If you think you have ideas for how to tackle it, I suggest you discuss them here before sending code for review. That way, we can make smaller steps and see whether they're headed in the right direction to resolve the issue.

[FenwickElliott]

Thanks for getting back to me.

I'm very new to Go (and OS contributing) so I might not succeed. At first read it looked like you had figured out the solution and just needed someone to do the leg work which is why I thought it looked doable.

I'll definitely check in before submitting any code.

Thanks,

[dmitshur]

I'm very new to Go (and OS contributing) so I might not succeed.

No problem, that's completely acceptable. Even if you don't succeed, you'll have tried, perhaps learned something, and maybe will succeed in the end. If you only start things you know you'll succeed at, you won't be able to start many things.

At first read it looked like you had figured out the solution

On the contrary, I gave it a shot, and had a rough prototype, but see the TODOs, many of them are "figure out the general high-level approach", etc. So figuring out the exact plan is still a task to be done.

I'll definitely check in before submitting any code.

Sounds great!

[FenwickElliott]

Thank you for your encouragement.

I take your point though, this is proving harder than I thought. I'm going to keep trying but I wouldn't suggest anyone hold their breath for my fix.

Really, thank you.

[gmlewis]

This would be a great PR for any new contributor to this repo or a new Go developer. All contributions are greatly appreciated!

Feel free to volunteer for any issue and the issue can be assigned to you so that others don't attempt to duplicate the work.

Please check out our CONTRIBUTING.md guide to get started. (In particular, please remember to go generate ./... and don't use force-push to your PRs.)

Thank you!

[sagar23sj]

Hi @gmlewis. I would like to give this issue a try

[gmlewis]

Hi @gmlewis. I would like to give this issue a try

Thank you, @sagar23sj , it is yours.

[VUHAILAM]

Hi @gmlewis and @dmitshur, Could you please let me know this issue has solved or updated? I would like to work on this issue!! Thank you!

[gmlewis]

@VUHAILAM - this issue is yours. Thank you!

[VUHAILAM]

Hi @dmitshur , my this problem understanding that we have a function testType which helps to verify the live Github API data types and self-define data types. And when unmarshalling to map, some fields are missed because omitempty option. I have some questions: Should we need to identify and ensure data fields of nested structs? And Could you please provide me some encoding/json rules examples in go-github ? Thank you!!

[gmlewis]

@VUHAILAM - I'm not sure why I ever labeled this as a "good first issue"... that was a mistake.

Looking back at the comments, Dmitri says above that this is a hard issue, and if you wish to withdraw, I'll remove your name from the issue.

[VUHAILAM]

@gmlewis I think we should break down this issue. I prepared some code can help to solve for the general data types in go-github.

func getMapJsonKeyName(typ interface{}) map[string]interface{} {
	var structType reflect.Type
	if isSlice := reflect.Indirect(reflect.ValueOf(typ)).Kind() == reflect.Slice; isSlice {
		structType = reflect.Indirect(reflect.ValueOf(typ)).Index(0).Type()
	} else {
		structType = reflect.Indirect(reflect.ValueOf(typ)).Type()
	}

	fieldsMap := make(map[string]interface{})

	for i := 0; i < structType.NumField(); i++ {
		name, ok := getJsonKeyName(structType.Field(i))
		if !ok {
			continue
		}

		if structType.Field(i).Type().Kind() == reflect.Struct {
			fieldsMap[name] = getMapJsonKeyName(structType.Field(i).Type())
		} else {
			fieldsMap[name] = struct{}{}
		}
	}
	return fieldsMap
}

func compareFieldsMap(m1 map[string]interface{}, fieldsMap map[string]interface{}, typ interface{}) {
	for k, v := range m1 {
		if *skipURLs && strings.HasSuffix(k, "_url") {
			continue
		}
		if _, ok := fieldsMap[k]; !ok {
			fmt.Printf("%v missing field for key: %v (example value: %v)\n", reflect.TypeOf(typ), k, v)
		} else {
			if reflect.ValueOf(v).Kind() == reflect.Map {
				compareFieldsMap(v.(map[string]interface{}), fieldsMap[k].(map[string]interface{}), typ)
			}
		}
	}
}

func getJsonKeyName(field reflect.StructField) (string, bool) {
	if !field.IsExported() && !field.Anonymous {
		return "", false
	}

	tag, ok := field.Tag.Lookup("json")
	if !ok {
		return field.Name, true
	}

	if tag == "-" {
		return "", false
	}

	name := strings.Split(tag, ",")[0]
	if name == "" {
		return "", false
	}

	return name, true
}

Could you please guilde me the next step. I think I can work on it or at least I can solve the basic of this issue. Thank you!!

[gmlewis]

I won't be able to dig deep into this issue until at least the beginning of the new year. It is a 5-year-old issue and is probably not a very high priority, so I would say to give it your best shot and report your findings to hopefully move the issue forward somewhat. Thanks.