Skip to content

Add json parse when the request is 'application/x-www-form-urlencoded'. #100

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

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

Conversation

AMS777
Copy link

@AMS777 AMS777 commented Mar 26, 2018

The current json parse is only valid for requests of type 'application/json', so a valid parse for requests of type 'application/x-www-form-urlencoded' is added.

AMS777 added 2 commits March 26, 2018 18:34
The current json parse is only valid for requests of type 'application/json', so a valid parse for requests of type 'application/x-www-form-urlencoded' is added.
Fix JSHint error:

"modules/ember-cli-fake-server/lib/json-utils.js: line 22, col 11, Misleading line break before '+'; readers may interpret this as an expression boundary."
@bantic
Copy link
Member

bantic commented Mar 27, 2018

@AMS777 Thanks for this PR! I'm somewhat wary of making the assumption that because the initial JSON.parse failed, the request must be x-www-form-urlencoded.

In addition, because the json method is there solely for convenience, I think if someone had this issue I'd suggest that they do their own JSON parsing as needed (essentially, that they avoid calling request.json() and instead write their own utility function similar to what you have in this PR).

I'm not opposed to making the json convenience function more robust, but can you give a little more background on your use-case? Or can you think of a more robust option for doing the content-type detection?

@AMS777
Copy link
Author

AMS777 commented Mar 28, 2018

@bantic By no means pretends this change to be a complete solution to parsing the request data. It just adds one more request data type to be parsed.

The JSON type is still the first case to be tried, and it fails if the request content does not match that type. Instead of loggin an exception message yet another parse try is performed, covering one request content type more. If it fails, the same exception message is logged so the same functionality as before is covered but with one less failing case.

I did this parsing when testing ember-simple-auth which doesn't make JSON requests, but when I needed to put it more than one time and was going to refactor it, I though it could be a good idea to put it on the request.json() method to make it agnostic to the request content type. Thus you have not to stop to think what kind of content type has the request, you just call request.json().

I considered some methods to detect the request content type, like getting the headers, setting them as string, making a case-insensitve search for 'content-type' and checking if it's vaule contains 'json'. I saw it somewhat complicated and decided to follow the existing method: try to parse it no matter what it is and if it fails log an exception message. I think it's the simplest way, so I kept it.

I also considered to make a separate, specific function request.jsonFromXWwwFormUrlEncoded(), but I think having just one agnostic function request.json() was easier to use.

@bantic
Copy link
Member

bantic commented Apr 6, 2018

@AMS777 Thanks for the thoughtful response.
My concern with the multiple try/catch is that it seems odd to assume that if the first JSON.parse threw an exception that we should treat the request body as form-urlencoded instead and try again.

I'd be more interested in something like this:

let json;
if (requestLooksLikeFormEncoded(request)) {
  json = parseRequestAsFormEncoded(request.requestBody);
} else {
  try {
     json = JSON.parse(request.requestBody);
  } catch(e) {
    // ...
  }
}

If you agree and are ok restructuring it like that (and adding some tests), that would be great!

@AMS777
Copy link
Author

AMS777 commented Apr 9, 2018

Ok, @bantic, I'll take a look into it. Your solution is more clear and verbose, no doubt of it.

I proposed the other solution just because it is simpler for the same result.

@AMS777
Copy link
Author

AMS777 commented Apr 9, 2018

I've restructured the request content type identification as you've proposed and have added 2 tests:

test('json() function parses x-www-form-urlencoded request payload')
test('json() function parses x-www-form-urlencoded request payload with special characters')

@bantic
Copy link
Member

bantic commented Apr 9, 2018

@AMS777 thanks for the rev here! It might take me a few days to have a chance to review and respond fully, but I'll do so as soon as I can.

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.

2 participants