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

Limit requests per hour #96

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

tjasmith
Copy link
Collaborator

Fixes: #4

The license sumission is reduced to 100 per hour by a particular user.
When the limit is reached, the modal below is shown.
quota2

@Ugtan
Copy link
Collaborator

Ugtan commented Mar 16, 2019

@tjasmith Why add a whole new module when you can simply do it with drf throttling?

@tjasmith
Copy link
Collaborator Author

@Ugtan
Thanks for the question.

I decided to go on with django rate limit because it can be used on normal django views(which is the view on which it was supposed to be implemented.)
Moreover, it can be used on class-based views(if we decide to switch to those), and can also be applied with a mixin.

I read about django rest framework throttling, but it didn't suit my needs(adding this to a normal django view), while the module which I choose did.

@goneall
Copy link
Member

goneall commented Mar 17, 2019

Will this limit the rate on all requests or just the license submittal? The request I'm most concerned about is the check license. It would be good to limit all of the requests that can be made through the API.

@tjasmith
Copy link
Collaborator Author

@goneall
This is just for the license submittal request. Ok, I shall now implement this on all other requests.

@rtgdk
Copy link
Collaborator

rtgdk commented Mar 18, 2019

@tjasmith Also, write tests to check whether this modal is actually shown or not. You can use selenium to automate these requests.

@tjasmith
Copy link
Collaborator Author

@rtgdk

Ok. Thanks for the hint.

@techytushar techytushar added the wip Work in Progress label Mar 23, 2019
@tjasmith
Copy link
Collaborator Author

tjasmith commented Mar 25, 2019

@goneall I have limited the rate on all django rest framework apis, and on the following post requests:

  • check license
  • validate
  • compare
  • convert
  • xml upload
  • validate xml
  • submit license

@rtgdk I have written tests for the return of the requests using django's RequestFactory(to be able to simulate that the rate limit has been reached).
Here is an image of the tests passing:
limitrequeststests

Copy link
Member

@goneall goneall left a comment

Choose a reason for hiding this comment

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

Changes look good to me. There are some changes in the white space which is causing some extra diffs. Are these necessary? I'm concerned it may create more merge conflicts.

@goneall goneall requested a review from rtgdk March 25, 2019 16:59
@rtgdk
Copy link
Collaborator

rtgdk commented Mar 26, 2019

@tjasmith In the test, you should send 100 request from selenium(either one by one or a bunch of them in parallel [In different tabs/windows of the browser]). The test you have written tests whether the view function is returning the correct error or not. You can keep this test. But add the selenium test to test whether this error is shown in real or not on the UI.

@tjasmith
Copy link
Collaborator Author

@rtgdk Ok, On it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wip Work in Progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants