express-rate-limit / ratelimit-header-parser

Parse RateLimit headers of various forms into a normalized format
MIT License
7 stars 0 forks source link

Todo #1

Open nfriedly opened 1 year ago

nfriedly commented 1 year ago

Moving this out of the readme so that I can publish the module without the todo list being immortalized.

Todo

nfriedly commented 1 year ago

It's published! https://www.npmjs.com/package/ratelimit-header-parser

nfriedly commented 1 year ago

I'm also thinking about renaming the public API:

Additionally, I'm thinking about Policy headers. In general, we could add policy info onto the RateLimit object, but it's possible to have a policy without the associated remaining or reset values, so what then? Maybe omit them from getRatelimit(), but have a setting on getRateLimits() to include policy-only objects in the results?

gamemaker1 commented 1 year ago

getRateLimit: replace current parseRateLimit

Maybe this could be getRateLimitInfo? Similar to the getRateLimitPolicy function I propose below.

getRateLimits: new function that returns an array of all the ratelimits (if any)

Could you please give me an example of when multiple rate limit headers could be returned?

parseCombinedRateLimitHeader: expose the current method. Maybe rename it to parseCombinedRateLimit or something else shorter?

Maybe we can export these functions as parsers? Each type of ratelimit headers gets its own parsers/<name>.ts file, and all these functions are exported from index.ts as a parsers object.


About the policy headers - should we make that a separate method, like getRateLimitPolicy? That way we don't need to worry about whether the policy appears with or without the rest of the headers.

gamemaker1 commented 1 year ago

Also, should we add a test-examples step to the CI, to run all the programs in the examples/ folder against the source code on every commit? It'll work like the external tests we have in express-rate-limit.

nfriedly commented 1 year ago

Multiple rate limits example: User & Client limits: https://apidocs.imgur.com/

In general, I'd like to match the policy to the RateLimit and include all the details in a single object. However, the IETF draft had had a few examples with multiple policies but only one RateLimit header, which is the scenario I'm thinking about how to handle.

As for testing the examples, that sounds like a good idea, at least to ensure they don't throw. Validating the results might be a bit harder without making the examples themselves more tricky

gamemaker1 commented 1 year ago

Multiple rate limits example: User & Client limits: apidocs.imgur.com

In general, I'd like to match the policy to the RateLimit and include all the details in a single object. However, the IETF draft had had a few examples with multiple policies but only one RateLimit header, which is the scenario I'm thinking about how to handle.

Oh I see what you mean. I think we should do the following:

As for testing the examples, that sounds like a good idea, at least to ensure they don't throw. Validating the results might be a bit harder without making the examples themselves more tricky

I don't see the need to validate the results per se, we're already doing that in the tests. Just a no-throw is what I implemented in 9e6a84b, does that seem good?

nfriedly commented 1 year ago

I feel like the normal case is going to be only retrieving a single rate limit, which is why I was thinking of differentiating between the singular and plural functions.

The singular one can also be a bit better optimized, because it can stop as soon as it finds a single match, whereas a plural version would have to do an exhaustive search each time. (Not that it makes a huge difference...)

I don't see the need to validate the results per se, we're already doing that in the tests. Just a no-throw is what I implemented in https://github.com/express-rate-limit/ratelimit-header-parser/commit/9e6a84b045e7b29ef561421911fe2610e1e47b7a, does that seem good?

Yeah, that's awesome!

nfriedly commented 1 year ago

One other thought: It currently can potentially return an object with NaN's and/or Invalid Dates if parsing some of the values fails. That's probably not very useful, but I'm on the fence about what to do in that scenario:

I'm kind of leaning towards throwing an error as the default, but having a config option to do something else.

gamemaker1 commented 1 year ago

The singular one can also be a bit better optimized, because it can stop as soon as it finds a single match, whereas a plural version would have to do an exhaustive search each time. (Not that it makes a huge difference...)

Yea, exactly. I wrote a library to parse the Accept header a while back and faced the same problem. I ended up doing this, where the singular function basically calls the plural one and returns the first result.

It currently can potentially return an object with NaN's and/or Invalid Dates if parsing some of the values fails.

Given that the Retry-After header and reset time are mentioned quite frequently in IETF's draft 7, I too think we should throw an error if we can't parse the date.

We could add a option to the ParserOptions type called strict, which defaults to true. If set to false, we don't throw an error and instead returned undefined in place of a Date.

nfriedly commented 11 months ago

After looking at https://github.com/ietf-wg-httpapi/ratelimit-headers/pull/130 and https://datatracker.ietf.org/doc/rfc8941/ I added this to the list: