Skip to content

Commit

Permalink
Combine DecodeOne and DecodeMany into Decode
Browse files Browse the repository at this point in the history
This commit adds a new function Decode which combines the functionality
of DecodeOne and DecodeMany functions by using reflection. In the same
manner, the new function Encode combines EncodeOne and EncodeMany.

The aim behind combining the two functions into one is to help reduce
the verbosity and duplication of code (especially in the case of
decoding) for every new service method corresponding to an API endpoint
that has to be written in the future (as well as the existing ones).

Examples that describe the problem and the alternatives to the above
solution follow. For the examples the type kitsu.User is used but it
could be any kitsu type. Error handling omitted for brevity.

Ideally, in order to write a new service method we would do:

  req := kitsu.Client.NewRequest("POST", url", body) // 1. Encode body.

  u := new(User)
  kitsu.Client.Do(req, u) // 2a. Decode response body to u.

  // or

  var users []*User
  kitsu.Client.Do(req, users) // 2b. Decode response body to users.

In this ideal version, NewRequest is able to call an Encode function
(inside it) which can encode the *User and []*User types and Do is able
to call a Decode function that can decode to *User and []*User types.

The idea behind the ideal version is that it allows to have one single,
consistent way to write any future service method by having NewRequest
and Do handle encoding and decoding behind the scenes for any kitsu
type.

Our dependency (google/jsonapi) uses different signatures for decoding
single and multiple resources which are used by our functions DecodeOne
and DecodeMany respectively.

Having different signatures for DecodeOne and DecodeMany means that we
need to keep two versions of Do (DoOne and DoMany).

DoOne is like the ideal version:

  u := new(User)
  kitsu.Client.DoOne(req, u)

DoMany needs to know the type of the single element of the slice and
returns []interface{} (because that's what google/jsonapi returns) so
usage is diffrent while verbosity and duplication increases for each
service method that needs to decode many:

  sliceOfInterface := kitsu.Client.DoMany(req, reflect.TypeOf(&User{}))
  users := make([]*User, 0, len(sliceOfInterface))
  // loop with type assertions to append to users

An alternative to having two versions of Do is to separate the decode
functionality from Do like so:

  req := kitsu.Client.NewRequest("POST", url", body) // 1. Encode body.

  resp := kitsu.Client.Do(req) // 2. Simplified Do.
  defer resp.Body.Close()      // 3. Body needs to be closed by caller.

  u := new(User)
  decodeUser(resp.Body, u) // 4a. Decode to u.

  // or

  users := decodeUserList(resp.Body) // 4b. Return users.

Notice that in 4b the function decodeUserList hides the loop with type
assertions for the specific type of User. Thus we need such functions
for each type or to directly have the code in the service method which
adds duplication anyways.

By using reflection to check the type and use the appropriate version
(One or Many) it is possible to handle both cases in one function while
keeping the same signature as in the ideal version. The only difference
is that in the case of many we have to pass the address of the slice so
that the slice header can be changed:

  req := kitsu.Client.NewRequest("POST", url", body) // 1. Encode body.

  u := new(User)
  kitsu.Client.Do(req, u) // 2a. Decode to u.

  // or

  var users []*User
  kitsu.Client.Do(req, &users) // 2b. Notice the added '&' in &users.

The reflection version gets us very close to the ideal version.

The number of Kitsu API endpoinds that have to be potentially supported
by this package is large:

https://docs.google.com/spreadsheets/d/1GsZ9w17ZLfrlb86vlv4vbAuxMPnM9uyoYXJIwKhpBTo/edit#gid=397288529

Reflection is never clear but considering the number of service methods
that have to be written for each API endpoint it seems like a good
trade-off.

That said the reflection code in the new Encode and Decode functions has
to be improved, tested well and proven to be reliable in the long run.
If that is not the case we can swap to one of the alternative solutions
described above.
  • Loading branch information
nstratos committed Apr 24, 2017
1 parent d28b698 commit 8b8706c
Showing 1 changed file with 73 additions and 0 deletions.
73 changes: 73 additions & 0 deletions kitsu/internal/jsonapi/jsonapi.go
@@ -1,12 +1,85 @@
package jsonapi

import (
"fmt"
"io"
"reflect"
"runtime"

"github.com/nstratos/jsonapi"
)

func isZeroOfUnderlyingType(x interface{}) bool {
return reflect.DeepEqual(x, reflect.Zero(reflect.TypeOf(x)).Interface())
}

func Encode(w io.Writer, v interface{}) (err error) {
const errFormat = "cannot encode type %T, need pointer to struct or slice of pointers to structs"
defer func() {
if r := recover(); r != nil {
if _, ok := r.(runtime.Error); ok {
panic(r)
}
err = fmt.Errorf(errFormat+": %v", v, r.(error))
}
}()
if isZeroOfUnderlyingType(v) {
return fmt.Errorf("cannot encode nil value of %#v", v)
}
t := reflect.TypeOf(v)
switch t.Kind() {
default:
return fmt.Errorf(errFormat, v)
case reflect.Ptr:
if t.Elem().Kind() != reflect.Struct {
return fmt.Errorf(errFormat, v)
}
return jsonapi.MarshalOnePayload(w, v)
case reflect.Slice:
s := reflect.ValueOf(v)
if s.Type().Elem().Kind() != reflect.Ptr {
return fmt.Errorf(errFormat, v)
}
if s.Type().Elem().Elem().Kind() != reflect.Struct {
return fmt.Errorf(errFormat, v)

}
return jsonapi.MarshalManyPayload(w, v)
}
}

func Decode(r io.Reader, ptr interface{}) (Offset, error) {
const errFormat = "cannot decode to %T, need pointer to struct or pointer to slice"
if reflect.TypeOf(ptr).Kind() != reflect.Ptr {
return Offset{}, fmt.Errorf(errFormat, ptr)
}
v := reflect.Indirect(reflect.ValueOf(ptr))
switch v.Kind() {
default:
return Offset{}, fmt.Errorf(errFormat, ptr)
case reflect.Struct:
return Offset{}, jsonapi.UnmarshalPayload(r, ptr)
case reflect.Slice:
data, links, err := jsonapi.UnmarshalManyPayloadWithLinks(r, v.Type().Elem())
if err != nil {
return Offset{}, err
}
for _, d := range data {
v.Set(reflect.Append(v, reflect.ValueOf(d)))
}

o := Offset{}
var perr error
if links != nil {
o, perr = parseOffset(*links)
if perr != nil {
return Offset{}, perr
}
}
return o, nil
}
}

func EncodeOne(w io.Writer, v interface{}) error {
return jsonapi.MarshalOnePayload(w, v)
}
Expand Down

0 comments on commit 8b8706c

Please sign in to comment.