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

Fixed Issue x-www-form-urlencoded #51

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Fixed Issue x-www-form-urlencoded #51

wants to merge 2 commits into from

Conversation

antsankov
Copy link

This fixes a bug with the restful service attempting to unmarshal a x-www-form-urlencoded request like it is JSON in POST/PUT requests. This fix simply prevents umarshal from tampering with the request body and returning an error so it can be encoded further down the line using something like this.

I was considering doing the decoding using gorilla/schema to the payload in this PR like I'm doing in my program, but I don't know what the policy on external libraries is.

if err != nil {
// Payload decoding failed.
ctx = ctx.setError(BadRequest(err.Error()))
if r.Header["Content-Type"][0] == "application/x-www-form-urlencoded" {
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be:

if r.Header.Get("Content-Type") == "application/x-www-form-urlencoded" {

@tylertreat-wf
Copy link
Contributor

I would really prefer to make payload decoding pluggable. That seems like the appropriate long-term solution. There would be a PayloadDecoder interface:

type PayloadDecoder interface {
        Decode(*http.Request) Payload
}

Then API could have a RegisterPayloadDecoder method exposed which registers a PayloadDecoder for a given Content-Type:

RegisterPayloadDecoder(contentType string, decoder PayloadDecoder)

Then when a create/update request is received, we simply lookup the PayloadDecoder for the Content-Type and pass the request to it to get the payload. If there isn't one registered, default to JSON.

@tylertreat-wf
Copy link
Contributor

@alexandercampbell-wf @aaronkavlie-wf @stevenosborne-wf

@aaronkavlie-wf
Copy link
Contributor

Build is failing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants