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.

Sign up for GitHub

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

Jump to bottom

Add response handler #37

Open
wants to merge 2 commits into
base: v2.x.x
Choose a base branch
from
Open

Add response handler #37

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
40dff0a
Add response handler
dafortune Sep 6, 2018
2309ea0
Update patova tests
dafortune Sep 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ The object has the following schema (validated [here](./lib/index.js) using [Joi
* `event: String` - The name of the extension point in the request lifecycle when the bucket check must be performed. Options are `"onRequest"`, `"onPreAuth"`, `"onPostAuth"`,`"onPreHandler"` (anything before the request).
* `type: String|(request, callback) => ()` - Either the bucket type as a string or a function. If you use a function, it will be called for every request, this function must invoke the callback function when it is finished.
* `limitd`: an instance of limitd client
* `extractKey: (request, done) => ()` - A function that receives the `request` and a callback `done`.
* `extractKey: (request, reply, done) => ()` - A function that receives the `request` and a callback `done`.
* `request: Request` - The hapi.js [request object](http://hapijs.com/api#request-object).
* `reply: Reply` - The hapi.js [reply interface](http://hapijs.com/api#reply-interface). Useful if you want to skip the check.
* `done: (err: Error, key: String)` - A function that takes an error as the first parameter and the bucket key as the second parameter.
Expand All @@ -48,6 +48,15 @@ The object has the following schema (validated [here](./lib/index.js) using [Joi
* `error: Error` - The error that occurred.
* `reply: Reply` - The hapi.js [reply interface](http://hapijs.com/api#reply-interface).
> If an error occurs and no function is provided, the request lifecycle continues normally as if there was no token bucket restriction. This is a useful default behavior in case the limitd server goes down.
* `limitResponseHandler: (limitResult, req, reply) => ()` A function that receives the result of limit checking against limitd, the `request` and the reply interface and is responsible for sending the handling
Copy link

Choose a reason for hiding this comment

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

I would rename this option to takeResultHandler.
Wouldn't it be more generic to handle conformant and non-conformant results? The same way we could reply.continue() on non-conformant, it might be useful to reject even for conformant?

If we stay with only non-conformant, then I would rename the option to nonConformantResultHandler instead.

I would rename limitResult to takeResult since it's the result object returned from limitd's take() method.

Maybe we should describe succinctly the default behavior when the option is omitted.

and is responsible for sending the handling how to respond to the customer

This sentence needs better wording. Maybe:

and is responsible for handling a non-conformant request.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

takeResultHandler seems really tied to limitd and in theory there might be N takes and we will only call the handler one time.
Regarding renaming to take result my main concern is that as final user you should not have to know how limitd works to use patova, so you should not care if limitd has a method called take IMO.

I will make it more generic more generic to handle the non-limit case too, although in general I resist to make changes I don't feel have a clear use case and fail to see a clear one for the "conformant" case. Maybe not including the headers, or changing some of them I guess? In any case I see some side use cases as the one I pointed out previously, so 👍

how to respond to the customer. This function will be called only when non-conformant limits are detected.
* `limitResult: The result object from limit verification`:
* conformant: whether the response was conformant (is always `false` for this handler, but is kept for consistency)
* limit: the limit of tokens that the bucket accepts
* remaining: number of remaining tokens (is always 0 for this handler, but is kept for consistency)
* reset: next reset timestamp
* `request: Request` - The hapi.js [request object](http://hapijs.com/api#request-object).
* `reply: Reply` - The hapi.js [reply interface](http://hapijs.com/api#reply-interface). Useful if you want to skip the check.

## Contributing
Feel free to open issues with questions/bugs/features. PRs are also welcome.
Expand Down
34 changes: 25 additions & 9 deletions lib/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,8 @@ const schema = Joi.object().keys({
type: [Joi.string(), Joi.func()],
limitd: Joi.object(),
onError: Joi.func(),
extractKey: Joi.func()
extractKey: Joi.func(),
limitResponseHandler: Joi.func()
}).requiredKeys('type', 'event', 'limitd', 'extractKey');

function setResponseHeader(request, header, value) {
Expand Down Expand Up @@ -43,6 +44,17 @@ function getMinimumLimit(limit1, limit2) {
if (!limit1) { return limit2; }
if (!limit2) { return limit1; }

// Even if we have decided not to answer with an error immediately,
// if some limit is non conformat then we will consider it the
// minimum applicable
if (!limit1.conformant && limit2.conformant) {
return limit1;
}

if (limit1.conformant && !limit2.conformant) {
return limit2;
}

if (limit1 && limit2.remaining > limit1.remaining) {
return limit1;
}
Expand All @@ -55,6 +67,16 @@ function setupRateLimitEventExt(server, options) {
const extractKey = options.extractKey;
const onError = options.onError;

const limitResponseHandler = options.limitResponseHandler || ((result, req, reply) => {
Copy link

Choose a reason for hiding this comment

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

Minor:
We could move the default handler at the root of the file, out of the setupRateLimitEventExt function (it is not closing on any other variable).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

in this case it is mostly the same because this is a builder not used at high rate, I'd prefer to keep the function as closed as possible to their usage unless there are perf reasons not to do it.

const error = Boom.tooManyRequests();
error.output.headers = new RateLimitHeaders(
result.limit,
result.remaining,
result.reset);

reply(error);
});

const extractKeyAndTakeToken = function(limitd, request, reply, type) {
extractKey(request, reply, (err, key) =>{
if (err) { return reply(err); }
Expand All @@ -77,18 +99,12 @@ function setupRateLimitEventExt(server, options) {
request.plugins.patova = request.plugins.patova || {};
request.plugins.patova.limit = newMinimumLimitResponse;

if (newMinimumLimitResponse.conformant) {
if (currentLimitResponse.conformant) {
// We continue only if the request is conformat so far
return reply.continue();
}

const error = Boom.tooManyRequests();
error.output.headers = new RateLimitHeaders(
newMinimumLimitResponse.limit,
newMinimumLimitResponse.remaining,
newMinimumLimitResponse.reset);

reply(error);
limitResponseHandler(currentLimitResponse, request, reply);
});
});
};
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "patova",
"version": "2.2.1",
"version": "2.3.0",
"description": "A limitd plugin for hapi.js",
"main": "index.js",
"scripts": {
Expand Down
167 changes: 134 additions & 33 deletions test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,20 @@ describe('options validation', () => {
});
});

it ('should fail if limitResponseHandler is not a function', () => {
plugin.register(null, {
type: 'user',
event: 'onRequest',
limitd: getLimitdClient(),
limitResponseHandler: 'string',
extractKey: EXTRACT_KEY_NOOP
}, err => {
expect(err.details).to.have.length(1);

const firstError = err.details[0];
expect(firstError.message).to.equal('"limitResponseHandler" must be a Function');
});
});
});

describe('with server', () => {
Expand Down Expand Up @@ -502,45 +516,132 @@ function itBehavesLikeWhenLimitdIsRunning(options) {
});

describe('when limitd responds not conformant', () => {
before((done) => {
server.start({ replyError: false }, [
{
type: options.bucket3type,
limitd: getLimitdClient(address),
extractKey: (request, reply, done) => { done(null, 'key'); },
event: 'onRequest'
},
{
type: options.emptyType,
limitd: getLimitdClient(address),
extractKey: (request, reply, done) => { done(null, 'key'); },
event: 'onRequest'
},
{
type: options.bucket3type,
limitd: getLimitdClient(address),
extractKey: (request, reply, done) => { done(null, 'key'); },
event: 'onRequest'
}
], done);
describe('and when limitResponseHandler is not provided', () => {
before((done) => {
server.start({ replyError: false }, [
{
type: options.bucket3type,
limitd: getLimitdClient(address),
extractKey: (request, reply, done) => { done(null, 'key'); },
event: 'onRequest'
},
{
type: options.emptyType,
limitd: getLimitdClient(address),
extractKey: (request, reply, done) => { done(null, 'key'); },
event: 'onRequest'
},
{
type: options.bucket3type,
limitd: getLimitdClient(address),
extractKey: (request, reply, done) => { done(null, 'key'); },
event: 'onRequest'
}
], done);
});

after(server.stop);

it('should send response with 429 if limit has passed for some plugin configuration and set limit header', function(done){
const request = { method: 'POST', url: '/users', payload: { } };
server.inject(request, res => {
const body = JSON.parse(res.payload);
const headers = res.headers;

expect(res.statusCode).to.equal(429);
expect(body.error).to.equal('Too Many Requests');

expect(headers['x-ratelimit-limit']).to.equal(0);
expect(headers['x-ratelimit-remaining']).to.equal(0);
expect(headers['x-ratelimit-reset']).to.equal(0);

done();
});
});
});

after(server.stop);
describe('and when limitResponseHandler is provided', () => {
let callParams = [];

before((done) => {
server.start({ replyError: false }, [
{
type: options.bucket3type,
limitd: getLimitdClient(address),
extractKey: (request, reply, done) => { done(null, 'key'); },
event: 'onRequest',
limitResponseHandler: (result, request, reply) => {
callParams.push(result);

reply.continue()
}
},
{
type: options.emptyType,
limitd: getLimitdClient(address),
extractKey: (request, reply, done) => { done(null, 'key'); },
event: 'onRequest',
limitResponseHandler: (result, request, reply) => {
callParams.push(result);

reply.continue();
}
},
{
type: options.bucket3type,
limitd: getLimitdClient(address),
extractKey: (request, reply, done) => { done(null, 'key'); },
event: 'onRequest',
limitResponseHandler: (result, request, reply) => {
callParams.push(result);

reply.continue()
}
}
], done);
});

it('should send response with 429 if limit has passed for some plugin configuration and set limit header', function(done){
const request = { method: 'POST', url: '/users', payload: { } };
server.inject(request, res => {
const body = JSON.parse(res.payload);
const headers = res.headers;
after(server.stop);

expect(res.statusCode).to.equal(429);
expect(body.error).to.equal('Too Many Requests');
it('should call each handler a single time', (done) => {
const request = { method: 'POST', url: '/users', payload: { } };
server.inject(request, res => {
expect(callParams.length).to.equal(3);

expect(headers['x-ratelimit-limit']).to.equal(0);
expect(headers['x-ratelimit-remaining']).to.equal(0);
expect(headers['x-ratelimit-reset']).to.equal(0);
done();
});
});

done();
it('should call each handler with their own result', (done) => {
const request = { method: 'POST', url: '/users', payload: { } };
server.inject(request, res => {
expect(callParams[0].limit).to.equal(3);
expect(callParams[1].limit).to.equal(0);
expect(callParams[2].limit).to.equal(3);

done();
});
});

it('should call limitResponseHandler to handle the answer', function(done){
const request = { method: 'POST', url: '/users', payload: { } };
server.inject(request, res => {
expect(res.statusCode).to.equal(200);
Copy link

Choose a reason for hiding this comment

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

Would it make sense to spy on limitResponseHandler and check that it was actually invoked, to eliminate the possibility that the take returned a conformant response? Otherwise, I think that configuring only the options.emptyType bucket would make it more obvious that take returns a non-conformant result.

I'd also add a test with a custom handler that returns non-429 result (to witness the handler overriding the default error code). That would also self-document one use-case showing how to use the new handler.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think that the kind of test patova has let us spy in an easy way and non-hacky way. Also we have other test above for the same condition that check that there is a 429 response in this case.

I could maybe check the response handler for the result if we want to be 100% sure in this particular test case

expect(res.payload).to.eql('created');

done();
});
});

it('should return non-conformant values as the minimum when they apply', function(done){
const request = { method: 'POST', url: '/users', payload: { } };
server.inject(request, res => {
expect(res.headers['x-ratelimit-limit']).to.equal(0);
expect(res.headers['x-ratelimit-remaining']).to.equal(0);
expect(res.headers['x-ratelimit-reset']).to.equal(0);

done();
});
});
});
});
Expand Down