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

[api] Added Scoped Throttling #34 #81

Merged

Conversation

ManishShah120
Copy link
Member

@ManishShah120 ManishShah120 commented Dec 26, 2020

closses #34

@ManishShah120
Copy link
Member Author

ManishShah120 commented Dec 26, 2020

@nemesisdesign, @atb00ker have a look 🤔

@coveralls
Copy link

coveralls commented Dec 26, 2020

Coverage Status

Coverage decreased (-0.01%) to 99.329% when pulling b9207f5 on ManishShah120:issues/34-Add-scoped_throttling into b130a44 on openwisp:master.

Copy link
Member

@atb00ker atb00ker left a comment

Choose a reason for hiding this comment

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

2 initial comments:

tests/openwisp2/settings.py Outdated Show resolved Hide resolved
openwisp_ipam/api/views.py Outdated Show resolved Hide resolved
tests/openwisp2/settings.py Outdated Show resolved Hide resolved
openwisp_ipam/apps.py Outdated Show resolved Hide resolved
@nemesifier nemesifier added this to In progress in OpenWISP Priorities for next releases via automation Dec 30, 2020
@ManishShah120 ManishShah120 force-pushed the issues/34-Add-scoped_throttling branch 2 times, most recently from e560c02 to 4d6e446 Compare January 6, 2021 21:52
@ManishShah120
Copy link
Member Author

@nemesisdesign , @atb00ker

@@ -85,6 +85,7 @@
STATIC_URL = '/static/'
EMAIL_BACKEND = 'django.core.mail.backends.console.EmailBackend'
OPENWISP_USERS_AUTH_API = True
OPENWISP_IPAM_API = True
Copy link
Member

Choose a reason for hiding this comment

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

let's avoid this setting, some features of the API is used by the admin as well so it doesn't make sense to turn it off

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay

@@ -9,7 +10,13 @@ class OpenWispIpamConfig(ApiAppConfig):
name = 'openwisp_ipam'
verbose_name = 'IPAM'

API_ENABLED = getattr(settings, 'OPENWISP_IPAM_API')
Copy link
Member

Choose a reason for hiding this comment

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

simply set this to True

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure

@ManishShah120 ManishShah120 force-pushed the issues/34-Add-scoped_throttling branch from 4d6e446 to 55cc03f Compare January 7, 2021 07:25
@ManishShah120
Copy link
Member Author

@nemesisdesign I have done the changes as requested have a look.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Thanks @ManishShah120 👍

OpenWISP Priorities for next releases automation moved this from In progress to Reviewer approved Jan 7, 2021
OpenWISP Priorities for next releases automation moved this from Reviewer approved to In progress Jan 7, 2021
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

I realized something important.
We're not documenting how to change the default throttling settings.
Can it be done without having to modify the code of openwisp-ipam? I mean just by adding something in settings.py. Can you double check this please and let me know?
I think I'll have to open an issue for fw-upgrader too since it should be done there too.

PS: for the commit message, use this:

[feature] Added throttling of API requests #34

See https://www.conventionalcommits.org/.

@ManishShah120
Copy link
Member Author

I realized something important.
We're not documenting how to change the default throttling settings.
Can it be done without having to modify the code of openwisp-ipam? I mean just by adding something in settings.py. Can you double check this please and let me know?

Okay, @nemesisdesign I'll check and see if it can be done and let you know.

I think I'll have to open an issue for fw-upgrader too since it should be done there too.

Yeah, I guess so

@ManishShah120 ManishShah120 force-pushed the issues/34-Add-scoped_throttling branch from 55cc03f to 8376c9a Compare January 7, 2021 18:44
@ManishShah120
Copy link
Member Author

I realized something important.
We're not documenting how to change the default throttling settings.
Can it be done without having to modify the code of openwisp-ipam? I mean just by adding something in settings.py. Can you double check this please and let me know?
I think I'll have to open an issue for fw-upgrader too since it should be done there too.

Hey, @nemesisdesign I confirmed in other openwisp modules and found no sign of documentation related to changing of default throttling settings, So it will be a good idea to introduce in it.

And about the modification of the default throttling settings, yes it can be done but in that case, we will need to move this portion of the code from apps.py

REST_FRAMEWORK_SETTINGS = {
    'DEFAULT_THROTTLE_RATES': {
         'ipam': default_or_test('400/hour', None)},
}

to settings.py file, or if there is any other way possible please let me know. I'll try to learn and do it.

@atb00ker
Copy link
Member

atb00ker commented Jan 9, 2021

to settings.py file, or if there is any other way possible please let me know. I'll try to learn and do it.

This method looks good to me! 😄

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

@ManishShah120 question:

  • if you leave the code as is
  • then add the setting REST_FRAMEWORK_SETTINGS in settings.py as you described (just without using default_or_test which is not meant to be used during configuration)

Does the override work?

@ManishShah120
Copy link
Member Author

Yes, @nemesisdesign, It does override's the previous codes implemented in apps.py and takes effect whatever value is provided in the settings.py file and if we comment out this REST_FRAMEWORK from settings.py then again it takes effect whatever is implemented in apps.py file.

Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Yes, @nemesisdesign, It does override's the previous codes implemented in apps.py and takes effect whatever value is provided in the settings.py file and if we comment out this REST_FRAMEWORK from settings.py then again it takes effect whatever is implemented in apps.py file.

Ok @ManishShah120, thanks for double checking, please can you add a few lines in the README that explain how to override the throttling settings?

@ManishShah120
Copy link
Member Author

@nemesisdesign , Is this Okay.

Copy link
Member

@atb00ker atb00ker left a comment

Choose a reason for hiding this comment

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

Quick look, LGTM! 😄

OpenWISP Priorities for next releases automation moved this from In progress to Reviewer approved Jan 13, 2021
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

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

Great work @ManishShah120 👍

@nemesifier nemesifier merged commit ac9f346 into openwisp:master Jan 13, 2021
OpenWISP Priorities for next releases automation moved this from Reviewer approved to Done Jan 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants