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.

Sign up for GitHub

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

Added custom json serializer support #40

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added custom json serializer support #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 2 additions & 4 deletions decoder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
package render

import (
"encoding/json"
"encoding/xml"
"errors"
"github.com/ajg/form"
"io"
"io/ioutil"
"net/http"

"github.com/ajg/form"
)

// Decode is a package-level variable set to our default Decoder. We do this
Expand Down Expand Up @@ -41,7 +39,7 @@ func DefaultDecoder(r *http.Request, v interface{}) error {
// DecodeJSON decodes a given reader into an interface using the json decoder.
func DecodeJSON(r io.Reader, v interface{}) error {
defer io.Copy(ioutil.Discard, r) //nolint:errcheck
return json.NewDecoder(r).Decode(v)
return jsonMarshaller.Decode(r, v)
}

// DecodeXML decodes a given reader into an interface using the xml decoder.
Expand Down
46 changes: 46 additions & 0 deletions marshaller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package render

import (
"encoding/json"
"io"
)

var jsonMarshaller Marshaller = jsonDefaultMarshaller{}
Copy link

Choose a reason for hiding this comment

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

Maybe we should add a marshaller for XML as well so that this can be configured as well.


type Marshaller interface {
Marshal(v interface{}) ([]byte, error)
Unmarshall(data []byte, v interface{}) error
NewEncoder(w io.Writer) Encoder
Decode(r io.Reader, v interface{}) error
Copy link

Choose a reason for hiding this comment

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

Having NewEncoder but no NewDecoder in the Marshaller interface seems a bit inconsistent. I'm not sure which way to go here. I'd propose to either

  1. add a Decoder interface with only a Decode function and NewDecoder() instead of Decode() or
  2. omit the Encoder interface and rely on the Marshaller implementation taking care of configuring the Encoder the right way (for the default we can encoder.SetEscapeHTML(true) in the Encode function).

I think 2. is a bit nicer since configuring the Encoder always needs to take place in the Encode/NewEncoder function if the user provides an own implementation. But I'd be fine with 1. as well.

}
Comment on lines +10 to +15
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, thanks for your contribution. Few comments:

  1. I feel like this is too big of an interface.
  2. Unmarshall() should be spelled with single l.
  3. I think we'd need to account for this issue: DecodeJSON accepts "{}\n some garbage data" incorrectly #42 and perhaps drop NewEncoder() and NewDecoder() for good.

Copy link

Choose a reason for hiding this comment

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

  1. Depends on 3. if we want to keep the Decoder/Encoder way of handling data we still need at least marshal (if we want to apply the interface for XML as well, which would make sense)
  2. 👍
  3. Not completely sure but as I see it, not using the Decoder/Encoder requires a double-pass over the data (1. collecting the []byte from the io.reader and 2. later parse the []byte to struct). So in general I'm in favor of using Decoder/Encoder. Nevertheless, if this additional data which goes undetected is a security issue we should definitely avoid it. But that's the point I'm not sure about. Doesn't it help explicitly discarding everything which comes after the parsed JSON object?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, can we please confirm that discard solves the "confused deputy" issue?

Copy link

Choose a reason for hiding this comment

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

Let's continue this discussion in the respective issue.

Copy link

Choose a reason for hiding this comment

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

Alright after #42 (comment) I'd personally prefer sticking with Encoder/Decoder checking for EOF after the decoder.Decode() call. If the reader is not at an EOF we just return an error.

What are your opinions on that?

Copy link

Choose a reason for hiding this comment

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

So I'd be at

type Encoder interface {
	Encode(w io.Writer, r *http.Request, ctx context.Context, v interface{}) (error)
}

type Decoder interface {
	Decode(w io.Reader, r *http.Request, ctx context.Context, v interface{}) (error)
}

if really needed one could pass options with the help of the ctx (but I quite dislike working like that if not necessary). I'm still not quite sure if directly adding the request-object is really needed (I personally have no use-case for this, but also I'm not that experienced in this regard) but if you say that using it is a common case, let's directly include it in the signature.

How do you suggest that we proceed in this subject. Ask @josearomeroj to make the modifications (if there is a willingness to do so) or make a new PR (or something different)?

Copy link
Contributor

Choose a reason for hiding this comment

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

We could reuse the r.Context() from *http.Request

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a new PR would make sense if you don't mind

Copy link

Choose a reason for hiding this comment

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

We could reuse the r.Context() from *http.Request

Oh I've missed this one.

I think a new PR would make sense if you don't mind

Alright. I'd wait until tomorrow if @josearomeroj responds. If not I'd be willing to work on this.

Copy link
Author

Choose a reason for hiding this comment

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

If you see it clearly, go ahead! I'm going to be busy this week and I won't be able to work on it.


type Encoder interface {
SetEscapeHTML(on bool)
Encode(v interface{}) error
}

type jsonDefaultMarshaller struct{}

func (j jsonDefaultMarshaller) NewEncoder(w io.Writer) Encoder {
return json.NewEncoder(w)
}

func (j jsonDefaultMarshaller) Decode(r io.Reader, v interface{}) error {
return json.NewDecoder(r).Decode(v)
}

func (j jsonDefaultMarshaller) Marshal(v interface{}) ([]byte, error) {
return json.Marshal(v)
}

func (j jsonDefaultMarshaller) Unmarshall(data []byte, v interface{}) error {
return json.Unmarshal(data, v)
}

func SetJsonMarshaller(m Marshaller) {
Copy link

Choose a reason for hiding this comment

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

Looking at Respond and Decode it would maybe be more consistent to simply expose the jsonMarshaller so that it can be set directly.

if m == nil {
return
}

jsonMarshaller = m
}
5 changes: 2 additions & 3 deletions responder.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ package render
import (
"bytes"
"context"
"encoding/json"
"encoding/xml"
"fmt"
"net/http"
Expand Down Expand Up @@ -92,7 +91,7 @@ func HTML(w http.ResponseWriter, r *http.Request, v string) {
// Content-Type as application/json.
func JSON(w http.ResponseWriter, r *http.Request, v interface{}) {
buf := &bytes.Buffer{}
enc := json.NewEncoder(buf)
enc := jsonMarshaller.NewEncoder(buf)
enc.SetEscapeHTML(true)
if err := enc.Encode(v); err != nil {
http.Error(w, err.Error(), http.StatusInternalServerError)
Expand Down Expand Up @@ -182,7 +181,7 @@ func channelEventStream(w http.ResponseWriter, r *http.Request, v interface{}) {
}
}

bytes, err := json.Marshal(v)
bytes, err := jsonMarshaller.Marshal(v)
if err != nil {
w.Write([]byte(fmt.Sprintf("event: error\ndata: {\"error\":\"%v\"}\n\n", err))) //nolint:errcheck
if f, ok := w.(http.Flusher); ok {
Expand Down