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

#1305 RateLimiting headers #1307

Open
wants to merge 19 commits into
base: develop
Choose a base branch
from

Conversation

jlukawska
Copy link
Contributor

@jlukawska jlukawska commented Jul 31, 2020

Fixes #1305

Proposed Changes

  • It doesn't work because the headers are set on the newly created HttpContext and not the original one that is returned with a response. The fix is that the rate limiting headers are set for the original HttpContext returned by HttpContextAccessor.

@jackletter
Copy link

finnaly HttpResponseFeature.OnStarting was invoked, but HttpResponseFeature.OnStarting method body has nothing

@garybond
Copy link

garybond commented Apr 8, 2022

+1 to get this merged. This is stopping me being able to update a project to net6.0 + Ocelot v18

@jackletter

This comment was marked as off-topic.

@Versa78
Copy link

Versa78 commented Apr 21, 2022

+1 We need this fix as well

@raman-m raman-m changed the title Fix/1305 ratelimiting headers #1305 RateLimiting headers Jul 18, 2023
@raman-m raman-m changed the base branch from master to develop July 18, 2023 11:13
@raman-m raman-m self-requested a review July 18, 2023 11:21
@raman-m raman-m force-pushed the fix/1305-ratelimiting-headers branch from 72da5b1 to 1060ef9 Compare August 2, 2023 13:33
@raman-m
Copy link
Member

raman-m commented Aug 2, 2023

The feature branch has been rebased onto ThreeMammals:develop!

We can start the code review...

@jackletter

This comment was marked as off-topic.

@raman-m
Copy link
Member

raman-m commented Aug 2, 2023

@jackletter commented on Oct 24, 2020:

finnaly HttpResponseFeature.OnStarting was invoked, but HttpResponseFeature.OnStarting method body has nothing

Is this a result of code review (testing)?
If Not, could you review the code please?


P.S. Don't write non-English messages please!

@raman-m
Copy link
Member

raman-m commented Aug 2, 2023

@garybond @Versa78 @neetra @nls44
Welcome to the code review!
Your opinion is highly required.

@raman-m raman-m added bug Identified as a potential bug proposal Proposal for a new functionality in Ocelot needs feedback Issue is waiting on feedback before acceptance labels Aug 2, 2023
@raman-m raman-m added Rate Limiting Ocelot feature: Rate Limiting May-Jun'24 May-June 2024 release and removed needs feedback Issue is waiting on feedback before acceptance labels Apr 30, 2024
@raman-m raman-m added this to the May-June'24 milestone Apr 30, 2024
@raman-m
Copy link
Member

raman-m commented May 1, 2024

Great! Please inform me once the development is complete. And I'll invite another team members to review...

@jlukawska jlukawska requested a review from raman-m May 4, 2024 19:32
@raman-m
Copy link
Member

raman-m commented May 7, 2024

Hello Ms. J, thank you for your proactivity!
Today I've merged important PR #1592 into develop, and now we have unresolved conflicts.
Since only 4 files have changed, we could simply copy the changes to a clean, rebased branch.
Nonetheless, rebasing onto ThreeMammals:develop is required!

@jlukawska
Copy link
Contributor Author

Hello Ms. J, thank you for your proactivity! Today I've merged important PR #1592 into develop, and now we have unresolved conflicts. Since only 4 files have changed, we could simply copy the changes to a clean, rebased branch. Nonetheless, rebasing onto ThreeMammals:develop is required!

Sorry, but I'm not sure how to rebase, I haven't done it before. Do you mean "Sync fork" on the branch and "Discard 15 commits"? And make changes again to this branch manually?

I think resolving conflicts using merge from develop would be easier and more readable, because it preserves the history.

@raman-m
Copy link
Member

raman-m commented May 9, 2024

Apologies, I was mistaken! Rebasing is not the recommended method for resolving conflicts after extensive refactoring. A manual approach is preferable. Also, avoid using the Sync fork button for feature branches, as it poses a high risk of losing changes on the origin.

I think resolving conflicts using merge from develop would be easier and more readable, because it preserves the history.

I wouldn't advise a simple merge from develop. And simple merge from target branch doesn't guarantee linear history ))
Given the complex situation after extensive refactoring, the Visual Studio Merge tool may present many surprises. A manual approach, as described above, is recommended. See "2nd Step"

1st Step

The current situation with your develop:

This branch is 2 commits ahead of, 3 commits behind ThreeMammals/Ocelot:develop.

Use Sync Fork to resolve the problem!
After successful sync of your develop you should see the message: "This branch is up to date with ThreeMammals/Ocelot:develop."
Example is in my fork: https://github.com/raman-m/Ocelot/tree/develop

2nd Step

If you're up for this git challenge, here's my understanding:

  • Create a local backup of the branch: git branch fix/1305-ratelimiting-headers-BACKUP
  • Fetch all updates and pull the latest state of develop: git fetch --all followed by git switch develop and git pull, ensuring your develop matches the head repository: both develop must be identical.
  • Hard reset the feature branch to the HEAD of develop: git switch fix/1305-ratelimiting-headers followed by git reset --hard develop
  • Manually apply changes to the current 4 files, compile, run tests, and then force push to origin: git push --force

jlukawska and others added 3 commits May 10, 2024 08:02
Merge from upstream repository
…ing-headers

# Conflicts:
#	src/Ocelot/RateLimiting/Middleware/RateLimitingMiddleware.cs
#	test/Ocelot.AcceptanceTests/ClientRateLimitTests.cs
#	test/Ocelot.UnitTests/RateLimit/ClientRateLimitMiddlewareTests.cs
@jlukawska
Copy link
Contributor Author

I've resolved conflicts using my method - by doing the merge. I've never had any major problems with it.

@raman-m
Copy link
Member

raman-m commented May 14, 2024

It's up to you! Merge-commits aren't an issue since we use the Squash option when merging PRs into develop. However, merge-commits become problematic with the classic merge-commit method, leading to a non-linear history issue.
Thanks for resolving the conflicts!

Please ensure your develop branch is updated and matches the head repository. Currently, there's a discrepancy in commits:

This branch is 3 commits ahead of, 1 commit behind ThreeMammals/Ocelot:develop.

If you're unsure how to sync your develop branch with the head repository, consider adding me as a temporary contributor, and I'll use the Sync fork button to align them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Identified as a potential bug May-Jun'24 May-June 2024 release proposal Proposal for a new functionality in Ocelot Rate Limiting Ocelot feature: Rate Limiting
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"DisableRateLimitHeaders": false is not showing X-Rate-Limit and Retry-After headers in response
8 participants