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.

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

Fix JSON Parsing #28

Merged
merged 2 commits into from
Dec 4, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 0 additions & 85 deletions any_test.go

This file was deleted.

9 changes: 3 additions & 6 deletions http/response.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,18 +67,15 @@ func (res *Response) RawNext() (interface{}, error) {
}
}

a := &cmds.Any{}
a.Add(&cmdkit.Error{})
a.Add(res.req.Command().Type)

err := res.dec.Decode(a)
m := &cmds.MaybeError{Value: res.req.Command().Type}
err := res.dec.Decode(m)

// last error was sent as value, now we get the same error from the headers. ignore and EOF!
if err != nil && res.err != nil && err.Error() == res.err.Error() {
err = io.EOF
}

return a.Interface(), err
return m.Get(), err
}

func (res *Response) Next() (interface{}, error) {
Expand Down
111 changes: 111 additions & 0 deletions maybeerror_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
package cmds

import (
"encoding/json"
"io"
"reflect"
"strings"
"testing"

"github.com/ipfs/go-ipfs-cmdkit"
)

type Foo struct {
Bar int
}

type Bar struct {
Foo string
}

type ValueError struct {
Error error
Value interface{}
}

type anyTestCase struct {
Value interface{}
JSON string
Decoded []ValueError
}

func TestMaybeError(t *testing.T) {
testcases := []anyTestCase{
anyTestCase{
Value: &Foo{},
JSON: `{"Bar":23}{"Bar":42}{"Message":"some error", "Type": "error"}`,
Decoded: []ValueError{
ValueError{Error: nil, Value: &Foo{23}},
ValueError{Error: nil, Value: &Foo{42}},
ValueError{Error: nil, Value: cmdkit.Error{Message: "some error", Code: 0}},
},
},
anyTestCase{
Value: Foo{},
JSON: `{"Bar":23}{"Bar":42}{"Message":"some error", "Type": "error"}`,
Decoded: []ValueError{
ValueError{Error: nil, Value: &Foo{23}},
ValueError{Error: nil, Value: &Foo{42}},
ValueError{Error: nil, Value: cmdkit.Error{Message: "some error", Code: 0}},
},
},
anyTestCase{
Value: &Bar{},
JSON: `{"Foo":""}{"Foo":"Qmabc"}{"Message":"some error", "Type": "error"}`,
Decoded: []ValueError{
ValueError{Error: nil, Value: &Bar{""}},
ValueError{Error: nil, Value: &Bar{"Qmabc"}},
ValueError{Error: nil, Value: cmdkit.Error{Message: "some error", Code: 0}},
},
},
anyTestCase{
Value: Bar{},
JSON: `{"Foo":""}{"Foo":"Qmabc"}{"Message":"some error", "Type": "error"}`,
Decoded: []ValueError{
ValueError{Error: nil, Value: &Bar{""}},
ValueError{Error: nil, Value: &Bar{"Qmabc"}},
ValueError{Error: nil, Value: cmdkit.Error{Message: "some error", Code: 0}},
},
},
}

for _, tc := range testcases {
m := &MaybeError{Value: tc.Value}

r := strings.NewReader(tc.JSON)
d := json.NewDecoder(r)

var err error

for _, dec := range tc.Decoded {
err = d.Decode(m)
if err != dec.Error {
t.Fatalf("error is %v, expected %v", err, dec.Error)
}

rx := m.Get()
rxIsPtr := reflect.TypeOf(rx).Kind() == reflect.Ptr

ex := dec.Value
exIsPtr := reflect.TypeOf(ex).Kind() == reflect.Ptr

if rxIsPtr != exIsPtr {
t.Fatalf("value is %#v, expected %#v", m.Get(), dec.Value)
}

if rxIsPtr {
rx = reflect.ValueOf(rx).Elem().Interface()
ex = reflect.ValueOf(ex).Elem().Interface()
}

if rx != ex {
t.Fatalf("value is %#v, expected %#v", m.Get(), dec.Value)
}
}

err = d.Decode(m)
if err != io.EOF {
t.Fatal("data left in decoder:", m.Get())
}
}
}
105 changes: 23 additions & 82 deletions writer.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,34 +67,24 @@ func (r *readerResponse) Length() uint64 {
}

func (r *readerResponse) RawNext() (interface{}, error) {
a := &Any{}
a.Add(cmdkit.Error{})
a.Add(r.req.Command().Type)

err := r.dec.Decode(a)
m := &MaybeError{Value: r.req.Command().Type}
err := r.dec.Decode(m)
if err != nil {
return nil, err
}

r.once.Do(func() { close(r.emitted) })

v := a.Interface()
v := m.Get()
return v, nil
}

func (r *readerResponse) Next() (interface{}, error) {
a := &Any{}
a.Add(cmdkit.Error{})
a.Add(r.req.Command().Type)

err := r.dec.Decode(a)
v, err := r.RawNext()
if err != nil {
return nil, err
}

r.once.Do(func() { close(r.emitted) })

v := a.Interface()
if err, ok := v.(cmdkit.Error); ok {
v = &err
}
Expand Down Expand Up @@ -177,81 +167,32 @@ func (re *WriterResponseEmitter) Emit(v interface{}) error {
return re.enc.Encode(v)
}

type Any struct {
types map[reflect.Type]bool
order []reflect.Type
type MaybeError struct {
Value interface{} // needs to be a pointer
Error cmdkit.Error
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we not assume that it's one or the other? (i.e., if Error is nil, it's a value)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, Error is a struct so I can't check for nil. I could check if the message is empty but that seems like a bad hack. Checking whether Value is nil won't work because it's supposed to be set to a non-nil value by the user. So inferring whether we have an error or not is not possible in a clean way without isError.

I think the danger of incosistencies (i.e. isError is false but we in fact do have an error) is very small, so I'd like to stick with it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Sounds good.


v interface{}
isError bool
}

func (a *Any) UnmarshalJSON(data []byte) error {
var (
iv interface{}
err error
)

for _, t := range a.order {
v := reflect.New(t).Elem().Addr()

isNil := func(v reflect.Value) (yup, ok bool) {
ok = true
defer func() {
r := recover()
if r != nil {
ok = false
}
}()
yup = v.IsNil()
return
}

isZero := func(v reflect.Value, t reflect.Type) (yup, ok bool) {
ok = true
defer func() {
r := recover()
if r != nil {
ok = false
}
}()
yup = v.Elem().Interface() == reflect.Zero(t).Interface()
return
}

err = json.Unmarshal(data, v.Interface())

vIsNil, isNilOk := isNil(v)
vIsZero, isZeroOk := isZero(v, t)

nilish := (isNilOk && vIsNil) || (isZeroOk && vIsZero)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like we do actually need this logic.

Copy link
Contributor Author

@keks keks Dec 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get it. Why? nevermind.

if err == nil && !nilish {
a.v = v.Interface()
return nil
}
func (m *MaybeError) Get() interface{} {
if m.isError {
return m.Error
}

err = json.Unmarshal(data, &iv)
a.v = iv

return err
return m.Value
}

func (a *Any) Add(v interface{}) {
if v == nil {
return
}
if a.types == nil {
a.types = map[reflect.Type]bool{}
}
t := reflect.TypeOf(v)
isPtr := t.Kind() == reflect.Ptr
if isPtr || t.Kind() == reflect.Interface {
t = t.Elem()
func (m *MaybeError) UnmarshalJSON(data []byte) error {
err := json.Unmarshal(data, &m.Error)
if err == nil {
m.isError = true
return nil
}

a.types[t] = isPtr
a.order = append(a.order, t)
}
// make sure we are working with a pointer here
v := reflect.ValueOf(m.Value)
if v.Kind() != reflect.Ptr {
m.Value = reflect.New(v.Type()).Interface()
}

func (a *Any) Interface() interface{} {
return a.v
return json.Unmarshal(data, m.Value)
}