Support content-type with parameters #40

mactkg · 2019-06-06T14:34:22Z

A current code can't process HTTP request included content-type with parameters such as application/json;charset=utf-8.
This PR resolve it, also, define default behaviour when content-type isn't set.
According to RFC2616, we should treat unknown content-type data as "application/octet-stream".

ref: https://www.w3.org/Protocols/rfc2616/rfc2616-sec7.html

A previous code can't process HTTP request included content-type with parameters such as "application/json;charset=utf-8". This commit resolve it, also, define default behaviour when content-type isn't set. According to RFC2616, we should treat unknown content-type data as "application/octet-stream".

mactkg · 2019-06-06T14:36:10Z

mod.ts

@@ -114,6 +119,9 @@ export class App {
          }
          Object.assign(params, obj);
          break;
+        case 'application/octet-stream':
+          params['data'] = decodedBody


I can't come up with the best way to treat application/octet-stream data...

Thank you for catching this!

I think the data of 'application/octet-stream' is not params, it's request body and it should implement Deno.Reader.

In my opinion, req.body should be passed to the handler as body prop in ctx.
The line of decoder.decode(await readAll(req.body)); is just a workaround and I must fix this.

I'm sorry that I can't accept data param as a request body.

I opened issue for this.
#41

In my opinion, req.body should be passed to the handler as body prop in ctx.

I think so too. For now, can I remove a case for processing application/octet-stream data?
The code will be modified like this:

switch(type) { // bla bla... Object..assign(params, obj); break; case 'application/octet-stream': // FIXME: we skip here for now, it should be implemented when Issue #41 resolved. break; }

Looks good! Thanks!

syumai

Thank you for giving me a PR! ;)
Please take a look at my comments.

syumai · 2019-06-06T16:23:24Z

mod.ts

@@ -114,6 +119,9 @@ export class App {
          }
          Object.assign(params, obj);
          break;
+        case 'application/octet-stream':
+          params['data'] = decodedBody


Thank you for catching this!

I think the data of 'application/octet-stream' is not params, it's request body and it should implement Deno.Reader.

In my opinion, req.body should be passed to the handler as body prop in ctx.
The line of decoder.decode(await readAll(req.body)); is just a workaround and I must fix this.

I'm sorry that I can't accept data param as a request body.

I opened issue for this.
#41

syumai · 2019-06-06T16:36:26Z

serve_test.ts

@@ -48,6 +49,23 @@ const testCases: Array<testCase> = [
    method: Method.GET,
    expected: 'john',
  },
+  {
+    name: 'valid post',


Thanks! 🎉

mod.ts

syumai · 2019-06-06T17:03:17Z

mod.ts

-      const contentType = req.headers.get('content-type');
+      const rawContentType = req.headers.get('content-type') || "application/octet-stream";
+      const [contentType, ...typeParamsArray] = rawContentType.split(';')
+      const typeParams = Object.assign({}, ...typeParamsArray.map(params => params.split('=')))


This code looks not correct, how about using reduce?

Suggested change

const typeParams = Object.assign({}, ...typeParamsArray.map(params => params.split('=')))

const typeParams = typeParamsArray.reduce((params, curr) => {

const [key, value] = curr.split('=');

params[key] = value;

return params;

}, {});

Please see this: https://jsbin.com/verabuh/edit?js,console

Indeed...! Tests I wrote are passed but the default value of params['charset'] just worked... 😓

syumai · 2019-06-06T17:07:06Z

mod.ts

+      const [contentType, ...typeParamsArray] = rawContentType.split(';')
+      const typeParams = Object.assign({}, ...typeParamsArray.map(params => params.split('=')))
+
+      const decoder = new TextDecoder(typeParams['charset'] || "utf-8")


To get charset in this form, keys must be transformed using function like lowerCase in lodash.
I just want a TODO comment for this ;)

Suggested change

const decoder = new TextDecoder(typeParams['charset'] || "utf-8")

const decoder = new TextDecoder(typeParams['charset'] || "utf-8"); // TODO: downcase `charset` key

syumai · 2019-06-06T17:10:05Z

serve_test.ts

+    registered: post('/params', ({ params }) => [200, params.name]),
+    path: 'params',
+    params: JSON.stringify({ name: 'tom' }),
+    headers: {['content-type']: 'application/json; charset=utf-8'},


No brackets are needed here.

Suggested change

headers: {['content-type']: 'application/json; charset=utf-8'},

headers: { 'content-type': 'application/json; charset=utf-8' },

syumai · 2019-06-06T17:12:12Z

serve_test.ts

@@ -79,7 +97,7 @@ for (const tc of testCases) {
      const reqInit: RequestInit = { method: tc.method };
      if (typeof tc.params === 'string') {
        reqInit.body = tc.params;
-        reqInit.headers = { 'content-type': 'application/json' };
+        reqInit.headers = Object.assign({ 'content-type': 'application/json' }, tc.headers);


In this case, I prefer this form.

Suggested change

reqInit.headers = Object.assign({ 'content-type': 'application/json' }, tc.headers);

reqInit.headers = tc.headers || { 'content-type': 'application/json' };

Co-Authored-By: syumai <[email protected]>

ref: syumai#41

mactkg · 2019-06-07T01:12:44Z

@syumai Thanks for your reviewing. Please review again 🐈

syumai

LGTM, Thanks! 🎉

mactkg commented Jun 6, 2019

View reviewed changes

syumai requested changes Jun 6, 2019

View reviewed changes

mactkg and others added 4 commits June 7, 2019 08:34

Trim white spaces of params

30a8f33

Co-Authored-By: syumai <[email protected]>

Fix some code style, and add comment

290d0e1

Co-Authored-By: syumai <[email protected]>

Fix parsing params

93cb8c2

Co-Authored-By: syumai <[email protected]>

Remove processing for data of 'application/octet-stream'

8f01424

ref: syumai#41

syumai approved these changes Jun 7, 2019

View reviewed changes

syumai merged commit 530c0a3 into syumai:master Jun 7, 2019

mactkg deleted the feature/support-mime-with-params branch June 7, 2019 02:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support content-type with parameters #40

Support content-type with parameters #40

mactkg commented Jun 6, 2019

mactkg Jun 6, 2019

syumai Jun 6, 2019 •

edited

Loading

mactkg Jun 7, 2019

syumai Jun 7, 2019

syumai left a comment •

edited

Loading

syumai Jun 6, 2019 •

edited

Loading

syumai Jun 6, 2019

syumai Jun 6, 2019

mactkg Jun 6, 2019

syumai Jun 6, 2019 •

edited

Loading

syumai Jun 6, 2019

syumai Jun 6, 2019

mactkg commented Jun 7, 2019

syumai left a comment

-      const typeParams = Object.assign({}, ...typeParamsArray.map(params => params.split('=')))
+      const typeParams = typeParamsArray.reduce((params, curr) => {
+        const [key, value] = curr.split('=');
+        params[key] = value;
+        return params;
+      }, {});

	const decoder = new TextDecoder(typeParams['charset'] \|\| "utf-8")
	const decoder = new TextDecoder(typeParams['charset'] \|\| "utf-8"); // TODO: downcase `charset` key

	headers: {['content-type']: 'application/json; charset=utf-8'},
	headers: { 'content-type': 'application/json; charset=utf-8' },

	reqInit.headers = Object.assign({ 'content-type': 'application/json' }, tc.headers);
	reqInit.headers = tc.headers \|\| { 'content-type': 'application/json' };

Support content-type with parameters #40

Support content-type with parameters #40

Conversation

mactkg commented Jun 6, 2019

Choose a reason for hiding this comment

syumai Jun 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syumai left a comment • edited Loading

Choose a reason for hiding this comment

syumai Jun 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

syumai Jun 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mactkg commented Jun 7, 2019

syumai left a comment

Choose a reason for hiding this comment

syumai Jun 6, 2019 •

edited

Loading

syumai left a comment •

edited

Loading

syumai Jun 6, 2019 •

edited

Loading

syumai Jun 6, 2019 •

edited

Loading