Fix "Panic: reflect.Value.Interface: cannot return value obtained fro…

[RangelReale]

…m unexported field or method"

reflect.Value.Interface() can panic, so it is needed to call reflect.Value.CanInterface() to prevent this.

I got this error marshalling a struct with this "tableName" field, which is needed by the go-pg package.

type Project struct {
tableName struct{} sql:"project"

Name      string    `json:"name" groups:"post"`

}

[coveralls]

Coverage increased (+0.2%) to 81.928% when pulling 8beb899 on RangelReale:fix_interface into 73de227 on liip:master.

[coveralls]

Coverage increased (+0.2%) to 81.928% when pulling df73e2a on RangelReale:fix_interface into 73de227 on liip:master.

[coveralls]

Coverage increased (+0.6%) to 82.266% when pulling 9c8ab07 on RangelReale:fix_interface into 73de227 on liip:master.

[mweibel]

Thanks for your PR! 👍
I added some nitpick comments. What I'm a bit uneasy about is this bypassing of CanInterface.
Can you elaborate why this is needed? Should we rather open an issue for Go to fix CanInterface?
Is the bypassCanInterface production-ready code?

[mweibel]

This enables us to call fmt.Printf on a value even if it's derived

We don't use it for fmt.Printf or am I missing something? I assume this is a verbatim copy of the comment in deepequal. I think we should adapt that comment to match our case.
Also this sentence is wrong as it's in Go already:

for a possible future alternative to this hack.

[mweibel]

unsure if we should not just use this value because we don't test sheriff in older versions than 1.6 anyway.

[mweibel]

go vet says this is an error because json tag in unexported field name. The test must actually test this specific case, right?

./sheriff_test.go:469: struct field tableName has json tag but is not exported
(https://travis-ci.org/liip/sheriff/jobs/297684025)

Seems like we need to switch to specific flags only.. I can handle this later on.

[RangelReale]

Well, to be fair, I really didn't understand what the source from deepequal.go was doing, as I am not too versed on the reflect package.
I had 2 problems that made me do this PR:

• The "tableName struct{}" field was panicking on ".Interface()", because it is an unexported field. To solve this, I first called ".CanInterface()" before calling "marshalValue", and it solved this problem. The "json" tag was to prevent returning a "": "" on the json. (This member is needed for the go-pg library, see https://github.com/go-pg/pg/wiki/Model-Definition)
• The nil-pointer field was also panicking on ".Interface()", I think this was a previous bug.
• Bug calling ".CanInterface()" on a nil-pointer field returns false, but on this case we want to return a nil interface, not skipping it.

To solve these problems, I tried calling ".IsValid()" which is supposed to tell if a value is "the zero value". But for the "tableName" field, IsValid=true and CanInterface=false, so it didn't work in all cases. I tried all kinds of combinations, but failed to find one valid for all cases.

Looking around, this seems to be an inconsitency (maybe bug) of the reflect package.
The code I copied from had the exact same problem, and used some black magic to "fix" the reflect bug. As the repository is from "juju", a well-known Go user, I thought that this was the best way to fix it.
Where is the commit they did this:
juju/testing@9797b41#diff-4175905a405f621dd9b2f834b385422f

The TestInlineStruct in my sheriff_test.go contains all the problems I appointed, maybe it can be easier for you to just copy this test in your branch and try to fix it.

[RangelReale]

I made another try to fix this, it solved the 2 problems I had, see if you think this is better.

https://github.com/RangelReale/sheriff/tree/fix_interface2

[mweibel]

That looks much simpler indeed. I wonder if we can do an continue instead of wrapping everything in the if and indent everything even more, no?

I tried also with your branch here now:

diff --git a/sheriff.go b/sheriff.go
index 1dd957a..5b57987 100644
--- a/sheriff.go
+++ b/sheriff.go
@@ -227,10 +227,14 @@ func listContains(a []string, b []string) bool {
 // for a possible future alternative to this hack.
 // from https://github.com/juju/testing/blob/master/checkers/deepequal.go
 func interfaceOf(v reflect.Value) interface{} {
-       if !v.IsValid() {
+       if !v.IsValid() || !v.CanInterface() {
                return nil
        }
-       return bypassCanInterface(v).Interface()
+       //return bypassCanInterface(v).Interface()
+       //if !v.CanInterface() {
+       //      return nil
+       //}
+       return v.Interface()
 }

 type flag uintptr
diff --git a/sheriff_test.go b/sheriff_test.go
index 6408b4a..9bed4d1 100644
--- a/sheriff_test.go
+++ b/sheriff_test.go
@@ -474,6 +474,7 @@ type TestInlineStruct struct {

 func TestMarshal_InlineStruct(t *testing.T) {
        v := TestInlineStruct{
+               tableName: struct{}{},
                Field:  "World",
                Field2: nil,
        }

This works without panic, which case would be where it should panic, if using CanInterface?
Tried also in the test with omitting tableName, or making it a pointer etc. and worked always.

[RangelReale]

This does not panic, but skips nil pointer fields, instead of returning "nil" on the output map. Nil pointer fields should output "null" on json, not failing to appear altogether.

.IsValid returns false for nil values, this is the problem I was trying to solve too.

[mweibel]

This does not panic, but skips nil pointer fields, instead of returning "nil" on the output map. Nil pointer fields should output "null" on json, not failing to appear altogether.

ah, true. Depends on omitempty though.

.IsValid returns false for nil values, this is the problem I was trying to solve too.

I see. Ok. Will think a bit about it, thanks!

[mweibel]

diff --git a/sheriff.go b/sheriff.go
index 843101a..d3bb76d 100644
--- a/sheriff.go
+++ b/sheriff.go
@@ -144,6 +144,9 @@ func Marshal(options *Options, data interface{}) (interface{}, error) {
 //
 // There is support for types implementing the Marshaller interface, arbitrary structs, slices, maps and base types.
 func marshalValue(options *Options, v reflect.Value) (interface{}, error) {
+       if !v.IsValid() {
+               return nil, nil
+       }
        val := v.Interface()

        if marshaller, ok := val.(Marshaller); ok {

I added only this change and your added test doesn't panic anymore. Additionally it outputs the following map (when adding a fmt.Printf("%#v", actualMap) after calling Marshal(o, v)):

map[string]interface {}{"field":"World", "field2":interface {}(nil)}

This looks correct to me, is there something I'm missing still?

//Edit: ok I see, if I set the json tag of the unexported field to something else than - it panics still. Will investigate.

[mweibel]

@RangelReale Can you test if #6 works for you?