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.

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

Add callback to DDPRateLimiter.addRule, run after a rule is executed #8237

Merged
merged 2 commits into from Mar 1, 2017

Conversation

nlhuykhang
Copy link
Contributor

In reference to #5541

Copy link
Contributor

@abernix abernix left a comment

Choose a reason for hiding this comment

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

Thanks very much for submitting this! I think this looks useful. Do you mind adding tests in this file? You can run the tests locally with:

./meteor test-packages ./packages/ddp-rate-limiter

Also, just a nitpick below on variable name...

* @locus Server
*/
DDPRateLimiter.addRule = function (matcher, numRequests, timeInterval) {
return rateLimiter.addRule(matcher, numRequests, timeInterval);
DDPRateLimiter.addRule = function (matcher, numRequests, timeInterval, cb) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Any reason not to just call this callback instead of abbreviating it? Would be nice to match the docs above it and use the same name like the other vars do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is just my personal preference, I will update it to callback

* @return {string} Returns unique rule id
*/
RateLimiter.prototype.addRule = function (rule, numRequestsAllowed,
intervalTime) {
intervalTime, cb) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same nit as above, any reason not to use callback instead of cb?

numInvocationsLeft: Infinity,
userId: input.userId,
clientAddress: input.clientAddress,
connectionId: input.connectionId,
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to not return the entire input? It seems like type (e.g. method or publication) and name (Of the publication or method) are still left out here and they would be very useful.

Perhaps it might be better to just pass the input to _executeCallback separately? (_executeCallback(reply, ruleInput))?

Also, the JSDoc comment above needs to be changed to match whatever we decide.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I exclude type and name because I could not think of any useful use-cases for them. If it is still useful to have them, I will include them.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at #5541 (comment) it seems having access to the name is what @sebakerckhof was looking for. I'm not sure it makes sense to expand all of the properties into the reply. As an alternative, do you have any opinion on my suggestion above (in regards to passing input as a separate argument to the callback OR providing the input object in the reply as reply.input)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Just want to confirm I was indeed interested in getting the name and type. One use case is described in #5541 , namely only banning a user for exceeding certain specific calls, but I can imagine it might be useful for other purposes too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@abernix I already made the modification like you suggested in your earlier comment. The callback now has two arguments: the first is the result of the rule (reply) and other is the input (ruleInput). However I have not pushed the latest code because I am having difficulty with adding test cases for the callback in this file. Callbacks are registered in server so I am uncertain about how to get it result. Could you give me some guidance?

@nlhuykhang
Copy link
Contributor Author

@abernix I updated this PR as our discussion and also add some tests for the new callback. Do you think this PR is good now?

Copy link
Contributor

@abernix abernix left a comment

Choose a reason for hiding this comment

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

Looks almost great! Just one question...

Meteor.setTimeout(expect(), RATE_LIMIT_INTERVAL_TIME_MS);
},
function (test, expect) {
for (var i = 0; i < RATE_LIMIT_NUM_CALLS + 1; i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this very subtle + 1 is here in order to make sure that the rate-limiter is triggered before the next test, yes? If so, maybe add a comment to make this more obvious?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is to make sure the callback is triggered with the reject output (method will not be executed) of the rule.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, that's what I thought. Thanks for adding the comment!

}));
},
function (test, expect) {
Meteor.call('dummyMethod', function() {});
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to call expect at some point in this test function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we don't need to because these calls do not expect any results from server. Are there any possible errors?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's very unlikely in this environment but the next async step/test might (???) be executed before the DDP message was received and cause a failure. I think the async test handler is smart enough to know that there are no actual expectations here but explicitly showing that we expect nothing, it would make the test more clear.

// Call RATE_LIMIT_NUM_CALLS + 1 times to make the rule exceed limit and reject the execution
for (var i = 0; i < RATE_LIMIT_NUM_CALLS + 1; i++) {
Meteor.call('dummyMethod', function() {});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Same question here: need to call expect?

@abernix abernix merged commit 241c92d into meteor:devel Mar 1, 2017
@abernix
Copy link
Contributor

abernix commented Mar 1, 2017

Looks good! Thanks, for making those last requested changes, @nlhuykhang!

@abernix abernix added this to the Release 1.4.3.x milestone Mar 9, 2017
@abernix
Copy link
Contributor

abernix commented Mar 9, 2017

This should be released in Meteor 1.4.3.2. You can try the latest 1.4.3.2 beta and help confirm by running:

meteor update --release 1.4.3.2-beta.0

Please report back if you encounter any problems, and thanks for taking care of this!

@abernix abernix modified the milestones: Release 1.4.3.x, Release 1.4.3.2 Mar 9, 2017
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.

None yet

4 participants