feat: validation check for double-counting hits for a single request #364
Conversation
This impliments the idea discussed in #356 (comment) to check for cases of double-counting a single request. There's probably some situations that this doesn't account for, but I tried to cover the main ones. We'll want to update the PreciseMemoryStore and set it to `localKeys = true` after merging this. (Although, I'm not sure that `localKeys` is the name I want to settle on.)
|
I just added https://github.com/express-rate-limit/express-rate-limit/wiki/Error-Codes#err_erl_double_count to describe this. |
| Validations.singleCountKeys.set(request, stores) | ||
| } | ||
|
|
||
| const storeKey = store.localKeys ? store : store.constructor.name |
There was a problem hiding this comment.
I think this will be "PromisifiedStore" for any legacy store, which I suppose means there's a small chance of a false positive if two different legacy stores were used... but that seems very unlikely in practice.
It could also end up being "Object" or "Function" in some circumstances, but again, there would have to be two different stores that evaluated to the same name, both in use on a single request, to hit a false positive. So, also very unlikely.
I was trying to think about ways that store.constructor.name could throw, but I don't think it could happen. I don't think we can get this far is store is undefined or null, and I believe that any other value is guaranteed to have a .constructor.name.
There was a problem hiding this comment.
I thought the same, maybe we should change the PromisifiedStore to make it set the constructor.name property to Promisified${originalStoreName}?
There was a problem hiding this comment.
This might be a case where constructor.name fails. We should follow the fix mentioned in the first comment below the answer, and advise other stores to do so as well.
There was a problem hiding this comment.
I was going to say that I think setting constructor.name on one instance would affect all other instances, but it looks like it's readonly:
> class Foo {}
undefined
> a = new Foo
Foo {}
> b = new Foo
Foo {}
> a.constructor.name
'Foo'
> b.constructor.name = 'bar'
'bar'
> a.constructor == b.constructor
true
> a.constructor.name == b.constructor.name
true
> b.constructor.name
'Foo'
As for the minification case, I think it would still work for us. They'd have to be minifiying server-side JS, and using two different stores, and the minifier would have to choose the same name for both stores, and the request would have to hit both stores for it to report a false positive.
I think I'm just going to update the docs to explain that it could be a false-positive in rare circumstances, and call this check good enough.
|
Also, I'm thinking about switching from npm to pnpm for the project - it should help in faster installs in the external tests. It takes 20 minutes to run the tests right now, I'm hoping pnpm will reduce it to at least half? |
I've never used pnpn, so I don't really have an opinion on it. I'd say try it out and see if it's faster. One thing that crossed my mind when creating this PR is that even I don't run all of the integration tests locally, because I don't have databases set up for all of the stores. I think that's OK - CI will test that - but perhaps we should make it more clear in https://github.com/express-rate-limit/express-rate-limit/blob/main/.github/pull_request_template.md? E.g. instead of saying "All tests ( Then if we make that distinction, maybe we only use pnpm for the external tests so that contributors don't have to figure it out. |
|
Even I have stopped running the external tests, mostly because of the time it takes, and because, as you said, CI does take care of it anyways.
Sounds great, I will do this now. |
gamemaker1
changed the title
Done with the documentation part, on the main branch. I'll work on the pnpm part in a separate pr. This pr looks good to go! |
|
Awesome, I tweaked https://github.com/express-rate-limit/express-rate-limit/wiki/Error-Codes#err_erl_double_count so I'm going to go ahead and merge this. I might try and do #356 (comment) and #356 (comment) before we publish the next release since they're all somewhat related. |
This implements the idea discussed in #356 (comment) to check for cases of double-counting a single request.
There's probably some situations that this doesn't account for, but I tried to cover the main ones.
We'll want to update the PreciseMemoryStore and set it to
localKeys = trueafter merging this. (Although, I'm not sure thatlocalKeysis the name I want to settle on.)Related Issues
What Does This PR Do?
Added
Caveats/Problems/Issues
🤷♂️
Checklist
npm test) pass.methods/classes/constants/types have been annotated with TSDoc comments.
added for the same.