#1590 Use correct interval for request counting

[sergio-str]

Fixes #1590

• Rate Limiting works incorrectly if PeriodTimespan value is greater than Period #1590

Incrementing request counter if request time is in the interval
[FirstRequestTime, FirstRequestTime + PeriodTimespan] seems is incorrect. PeriodTimespan is a "ban" period, not a period for request counting. In case of PeriodTimespan interval significantly exceeds Period, Rate Limiting feature counts number of requests longer than expected. It leads to false rate limit exceeding error.

Proposed Changes

Increment request counter when request time within interval
[FirstRequestTime, FirstRequestTime + RateLimitingSettingPeriod] respectively to the Rate Limiting docs.

[raman-m]

Hi Sergii!

Thanks for the solution and interest in the project!

I've resolved one merge conflict. And pipelines are green.
Could we discuss the issue #1590 first plz?

[sergio-str]

Yes, sure.

[raman-m]

Sergii,

Now I see that at least your changes don't break current features.
But we have 2 parents in commit Merge branch 'develop' into Rate_linit_counting_interval_fix__issue_#….
I'm not a fan of merge commits with two parents after conflicts resolution, because they make non-linear history in base branch.
You need to put your commits on the top commit from base:develop!

Could I ask you to re-create PR please? Unfortunately your feature branch is broken. Your forked develop branch must be the same as develop branch in source repo!

• Just backup files please with new changes.
• Remove current fork, and make new fork to be sure you have the same history as in source repo having all commits up to date
  Or pull down all base:develop commits
• Don't apply changes in develop, just keep it updated via pull operation always, just for future usage
• Create new feature branch from develop one, for example, feat/1590
• Apply new changes from files backup.
• Create new PR
• Do not forget to link both PRs (old and new ones) for tracking purposes

As soon as you've created new PR, this PR will be closed as duplicate.
Thanks!

[sergio-str]

Hello Raman.
Sorry for significant delay, many things required my attention.
First, let me please confirm that you title and descr update is correct.
As for my implementation, I have a small doubt concerning timing calculation. I need to recheck them, and, perhaps, extend existing tests. Unfortunately I could not promise I will have solved it fast, but I will do all my best to finish with code changes in 2-3 weeks.
Cheers
S.

[raman-m]

No worries, Sergii!
Take some time to finish the solution!
Tom will wait for your fix for a couple of years. 🤣 It's a joke!

[raman-m]

Sergii,
Please merge PR 1 to update your repo!
After merging let's try to find the best option to update feature branch which has some merge conflicts for rebase-operation...
Thank you!

[raman-m]

sergio-str commented on May 29, 2023:

Unfortunately I could not promise I will have solved it fast, but I will do all my best to finish with code changes in 2-3 weeks.

Well... 3 weeks have been passed.
Any progress on that?
May I help you with this PR by some pair programming?

[raman-m]

The feature branch has been rebased onto ThreeMammals:develop!
Welcome to code review!
I guess we can develop and review the code simultaneously.

[raman-m]

⚠️ Development Complete❕

@RaynaldM @ggnaegi Could you please review?
The remaining work involves testing the user scenario from #1590. I will add a few tests in the next day or two.
I anticipate that the user scenario tests will be completed by Monday.

The code quality was unexpectedly poor. It pertains to the oldest Ocelot feature, which lacked unit tests but had some rudimentary acceptance tests. Consequently, I decided to refactor the code slightly to enable unit test writing. The test coverage is now satisfactory. This feature was first introduced in PR #37 in 2017.

[ggnaegi]

@raman-m Yes, around midnight your time!

[ggnaegi]

Ok, some questions from my side, but then ok. Thanks!

[raman-m]

All code review issues have been resolved! Awaiting your approval, @ggnaegi.
...after implementing the subsequent tests for bug/user scenarios. 😄

[raman-m]

Approved, but the rest of work is 👇

• Update feature docs. Done in commit 1ed45d0 ✔️

[raman-m]

Ready for delivery ✅

• Code review ✔️✔️
• Team approvals ✔️✔️
• Unit testing ✔️✔️ → RateLimitingTests and RateLimitingMiddlewareTests
• Acceptance testing ✔️ → ClientRateLimitingTests
• Feature docs ✔️ → ratelimiting.rst → commit 1ed45d0 where the entire document was reviewed and revised to reflect the new design