feat: rate limiter + unit tests + readme

[fforbeck]

This PR add tests for the Rate Limiter middleware, the main changes include:

• Updated README with steps and configs to run it locally
  • Project Setup
  • Running Tests
• Added Mocha, Chai and Sinon packages to run Unit Tests
• Rate Limiter
  • Feature Flag (env variable)
  • Fixes
    • Rate Limiter middleware needs to be placed after the IPFS Parsed URL middleware, so we can access the CID
    • Refactored the `import {version} from './package.json' to be set via task and imported as ENV variable, so we can easily tweak that in the tests and don't run into import issues
  • Unit Tests
    • 1. Calls next handler if no auth token is provided and rate limit is not exceeded
    • 2. Throws an error if no auth token is provided and rate limit is exceeded
    • 3. Calls next handler if auth token is present but no token metadata is found and rate limit is not exceeded
    • 4. Throws an error if auth token is present but no token metadata is found and rate limit is exceeded
    • 5. Calls next handler if auth token is present and token metadata is invalid but rate limit is not exceeded
    • 6. Throws an error if auth token is present and token metadata is invalid and rate limit is exceeded
    • 7. Calls next handler if auth token is present and token metadata is valid
  • Update Github Action to execute Unit Tests

[travis]

Could you say more about why Jest is useful here? We've typically used node:test for this project, and Mocha (https://mochajs.org/) or AVA (https://github.com/avajs/ava) on other projects - I'm not entirely sure why we've shied away from Jest, but I am a bit hesitant to add yet another testing framework to the mix for our team. Could definitely be convinced - I've used Jest in the past and it's generally pretty good - but need a bit more convincing ;-p

[fforbeck]

Could you say more about why Jest is useful here? We've typically used node:test for this project, and Mocha (https://mochajs.org/) or AVA (https://github.com/avajs/ava) on other projects - I'm not entirely sure why we've shied away from Jest, but I am a bit hesitant to add yet another testing framework to the mix for our team. Could definitely be convinced - I've used Jest in the past and it's generally pretty good - but need a bit more convincing ;-p

sure thing @travis

I don’t have super strong opinions on this either, but Jest does give you some nice built-in features like mocking, spies, and coverage reports right out of the box. With Mocha (or node:test), you’d need to add additional libraries like Sinon for mocks and nyc for coverage, which Jest handles natively.

That said, if we can use Mocha alongside node:test, it should work fine, even though Mocha isn't as feature-rich in those areas. Totally open to whichever approach the team prefers! 😊

[travis]

ok cool - I lean toward Mocha, but tbh I'd like @alanshaw to weigh in since he's written a lot more of this code (like, infinitely more, as this is really my first freeway contribution as well!) - Alan any preferences/thoughts on testing frameworks here?

[travis]

I've added a few comments, some of them blocking - I took https://conventionalcomments.org/ for a spin on this review, curious if you like it!

I probably won't Approve this PR in general since I've never contributed to Freeway myself - @alanshaw could you take a look at this when you get a chance?

[alanshaw]

Love the docs updates 🙏 thank you.

[alanshaw]

Could you say more about why Jest is useful here? We've typically used node:test for this project, and Mocha (https://mochajs.org/) or AVA (https://github.com/avajs/ava) on other projects - I'm not entirely sure why we've shied away from Jest, but I am a bit hesitant to add yet another testing framework to the mix for our team. Could definitely be convinced - I've used Jest in the past and it's generally pretty good - but need a bit more convincing ;-p

sure thing @travis

I don’t have super strong opinions on this either, but Jest does give you some nice built-in features like mocking, spies, and coverage reports right out of the box. With Mocha (or node:test), you’d need to add additional libraries like Sinon for mocks and nyc for coverage, which Jest handles natively.

That said, if we can use Mocha alongside node:test, it should work fine, even though Mocha isn't as feature-rich in those areas. Totally open to whichever approach the team prefers! 😊

Slightly my bad here, I tried node --test because it was new, but turns out it isn't great so I'd welcome a switch to something else. We also don't use it in any other repo.

I'm mildly against having 2 testing frameworks in the same repo, but at the same time I don't want to waste time here - there's a bunch of tests written now using it so...meh. The point is more that testing exists at all - I don't mind which framework we use. People like consistency though (thinking about the team mostly here as I have a high tolerance for chaos), and using the same framework(s) everywhere allows us to work more quickly and without context switches. We should establish a single/set of frameworks we're comfortable using as a team. I realise that may have changed now. @travis this is something needed to be decided for team norms.

Just a couple of notes: 1) I though jest was legacy/unmaintained? 2) We don't use jest anywhere else.

[Peeja]

FWIW, I'm highly in favor of Jest. It's not legacy; quite the opposite, AFAIK it's where the community primarily is right now. Granted I'm biased by being used to Jest, but I'd love to decide to (in the long run) move everything to Jest.

[alanshaw]

Yes, understood but I'm not convinced it'll be a performance problem.

Related, I'd probably implement the cache as a transparent wrapper around the accounting service and pass the accounting service as a dependency to the rate limiting service.

i.e. something like:

const accounting = Accounting.create({ serviceURL: env.ACCOUNTING_SERVICE_URL })
const cachedAccounting = Accounting.withCache(accounting, env.AUTH_TOKEN_METADATA)
const rateLimiter = RateLimiter.create(cachedAccounting)

const status = await rateLimiter.check(cid, request)
if (status  === RATE_LIMIT_EXCEEDED.YES) {
  // ...

So then it should be easier to test the rate limiter, since the rate limiter is not importing a function that requires a KV and an accounting service. Also easier to test the caching logic in isolation.

...anyways...just a suggestion - non blocking.

[fforbeck]

@alanshaw I've applied pretty much all the suggestions, however, I left some of them to be done in different PRs to minimize the number of changes here.
TODO

• Integration Tests
• Rename URLs from Readme
• Implement the cache as a transparent wrapper around the accounting service and pass the accounting service as a dependency on the rate-limiting service. feat: rate limiter + unit tests + readme #115 (comment)
• Log "rate limit disabled" just once at startup please not every request
• Update Cloudflare workers library to use the latest KV put/get types - which support JSON values
• Refactor Accounting Service: feat: rate limiter + unit tests + readme #115 (comment)
• Create a ticket for: feat: rate limiter + unit tests + readme #115 (comment)
• Add TTL: feat: rate limiter + unit tests + readme #115 (comment)