Go-github: 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.