Added custom json serializer support #40
Conversation
| Marshal(v interface{}) ([]byte, error) | ||
| Unmarshall(data []byte, v interface{}) error | ||
| NewEncoder(w io.Writer) Encoder | ||
| Decode(r io.Reader, v interface{}) error |
There was a problem hiding this comment.
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
- add a
Decoderinterface with only aDecodefunction andNewDecoder()instead ofDecode()or - omit the
Encoderinterface and rely on theMarshallerimplementation taking care of configuring theEncoderthe right way (for the default we canencoder.SetEscapeHTML(true)in theEncodefunction).
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.
| return json.Unmarshal(data, v) | ||
| } | ||
|
|
||
| func SetJsonMarshaller(m Marshaller) { |
| "io" | ||
| ) | ||
|
|
||
| var jsonMarshaller Marshaller = jsonDefaultMarshaller{} |
There was a problem hiding this comment.
Maybe we should add a marshaller for XML as well so that this can be configured as well.
|
[at]Maintainers would be nice if you could comment if this PR has a chance of being merged. My use-case is just to configure the json-decoder (just setting Oh and @josearomeroj would you still work on this PR if the Maintainers want any changes? (otherwise I'd be willing to adopt this PR by cloning and create a new PR) |
|
Hi @atticus-sullivan, thank you for your feedback. |
|
Oh nice, that's already the first step. Wasn't sure since almost a whole year passed since you opened this issue. Especially in this case not having this feature is a bit sad as looking at other PRs (#34 #33 #39 , I think even #11 would be possible to configure) this feature really helps a lot. |
| 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 | ||
| } |
There was a problem hiding this comment.
Hi, thanks for your contribution. Few comments:
- I feel like this is too big of an interface.
- Unmarshall() should be spelled with single l.
- 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.
There was a problem hiding this comment.
- 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) - 👍
- Not completely sure but as I see it, not using the Decoder/Encoder requires a double-pass over the data (1. collecting the
[]bytefrom theio.readerand 2. later parse the[]byteto 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 explicitlydiscarding everything which comes after the parsed JSON object?
There was a problem hiding this comment.
Ok, can we please confirm that discard solves the "confused deputy" issue?
There was a problem hiding this comment.
Let's continue this discussion in the respective issue.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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)?
There was a problem hiding this comment.
We could reuse the r.Context() from *http.Request
There was a problem hiding this comment.
I think a new PR would make sense if you don't mind
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Implemented custom serializer for json: #36