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

Added grouping routes for ratelimit #380

Merged
merged 10 commits into from
Sep 26, 2024

Conversation

aniketcodes
Copy link
Contributor

@aniketcodes aniketcodes commented Sep 2, 2024

This PR adds the capability to group various routes and have a common rate limit for them.
#369

Checklist

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina
Copy link
Member

mcollina commented Sep 3, 2024

CI is failing

@aniket-agi
Copy link

Yeah... There is some issue with the coverage...
I will update the test cases by today.

@aniketcodes
Copy link
Contributor Author

Updated the testcases

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

it's still red

@aniketcodes
Copy link
Contributor Author

Screenshot 2024-09-04 at 22 41 06

@aniketcodes
Copy link
Contributor Author

It is fixed. @gurgunday

@aniketcodes
Copy link
Contributor Author

Hi @gurgunday / @mcollina Can you please help me with these? Am I missing something with the test?

@aniketcodes
Copy link
Contributor Author

@gurgunday can you please run the CI again?

@gurgunday
Copy link
Member

I’ll take a look if it’s passing on your end

@aniketcodes
Copy link
Contributor Author

Hi @gurgunday
Can you please run the CI again?

There was some issue with node_modules and c8. I have fixed them.

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

Alright, the implementation LGTM, but I'm not really sure why we need it

Why not just group these routes together and extract them into a plugin? rate-limit supports encapsulation, so why not sth like:

fastify.register(async (fastify) => {
  await fastify.register(import("@fastify/rate-limit"), {
    max: 3,
    timeWindow: "1 minute",
  });

  fastify.get(
    "/otp/send",
    (request, reply) => {
      reply.send({ hello: "from ... grouped rate limit" });
    },
  );

  fastify.get(
    "/otp/resend",
    (request, reply) => {
      reply.send({ hello: "from ... grouped rate limit" });
    },
  );
});

@aniketcodes
Copy link
Contributor Author

Added it so that it is easily configurable across different route files.

@aniketcodes
Copy link
Contributor Author

@gurgunday Will this implementation enhance the feature of grouping the routes without encapsulation?

@gurgunday
Copy link
Member

I'm not sure, I feel like we're duplicating functionality and all these small additions might add up to slow down the rate limiter performance

@climba03003 @Uzlopak what do you think? Keep in mind #380 (review) already exists

@gugu
Copy link

gugu commented Sep 16, 2024

I'm not sure, I feel like we're duplicating functionality and all these small additions might add up to slow down the rate limiter performance

We have a kind-of-common use case for it: we have several similar endpoints. Some of them should be used from frontend, some from backend and so on. If we use per-route rate limit some clever users start to use all similar endpoints to achieve their task and 50 rps becomes 300 rps

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina merged commit b80b05d into fastify:master Sep 26, 2024
10 checks passed
lundibundi added a commit to lundibundi/fastify-rate-limit that referenced this pull request Oct 4, 2024
gurgunday pushed a commit that referenced this pull request Oct 4, 2024
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.

5 participants