Skip to content

Commit

Permalink
Updated from code review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
@FrankHassanabad
FrankHassanabad committed Jul 27, 2020
1 parent 0682b66 commit 42dfd27
Show file tree
Hide file tree
Showing 9 changed files with 275 additions and 104 deletions.
152 changes: 89 additions & 63 deletions src/core/server/http/http_server.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -796,7 +796,6 @@ test('exposes route details of incoming request to a route handler', async () =>
method: 'get',
path: '/',
options: {
timeout: { server: false }, // hapi populates the default
authRequired: true,
xsrfRequired: false,
tags: [],
Expand Down Expand Up @@ -907,11 +906,9 @@ test('exposes route details of incoming request to a route handler (POST + paylo
authRequired: true,
xsrfRequired: true,
tags: [],
timeout: { server: false }, // hapi populates the default
body: {
parse: true, // hapi populates the default
maxBytes: 1024, // hapi populates the default
timeout: 10000, // hapi populates the default
accepts: ['application/json'],
output: 'data',
},
Expand Down Expand Up @@ -990,198 +987,228 @@ describe('body options', () => {
await supertest(innerServer.listener).post('/').send({ test: 1 }).expect(200, {
parse: false,
maxBytes: 1024, // hapi populates the default
timeout: 10000, // hapi populates the default
output: 'data',
});
});
});

test('should accept a "timeout" in the body with "timeout" set to "false"', async () => {
describe('timeout options', () => {
test('should accept a socket "timeout" which is 3 minutes in milliseconds, "300000" for a POST', async () => {
const { registerRouter, server: innerServer } = await server.setup(config);

const router = new Router('', logger, enhanceWithContext);
router.post(
{
path: '/',
validate: false,
options: { body: { timeout: false } },
options: { timeout: 300000 },
},
(context, req, res) => {
try {
return res.ok({ body: req.route.options.body });
return res.ok({ body: { timeout: req.route.options.timeout } });
} catch (err) {
return res.internalError({ body: err.message });
}
}
);
registerRouter(router);

await server.start();
await supertest(innerServer.listener).post('/').send({ test: 1 }).expect(200, {
parse: true,
maxBytes: 1024, // hapi populates the default
timeout: false,
output: 'data',
timeout: 300000,
});
});

test('should accept a numeric "timeout" in the body which is 1 minutes in milliseconds, "60000"', async () => {
test('should accept a socket "timeout" which is 3 minutes in milliseconds, "300000" for a GET', async () => {
const { registerRouter, server: innerServer } = await server.setup(config);

const router = new Router('', logger, enhanceWithContext);
router.post(
router.get(
{
path: '/',
validate: false,
options: { body: { timeout: 60000 } },
options: { timeout: 300000 },
},
(context, req, res) => {
try {
return res.ok({ body: req.route.options.body });
return res.ok({ body: { timeout: req.route.options.timeout } });
} catch (err) {
return res.internalError({ body: err.message });
}
}
);
registerRouter(router);
await server.start();
await supertest(innerServer.listener).post('/').send({ test: 1 }).expect(200, {
parse: true,
maxBytes: 1024, // hapi populates the default
timeout: 60000,
output: 'data',
await supertest(innerServer.listener).get('/').expect(200, {
timeout: 300000,
});
});

test('should accept a numeric "timeout" in the body which is 5 minutes in milliseconds, "300000" if the socket timeout is set above 5 minutes', async () => {
test('should accept a socket "timeout" which is 3 minutes in milliseconds, "300000" for a DELETE', async () => {
const { registerRouter, server: innerServer } = await server.setup(config);

const router = new Router('', logger, enhanceWithContext);
router.post(
router.delete(
{
path: '/',
validate: false,
options: { body: { timeout: 300000 }, timeout: { socket: 300001 } },
options: { timeout: 300000 },
},
(context, req, res) => {
try {
return res.ok({ body: req.route.options.body });
return res.ok({ body: { timeout: req.route.options.timeout } });
} catch (err) {
return res.internalError({ body: err.message });
}
}
);
registerRouter(router);
await server.start();
await supertest(innerServer.listener).post('/').send({ test: 1 }).expect(200, {
parse: true,
maxBytes: 1024, // hapi populates the default
await supertest(innerServer.listener).delete('/').expect(200, {
timeout: 300000,
output: 'data',
});
});
});

describe('timeout options', () => {
test('should accept a socket "timeout" in the body which is 3 minutes in milliseconds, "300000"', async () => {
test('should accept a socket "timeout" which is 3 minutes in milliseconds, "300000" for a PUT', async () => {
const { registerRouter, server: innerServer } = await server.setup(config);

const router = new Router('', logger, enhanceWithContext);
router.post(
router.put(
{
path: '/',
validate: false,
options: { timeout: { socket: 300000 } },
options: { timeout: 300000 },
},
(context, req, res) => {
try {
return res.ok({ body: req.route.options.timeout });
return res.ok({ body: { timeout: req.route.options.timeout } });
} catch (err) {
return res.internalError({ body: err.message });
}
}
);
registerRouter(router);
await server.start();
await supertest(innerServer.listener).post('/').send({ test: 1 }).expect(200, {
server: false, // hapi populates the default
socket: 300000,
await supertest(innerServer.listener).put('/').expect(200, {
timeout: 300000,
});
});

test('should accept a socket "timeout" in the body which is "false"', async () => {
test('should accept a socket "timeout" which is 3 minutes in milliseconds, "300000" for a PATCH', async () => {
const { registerRouter, server: innerServer } = await server.setup(config);

const router = new Router('', logger, enhanceWithContext);
router.post(
router.patch(
{
path: '/',
validate: false,
options: { timeout: { socket: false } },
options: { timeout: 300000 },
},
(context, req, res) => {
try {
return res.ok({ body: req.route.options.timeout });
return res.ok({ body: { timeout: req.route.options.timeout } });
} catch (err) {
return res.internalError({ body: err.message });
}
}
);
registerRouter(router);
await server.start();
await supertest(innerServer.listener).post('/').send({ test: 1 }).expect(200, {
server: false, // hapi populates the default
socket: false,
await supertest(innerServer.listener).patch('/').expect(200, {
timeout: 300000,
});
});

test('should accept a server "timeout" in the body which is 1 minutes in milliseconds, "60000"', async () => {
const { registerRouter, server: innerServer } = await server.setup(config);
test('should reject a socket "timeout" of "0" for a POST', async () => {
const { registerRouter } = await server.setup(config);

const router = new Router('', logger, enhanceWithContext);
router.post(
{
path: '/',
validate: false,
options: { timeout: { server: 60000 } },
options: { timeout: 0 },
},
(context, req, res) => {
try {
return res.ok({ body: req.route.options.timeout });
return res.ok({ body: { timeout: req.route.options.timeout } });
} catch (err) {
return res.internalError({ body: err.message });
}
}
);
registerRouter(router);
await server.start();
await supertest(innerServer.listener).post('/').send({ test: 1 }).expect(200, {
server: 60000,
});
await expect(server.start()).rejects.toThrowError('Invalid routeConfig options');
});

test('should accept a server "timeout" in the body which is "false"', async () => {
const { registerRouter, server: innerServer } = await server.setup(config);
test('should reject a socket "timeout" of "0" for a GET', async () => {
const { registerRouter } = await server.setup(config);

const router = new Router('', logger, enhanceWithContext);
router.post(
router.get(
{
path: '/',
validate: false,
options: { timeout: { server: false } },
options: { timeout: 0 },
},
(context, req, res) => {
try {
return res.ok({ body: req.route.options.timeout });
return res.ok({ body: { timeout: req.route.options.timeout } });
} catch (err) {
return res.internalError({ body: err.message });
}
}
);
registerRouter(router);
await server.start();
await supertest(innerServer.listener).post('/').send({ test: 1 }).expect(200, {
server: false,
});
// NOTE: This error message actually indicates a bug from within Hapi Server as you cannot configure the payload timeout when you
// are using a GET or HEAD route. Because of this Hapi server bug you cannot technically set a timeout less than 10 seconds when
// configuring a GET route including 0
await expect(server.start()).rejects.toThrowError(
'Payload timeout must be shorter than socket timeout: get /'
);
});

test('should reject a socket "timeout" of "0" for a PUT', async () => {
const { registerRouter } = await server.setup(config);

const router = new Router('', logger, enhanceWithContext);
router.put(
{
path: '/',
validate: false,
options: { timeout: 0 },
},
(context, req, res) => {
try {
return res.ok({ body: { timeout: req.route.options.timeout } });
} catch (err) {
return res.internalError({ body: err.message });
}
}
);
registerRouter(router);
await expect(server.start()).rejects.toThrowError('Invalid routeConfig options');
});

test('should reject a socket "timeout" of "0" for a PATCH', async () => {
const { registerRouter } = await server.setup(config);

const router = new Router('', logger, enhanceWithContext);
router.patch(
{
path: '/',
validate: false,
options: { timeout: 0 },
},
(context, req, res) => {
try {
return res.ok({ body: { timeout: req.route.options.timeout } });
} catch (err) {
return res.internalError({ body: err.message });
}
}
);
registerRouter(router);
await expect(server.start()).rejects.toThrowError('Invalid routeConfig options');
});
});

Expand Down Expand Up @@ -1210,7 +1237,6 @@ test('should return a stream in the body', async () => {
await supertest(innerServer.listener).put('/').send({ test: 1 }).expect(200, {
parse: true,
maxBytes: 1024, // hapi populates the default
timeout: 10000, // hapi populates the default
output: 'stream',
});
});
Expand Down
24 changes: 17 additions & 7 deletions src/core/server/http/http_server.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -161,9 +161,10 @@ export class HttpServer {
this.log.debug(`registering route handler for [${route.path}]`);
// Hapi does not allow payload validation to be specified for 'head' or 'get' requests
const validate = isSafeMethod(route.method) ? undefined : { payload: true };
const { authRequired, tags, body = {}, timeout = {} } = route.options;
const { accepts: allow, maxBytes, output, parse, timeout: payloadTimeout } = body;
const { server, socket } = timeout;
const { authRequired, tags, body = {}, timeout } = route.options;
const { accepts: allow, maxBytes, output, parse } = body;
// Hapi does not allow timeouts on payloads to be specified for 'head' or 'get' requests
const payloadTimeout = isSafeMethod(route.method) || timeout == null ? undefined : timeout;

const kibanaRouteState: KibanaRouteState = {
xsrfRequired: route.options.xsrfRequired ?? !isSafeMethod(route.method),
Expand All @@ -185,11 +186,20 @@ export class HttpServer {
payload: [allow, maxBytes, output, parse, payloadTimeout].some(
(v) => typeof v !== 'undefined'
)
? { allow, maxBytes, output, parse, timeout: payloadTimeout }
: undefined,
timeout: [server, socket].some((v) => typeof v !== 'undefined')
? { server, socket }
? {
allow,
maxBytes,
output,
parse,
timeout: payloadTimeout,
}
: undefined,
timeout:
timeout != null
? {
socket: timeout + 1, // Hapi server requires the socket to be greater than payload settings so we add 1 millisecond
}
: undefined,
},
});
}
Expand Down
Loading

0 comments on commit 42dfd27

Please sign in to comment.