Repository: Yoctol/bottender
Pull request #286 (2018-06-15 08:37:28)

pass merged query and body to handler
#285 

- [x] tests on express, koa, micro, restify
- [x] we may want to use `lodash/isEmpty` to make sure req.query is not an empty object.
- [x] refine error message

```js
if (isEmpty(req.query) && !req.body) {
      throw new Error(
        'createMiddleware(): Missing body parser. Use `body-parser` or other similar package before this middleware.'
      );
}
```


Patch:
diff --git a/src/context/ConsoleContext.js b/src/context/ConsoleContext.js
index 789bf4ad..59543cbe 100644
--- a/src/context/ConsoleContext.js
+++ b/src/context/ConsoleContext.js
@@ -38,11 +38,11 @@ export default class ConsoleContext extends Context implements PlatformContext {
       // $FlowExpectedError
       return new Proxy(this, {
         get(target, key) {
-          if (typeof target[key] !== 'undefined') {
+          if (target[key] !== undefined) {
             return target[key];
           }
 
-          if (methodBlackList.includes(key)) return;
+          if (methodBlackList.some(method => method === key)) return;
           return async (...args) => {
             await target._methodMissing(key, args);
           };
diff --git a/src/express/createMiddleware.js b/src/express/createMiddleware.js
index dc7717a8..690f9fc0 100644
--- a/src/express/createMiddleware.js
+++ b/src/express/createMiddleware.js
@@ -1,15 +1,21 @@
+import isEmpty from 'lodash/isEmpty';
+
 function createMiddleware(bot) {
   const requestHandler = bot.createRequestHandler();
 
   const wrapper = fn => (req, res, next) =>
     fn(req, res).catch(err => next(err));
+
   return wrapper(async (req, res) => {
-    if (!req.body) {
+    if (isEmpty(req.query) && !req.body) {
       throw new Error(
-        'createMiddleware(): Missing body parser. Use `body-parser` or other similar package before this middleware.'
+        'createMiddleware(): Missing query and body, you may need a body parser. Use `body-parser` or other similar package before this middleware.'
       );
     }
-    const response = await requestHandler(req.body);
+    const response = await requestHandler({
+      ...req.query,
+      ...req.body,
+    });
     if (response) {
       res.set(response.headers || {});
       res.status(response.status || 200);
diff --git a/src/koa/createMiddleware.js b/src/koa/createMiddleware.js
index e4d0b675..2669750a 100644
--- a/src/koa/createMiddleware.js
+++ b/src/koa/createMiddleware.js
@@ -1,13 +1,18 @@
+import isEmpty from 'lodash/isEmpty';
+
 function createMiddleware(bot) {
   const requestHandler = bot.createRequestHandler();
   return async ({ request, response }) => {
-    if (!request.body) {
+    if (isEmpty(request.query) && !request.body) {
       throw new Error(
-        'createMiddleware(): Missing body parser. Use `koa-bodyparser` or other similar package before this middleware.'
+        'createMiddleware(): Missing query and body, you may need a body parser. Use `koa-bodyparser` or other similar package before this middleware.'
       );
     }
 
-    const res = await requestHandler(request.body);
+    const res = await requestHandler({
+      ...request.query,
+      ...request.body,
+    });
     if (res) {
       response.set(res.headers || {});
       response.status = res.status || 200;
diff --git a/src/micro/createRequestHandler.js b/src/micro/createRequestHandler.js
index c4d26d21..7c0d56eb 100644
--- a/src/micro/createRequestHandler.js
+++ b/src/micro/createRequestHandler.js
@@ -1,3 +1,5 @@
+import url from 'url';
+
 import { json, send } from 'micro';
 import parseUrlencoded from 'urlencoded-body-parser';
 import shortid from 'shortid';
@@ -52,7 +54,12 @@ function createRequestHandler(bot, config = {}) {
             return;
           }
         }
-        const response = await requestHandler(body);
+        const { query } = url.parse(req.url, true);
+
+        const response = await requestHandler({
+          ...query,
+          ...body,
+        });
         if (response) {
           Object.keys(response.headers).forEach(key => {
             res.setHeader(key, response.headers[key]);
diff --git a/src/restify/createMiddleware.js b/src/restify/createMiddleware.js
index c15d6d8d..0c57e409 100644
--- a/src/restify/createMiddleware.js
+++ b/src/restify/createMiddleware.js
@@ -1,13 +1,18 @@
+import isEmpty from 'lodash/isEmpty';
+
 function createMiddleware(bot) {
   const requestHandler = bot.createRequestHandler();
   return async (req, res, next) => {
-    if (!req.body) {
+    if (isEmpty(req.query) && !req.body) {
       throw new Error(
-        'createMiddleware(): Missing body parser. Use `restify.plugins.bodyParser()` before this middleware.'
+        'createMiddleware(): Missing query and body, you may need a body parser. Use `restify.plugins.bodyParser()` before this middleware.'
       );
     }
 
-    const response = await requestHandler(req.body);
+    const response = await requestHandler({
+      ...req.query,
+      ...req.body,
+    });
     if (response) {
       res.send(response.status || 200, response.body || '', response.headers);
     } else {


Test patch:
diff --git a/src/express/__tests__/createMiddleware.spec.js b/src/express/__tests__/createMiddleware.spec.js
index 92117597..86426f01 100644
--- a/src/express/__tests__/createMiddleware.spec.js
+++ b/src/express/__tests__/createMiddleware.spec.js
@@ -31,6 +31,48 @@ it('should response 200 when no error be thrown', async () => {
   expect(next).not.toBeCalled();
 });
 
+it('should merge query and body then pass into requestHandler', async () => {
+  const { bot, requestHandler } = setup();
+  requestHandler.mockResolvedValue();
+
+  const middleware = createMiddleware(bot);
+
+  const req = {
+    query: { a: '1' },
+    body: { b: 2 },
+  };
+  const res = {
+    send: jest.fn(),
+    status: jest.fn(),
+  };
+  const next = jest.fn();
+
+  await middleware(req, res, next);
+
+  expect(requestHandler).toBeCalledWith({ a: '1', b: 2 });
+});
+
+it('should overwrite query value if this key exists in body', async () => {
+  const { bot, requestHandler } = setup();
+  requestHandler.mockResolvedValue();
+
+  const middleware = createMiddleware(bot);
+
+  const req = {
+    query: { a: '1' },
+    body: { a: 2 },
+  };
+  const res = {
+    send: jest.fn(),
+    status: jest.fn(),
+  };
+  const next = jest.fn();
+
+  await middleware(req, res, next);
+
+  expect(requestHandler).toBeCalledWith({ a: 2 });
+});
+
 it('should response 200 if there is response return from requestHandler', async () => {
   const { bot, requestHandler } = setup();
   requestHandler.mockResolvedValue({});
@@ -95,7 +137,7 @@ it('should throw when no body exists', async () => {
   const error = next.mock.calls[0][0];
 
   expect(error).toBeDefined();
-  expect(error.message).toMatch(/Missing body parser/);
+  expect(error.message).toMatch(/Missing query and body/);
 });
 
 it('should catch error when requestHandler error', async () => {
diff --git a/src/koa/__tests__/createMiddleware.spec.js b/src/koa/__tests__/createMiddleware.spec.js
index 684b4b0c..1988af3b 100644
--- a/src/koa/__tests__/createMiddleware.spec.js
+++ b/src/koa/__tests__/createMiddleware.spec.js
@@ -25,6 +25,40 @@ it('should response 200 when no error be thrown', async () => {
   expect(response.status).toBe(200);
 });
 
+it('should merge query and body then pass into requestHandler', async () => {
+  const { bot, requestHandler } = setup();
+  requestHandler.mockResolvedValue();
+
+  const middleware = createMiddleware(bot);
+
+  const request = {
+    query: { a: '1' },
+    body: { b: 2 },
+  };
+  const response = {};
+
+  await middleware({ request, response });
+
+  expect(requestHandler).toBeCalledWith({ a: '1', b: 2 });
+});
+
+it('should overwrite query value if this key exists in body', async () => {
+  const { bot, requestHandler } = setup();
+  requestHandler.mockResolvedValue();
+
+  const middleware = createMiddleware(bot);
+
+  const request = {
+    query: { a: '1' },
+    body: { a: 2 },
+  };
+  const response = {};
+
+  await middleware({ request, response });
+
+  expect(requestHandler).toBeCalledWith({ a: 2 });
+});
+
 it('should response 200 if there is response return from requestHandler', async () => {
   const { bot, requestHandler } = setup();
   requestHandler.mockResolvedValue({});
@@ -92,5 +126,5 @@ it('should throw when no body exists', async () => {
   }
 
   expect(error).toBeDefined();
-  expect(error.message).toMatch(/Missing body parser/);
+  expect(error.message).toMatch(/Missing query and body/);
 });
diff --git a/src/micro/__tests__/createRequestHandler.spec.js b/src/micro/__tests__/createRequestHandler.spec.js
index e8dd5a73..bb8f8dfd 100644
--- a/src/micro/__tests__/createRequestHandler.spec.js
+++ b/src/micro/__tests__/createRequestHandler.spec.js
@@ -23,23 +23,29 @@ function setup({ platform }) {
       verifySignature: jest.fn(),
     },
   };
+  const res = {
+    setHeader: jest.fn(),
+    sendStatus: jest.fn(),
+  };
+
   return {
     bot,
     requestHandler,
+    res,
   };
 }
 
 it('should call verifyMessengerWebhook when GET', async () => {
-  const { bot, requestHandler } = setup({ platform: 'messenger' });
+  const { bot, requestHandler, res } = setup({ platform: 'messenger' });
   const middleware = jest.fn();
   verifyMessengerWebhook.mockReturnValue(middleware);
   requestHandler.mockResolvedValue();
 
   const microRequestHandler = createRequestHandler(bot);
 
-  const req = { method: 'GET' };
-  const res = {
-    sendStatus: jest.fn(),
+  const req = {
+    method: 'GET',
+    url: '',
   };
 
   await microRequestHandler(req, res);
@@ -48,21 +54,18 @@ it('should call verifyMessengerWebhook when GET', async () => {
 });
 
 it('should call verifySlackWebhook when POST and body with type', async () => {
-  const { bot } = setup({ platform: 'slack' });
+  const { bot, res } = setup({ platform: 'slack' });
   const middleware = jest.fn();
   verifySlackWebhook.mockReturnValue(middleware);
 
   const microRequestHandler = createRequestHandler(bot);
-  micro.json.mockImplementationOnce(req => req.body);
+  micro.json.mockImplementationOnce(() => ({ type: 'url_verification' }));
 
   const req = {
     method: 'POST',
-    body: { type: 'url_verification' },
+    url: '',
     headers: { 'content-type': 'application/json' },
   };
-  const res = {
-    sendStatus: jest.fn(),
-  };
 
   await microRequestHandler(req, res);
 
@@ -70,16 +73,16 @@ it('should call verifySlackWebhook when POST and body with type', async () => {
 });
 
 it('should not call verifyMessengerWebhook when GET if platform is not messenger', async () => {
-  const { bot, requestHandler } = setup({ platform: 'line' });
+  const { bot, requestHandler, res } = setup({ platform: 'line' });
   const middleware = jest.fn();
   verifyMessengerWebhook.mockReturnValue(middleware);
   requestHandler.mockResolvedValue();
 
   const microRequestHandler = createRequestHandler(bot);
 
-  const req = { method: 'GET' };
-  const res = {
-    sendStatus: jest.fn(),
+  const req = {
+    method: 'GET',
+    url: '',
   };
 
   await microRequestHandler(req, res);
@@ -89,7 +92,7 @@ it('should not call verifyMessengerWebhook when GET if platform is not messenger
 });
 
 it('should call verifyMessengerSignature if platform is Messenger', async () => {
-  const { bot, requestHandler } = setup({ platform: 'messenger' });
+  const { bot, requestHandler, res } = setup({ platform: 'messenger' });
   const middleware = jest.fn();
   middleware.mockResolvedValue(true);
   verifyMessengerSignature.mockReturnValue(middleware);
@@ -99,9 +102,9 @@ it('should call verifyMessengerSignature if platform is Messenger', async () =>
 
   const req = {
     method: 'POST',
+    url: '',
     headers: { 'content-type': 'application/json' },
   };
-  const res = {};
 
   await microRequestHandler(req, res);
 
@@ -110,7 +113,7 @@ it('should call verifyMessengerSignature if platform is Messenger', async () =>
 });
 
 it('should not send 200 if verifyMessengerSignature fail if platform is Messenger', async () => {
-  const { bot, requestHandler } = setup({ platform: 'messenger' });
+  const { bot, requestHandler, res } = setup({ platform: 'messenger' });
   const middleware = jest.fn();
   middleware.mockResolvedValue(false);
   verifyMessengerSignature.mockReturnValue(middleware);
@@ -120,9 +123,9 @@ it('should not send 200 if verifyMessengerSignature fail if platform is Messenge
 
   const req = {
     method: 'POST',
+    url: '',
     headers: { 'content-type': 'application/json' },
   };
-  const res = {};
 
   await microRequestHandler(req, res);
 
@@ -131,7 +134,7 @@ it('should not send 200 if verifyMessengerSignature fail if platform is Messenge
 });
 
 it('should call verifyLineSignature if platform is Line', async () => {
-  const { bot, requestHandler } = setup({ platform: 'line' });
+  const { bot, requestHandler, res } = setup({ platform: 'line' });
   const middleware = jest.fn();
   middleware.mockResolvedValue(true);
   verifyLineSignature.mockReturnValue(middleware);
@@ -141,9 +144,9 @@ it('should call verifyLineSignature if platform is Line', async () => {
 
   const req = {
     method: 'POST',
+    url: '',
     headers: { 'content-type': 'application/json' },
   };
-  const res = {};
 
   await microRequestHandler(req, res);
 
@@ -152,7 +155,7 @@ it('should call verifyLineSignature if platform is Line', async () => {
 });
 
 it('should not send 200 if verifyLineSignature fail if platform is Line', async () => {
-  const { bot, requestHandler } = setup({ platform: 'line' });
+  const { bot, requestHandler, res } = setup({ platform: 'line' });
   const middleware = jest.fn();
   middleware.mockResolvedValue(false);
   verifyLineSignature.mockReturnValue(middleware);
@@ -162,9 +165,9 @@ it('should not send 200 if verifyLineSignature fail if platform is Line', async
 
   const req = {
     method: 'POST',
+    url: '',
     headers: { 'content-type': 'application/json' },
   };
-  const res = {};
 
   await microRequestHandler(req, res);
 
@@ -173,21 +176,20 @@ it('should not send 200 if verifyLineSignature fail if platform is Line', async
 });
 
 it('should call verifySlackSignature if platform is Slack', async () => {
-  const { bot, requestHandler } = setup({ platform: 'slack' });
+  const { bot, requestHandler, res } = setup({ platform: 'slack' });
   const middleware = jest.fn();
   middleware.mockResolvedValue(true);
   verifySlackSignature.mockReturnValue(middleware);
   requestHandler.mockResolvedValue();
-  micro.json.mockImplementationOnce(req => req.body);
+  micro.json.mockImplementationOnce(() => ({ token: 'mytoken' }));
 
   const microRequestHandler = createRequestHandler(bot);
 
   const req = {
     method: 'POST',
+    url: '',
     headers: { 'content-type': 'application/json' },
-    body: { token: 'mytoken' },
   };
-  const res = {};
 
   await microRequestHandler(req, res);
 
@@ -196,21 +198,20 @@ it('should call verifySlackSignature if platform is Slack', async () => {
 });
 
 it('should not send 200 if verifySlackSignature fail and if platform is Slack', async () => {
-  const { bot, requestHandler } = setup({ platform: 'slack' });
+  const { bot, requestHandler, res } = setup({ platform: 'slack' });
   const middleware = jest.fn();
   middleware.mockResolvedValue(false);
   verifySlackSignature.mockReturnValue(middleware);
   requestHandler.mockResolvedValue();
-  micro.json.mockImplementationOnce(req => req.body);
+  micro.json.mockImplementationOnce(() => ({ token: 'mytoken' }));
 
   const microRequestHandler = createRequestHandler(bot);
 
   const req = {
     method: 'POST',
+    url: '',
     headers: { 'content-type': 'application/json' },
-    body: { token: 'mytoken' },
   };
-  const res = {};
 
   await microRequestHandler(req, res);
 
@@ -219,35 +220,69 @@ it('should not send 200 if verifySlackSignature fail and if platform is Slack',
 });
 
 it('should response 200 when no error be thrown in requestHandler', async () => {
-  const { bot, requestHandler } = setup({ platform: 'other' });
+  const { bot, requestHandler, res } = setup({ platform: 'other' });
   requestHandler.mockResolvedValue();
 
   const microRequestHandler = createRequestHandler(bot);
 
   const req = {
     method: 'POST',
-    body: {},
+    url: '',
     headers: { 'content-type': 'application/json' },
   };
-  const res = {};
 
   await microRequestHandler(req, res);
 
   expect(micro.send).toBeCalledWith(res, 200);
 });
 
+it('should merge query and body then pass into requestHandler', async () => {
+  const { bot, requestHandler, res } = setup({ platform: 'other' });
+  requestHandler.mockResolvedValue();
+  micro.json.mockImplementationOnce(() => ({ b: 2 }));
+
+  const microRequestHandler = createRequestHandler(bot);
+
+  const req = {
+    method: 'POST',
+    url: 'https://example.com/webhook?a=1',
+    headers: { 'content-type': 'application/json' },
+  };
+
+  await microRequestHandler(req, res);
+
+  expect(requestHandler).toBeCalledWith({ a: '1', b: 2 });
+});
+
+it('should overwrite query value if this key exists in body', async () => {
+  const { bot, requestHandler, res } = setup({ platform: 'other' });
+  requestHandler.mockResolvedValue();
+  micro.json.mockImplementationOnce(() => ({ a: 2 }));
+
+  const microRequestHandler = createRequestHandler(bot);
+
+  const req = {
+    method: 'POST',
+    url: 'https://example.com/webhook?a=1',
+    headers: { 'content-type': 'application/json' },
+  };
+
+  await microRequestHandler(req, res);
+
+  expect(requestHandler).toBeCalledWith({ a: 2 });
+});
+
 it('should response 200 if there is response return from requestHandler', async () => {
-  const { bot, requestHandler } = setup({ platform: 'other' });
+  const { bot, requestHandler, res } = setup({ platform: 'other' });
   requestHandler.mockResolvedValue({ headers: {} });
 
   const microRequestHandler = createRequestHandler(bot);
 
   const req = {
     method: 'POST',
-    body: {},
+    url: '',
     headers: { 'content-type': 'application/json' },
   };
-  const res = {};
 
   await microRequestHandler(req, res);
 
@@ -255,7 +290,7 @@ it('should response 200 if there is response return from requestHandler', async
 });
 
 it('should overwrite response when provide', async () => {
-  const { bot, requestHandler } = setup({ platform: 'other' });
+  const { bot, requestHandler, res } = setup({ platform: 'other' });
   requestHandler.mockResolvedValue({
     status: 400,
     headers: {
@@ -270,12 +305,9 @@ it('should overwrite response when provide', async () => {
 
   const req = {
     method: 'POST',
-    body: {},
+    url: '',
     headers: { 'content-type': 'application/json' },
   };
-  const res = {
-    setHeader: jest.fn(),
-  };
 
   await microRequestHandler(req, res);
 
diff --git a/src/restify/__tests__/createMiddleware.spec.js b/src/restify/__tests__/createMiddleware.spec.js
index cbbeb9d4..ea14fb62 100644
--- a/src/restify/__tests__/createMiddleware.spec.js
+++ b/src/restify/__tests__/createMiddleware.spec.js
@@ -29,6 +29,46 @@ it('should response 200 when no error be thrown', async () => {
   expect(next).toBeCalled();
 });
 
+it('should merge query and body then pass into requestHandler', async () => {
+  const { bot, requestHandler } = setup();
+  requestHandler.mockResolvedValue();
+
+  const middleware = createMiddleware(bot);
+
+  const req = {
+    query: { a: '1' },
+    body: { b: 2 },
+  };
+  const res = {
+    send: jest.fn(),
+  };
+  const next = jest.fn();
+
+  await middleware(req, res, next);
+
+  expect(requestHandler).toBeCalledWith({ a: '1', b: 2 });
+});
+
+it('should overwrite query value if this key exists in body', async () => {
+  const { bot, requestHandler } = setup();
+  requestHandler.mockResolvedValue();
+
+  const middleware = createMiddleware(bot);
+
+  const req = {
+    query: { a: '1' },
+    body: { a: 2 },
+  };
+  const res = {
+    send: jest.fn(),
+  };
+  const next = jest.fn();
+
+  await middleware(req, res, next);
+
+  expect(requestHandler).toBeCalledWith({ a: 2 });
+});
+
 it('should response 200 if there is response return from requestHandler', async () => {
   const { bot, requestHandler } = setup();
   requestHandler.mockResolvedValue({ headers: {} });
@@ -90,5 +130,5 @@ it('should throw when no body exists', async () => {
   }
 
   expect(error).toBeDefined();
-  expect(error.message).toMatch(/Missing body parser/);
+  expect(error.message).toMatch(/Missing query and body/);
 });