fix: Added error handler for status 429 #6678

Rits1272 · 2019-12-19T04:00:36Z

Fixes #6659

Short description of what this resolves: Added Error handler for status 429 in the errors.py file

I have read the Contribution & Best practices Guide and my PR follows them.
My branch is up-to-date with the Upstream development branch.
The unit tests pass locally with my changes
I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

All the functions created/modified in this PR contain relevant docstrings.

Rits1272 · 2019-12-19T04:01:12Z

Please review @codedsun

codecov · 2019-12-19T04:04:12Z

Codecov Report

Merging #6678 into development will decrease coverage by 0.2%.
The diff coverage is 20.54%.

@@               Coverage Diff               @@
##           development    #6678      +/-   ##
===============================================
- Coverage         65.6%   65.39%   -0.21%     
===============================================
  Files              300      300              
  Lines            15258    15319      +61     
===============================================
+ Hits             10010    10018       +8     
- Misses            5248     5301      +53

Impacted Files	Coverage Δ
app/instance.py	`91.3% <0%> (+0.49%)`	⬆️
app/models/order.py	`90.19% <100%> (+0.19%)`	⬆️
app/api/custom/orders.py	`26.89% <18.46%> (-14.38%)`	⬇️
app/api/helpers/order.py	`23.2% <20%> (-0.77%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4ca8411...9c60849. Read the comment docs.

codedsun

Also register this error in the app

Rits1272 · 2019-12-19T09:45:57Z

Kindly review again @codedsun

kushthedude

Follow Semantic PR Naming for your PR title.

kushthedude · 2019-12-19T17:23:42Z

Also, Squash your commits.

Rits1272 · 2019-12-20T03:37:27Z

Sure @kushthedude and thanks for reviewing @codedsun, @kushthedude and @mrsaicharan1!

Rits1272 · 2019-12-20T04:02:19Z

@kushthedude I have changed the PR title in accordance with semantic PR naming and also squashed the commits.

codedsun

where are you using theRequestLimitError? Please register its handler in the app also

app/__init__.py

For pep8 style Registered error handler Registered error handler update Added and registered error handler for status 429

Rits1272 · 2019-12-20T10:17:11Z

Please @codedsun check. I think I have registered the error handler successfully

app/api/helpers/errors.py

kushthedude · 2019-12-20T11:40:18Z

app/api/helpers/errors.py

+
+@error_blueprint.app_errorhandler(429)
+def handle_429(error):
+    raise RequestLimitError({'source': ''}, error.description).respond()


Why this? Keep it consistent with other handlers

Can you please tell me where other error handlers are implemented @kushthedude? Because I first tried to find how other handlers were registered but was not able to find them. Also, issue #6665 says to register custom error handlers that means previously no custom error handlers are registered. Please guide me further if I am going wrong

They are present in __init__.py of app

And you can't just raise in an error handler itself. The raise is caught in error handler so that proper response can be returned from there. The app will crash if you raise in the error handler itself which has caught the error, hence no point of catching at all

Ok @iamareebjamal. Working on it. Thanks for reviewing!

iamareebjamal · 2019-12-21T19:45:56Z

app/api/helpers/errors.py

+    Default class for 429 Error
+    """
+    status = 429
+    title = 'Request Limit Exceeded'


Not needed. Try to understand the issue

iamareebjamal · 2019-12-21T19:46:09Z

tests/all/integration/api/helpers/test_errors.py

+            # Request limit Error
+            request_limit_error = RequestLimitError({'source': ''},
+                                                    'Request limit exceeded')
+            self.assertEqual(request_limit_error.status, 429)


Will remove it

app/api/helpers/errors.py

houndci-bot · 2019-12-22T05:52:02Z

app/api/helpers/errors.py

@@ -24,7 +22,7 @@ def __init__(self, source, detail, title=None, status=None):
        if title is not None:
            self.title = title
        if status is not None:
-            self.status = status
+            self.status = staRequestLimitErrortus


undefined name 'staRequestLimitErrortus'

houndci-bot · 2019-12-22T05:52:02Z

app/__init__.py

+@app.errorhandler(429)
+def request_limit_error(err):
+    return make_response(
+            jsonify(error="ratelimit exceeded %s" % err.description)


whitespace before ','

houndci-bot · 2019-12-22T06:01:37Z

app/api/helpers/errors.py

@@ -77,4 +76,4 @@ class BadRequestError(ErrorResponse):
    Default class for 400 Error
    """
    status = 400
-    title = 'Bad Request'
+    title = 'Bad Request'


blank line at end of file

houndci-bot · 2019-12-22T06:01:37Z

app/__init__.py

 if __name__ == '__main__':
-    current_app.run()
+    current_app.run()


blank line at end of file

…6679) This reverts commit 85045c4.

…6656)" (#6679)" (#6684) This reverts commit 21d7635.

Updates the requirements on [geoip2](https://github.com/maxmind/libmaxminddb) to permit the latest version. - [Release notes](https://github.com/maxmind/libmaxminddb/releases) - [Changelog](https://github.com/maxmind/libmaxminddb/blob/master/Changes.md) - [Commits](https://github.com/maxmind/libmaxminddb/commits) Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Updates the requirements on [celery](https://github.com/celery/celery) to permit the latest version. - [Release notes](https://github.com/celery/celery/releases) - [Changelog](https://github.com/celery/celery/blob/master/Changelog.rst) - [Commits](celery/celery@v4.3.0...4.4.0) Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Updates the requirements on [sqlalchemy](https://github.com/sqlalchemy/sqlalchemy) to permit the latest version. - [Release notes](https://github.com/sqlalchemy/sqlalchemy/releases) - [Changelog](https://github.com/sqlalchemy/sqlalchemy/blob/master/CHANGES) - [Commits](https://github.com/sqlalchemy/sqlalchemy/commits) Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Updates the requirements on [coverage](https://github.com/nedbat/coveragepy) to permit the latest version. - [Release notes](https://github.com/nedbat/coveragepy/releases) - [Changelog](https://github.com/nedbat/coveragepy/blob/master/CHANGES.rst) - [Commits](nedbat/coveragepy@coverage-4.5...coverage-5.0) Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

Added error handler for status 429

…into development

…server into development

houndci-bot · 2019-12-23T11:15:21Z

app/api/helpers/errors.py

@@ -15,7 +15,7 @@ class ErrorResponse:

    def __init__(self, source, detail, title=None, status=None):
        """Initialize a jsonapi ErrorResponse Object
-
+        


blank line contains whitespace

app/__init__.py

iamareebjamal · 2019-12-23T13:52:50Z

app/__init__.py

+def ratelimit_handler(error):
+    exc = JsonApiException({'pointer': ''}, str(error))
+    return make_response(json.dumps(jsonapi_errors([exc.to_dict()])), exc.status,
+                        {'Content-Type': 'application/vnd.api+json'})


This is not what we want. Did you even test what it returns?

Ok, Working on it @iamareebjamal.

…server into development

Rits1272 · 2020-01-06T14:21:47Z

Please review @iamareebjamal

iamareebjamal · 2020-01-09T14:37:52Z

No need for jsonapi errors, source or details

…server into development

Rits1272 · 2020-01-09T17:36:29Z

Made the changes @iamareebjamal

iamareebjamal · 2020-01-09T19:02:54Z

app/instance.py

+             'title': 'Request Limit Exceeded',
+             }
+    return make_response(json.dumps(dict_), 429,
+                         {'Content-Type': 'application/vnd.api+json'})


Properly format and indent. Remove dict_ variable

Rits1272 · 2020-01-10T14:10:52Z

I made the suggested changes @iamareebjamal

iamareebjamal · 2020-01-10T14:55:41Z

app/instance.py

+            'status': 429,
+            'title': 'Request Limit Exceeded'
+        }),
+        429, {


{ should start from next line

Rits1272 · 2020-01-10T16:11:28Z

Made the changes @iamareebjamal

mrsaicharan1

Looks good but try to go through the issue thoroughly before sending in a fix from next time. Good start!

Rits1272 · 2020-01-11T05:51:29Z

Thanks a lot, @mrsaicharan1 and @iamareebjamal for having patience with me. I will try my best in improving my PR from next time.

codedsun requested changes Dec 19, 2019

View reviewed changes

mrsaicharan1 approved these changes Dec 19, 2019

View reviewed changes

kushthedude suggested changes Dec 19, 2019

View reviewed changes

mrsaicharan1 previously approved these changes Dec 20, 2019

View reviewed changes

Rits1272 changed the title ~~Added error handler for status 429~~ feat : Added error handler for status 429 Dec 20, 2019

auto-label bot added the feature label Dec 20, 2019

Rits1272 dismissed mrsaicharan1’s stale review via 1d5c9fa December 20, 2019 03:58

codedsun requested changes Dec 20, 2019

View reviewed changes

houndci-bot reviewed Dec 20, 2019

View reviewed changes

app/__init__.py Outdated Show resolved Hide resolved

Added Error handler for status 429

7e824fe

For pep8 style Registered error handler Registered error handler update Added and registered error handler for status 429

kushthedude suggested changes Dec 20, 2019

View reviewed changes

iamareebjamal reviewed Dec 21, 2019

View reviewed changes

houndci-bot reviewed Dec 22, 2019

View reviewed changes

kushthedude and others added 8 commits December 22, 2019 11:32

Revert "feat: Optimize marshamllow jsonapi link generation (#6656)" (#…

dfcad5b

…6679) This reverts commit 85045c4.

Revert "Revert "feat: Optimize marshamllow jsonapi link generation (#…

f6bd4b1

…6656)" (#6679)" (#6684) This reverts commit 21d7635.

fix: Order Post route view (#6683)

f5c7235

Added error handler for status 429

2305a56

Added error handler for status 429

Rits1272 added 4 commits December 23, 2019 16:36

Merge branch 'master' of https://github.com/fossasia/open-event-server …

569a696

…into development

Merge branch 'development' into development

9063705

Added error handler for status 429

9ef2e0d

Merge branch 'development' of https://github.com/Rits1272/open-event-…

cf80899

…server into development

houndci-bot reviewed Dec 23, 2019

View reviewed changes

Added error handler for status 429

d0419ef

iamareebjamal reviewed Dec 23, 2019

View reviewed changes

Rits1272 added 4 commits January 3, 2020 18:29

Merge branch 'development' of https://github.com/fossasia/open-event-…

0c65e38

…server into development

Added error handler for status 429

4ca8411

Merge branch 'development' into development

3403d03

Merge branch 'development' into development

a6b749d

Merge branch 'development' into development

1584492

Rits1272 added 2 commits January 9, 2020 23:00

Added 429 error handler

ffd433d

Merge branch 'development' of https://github.com/Rits1272/open-event-…

9fb7911

…server into development

iamareebjamal reviewed Jan 9, 2020

View reviewed changes

Added error handler for status 429

2a1ec01

iamareebjamal reviewed Jan 10, 2020

View reviewed changes

app/instance.py Outdated

'status': 429,

'title': 'Request Limit Exceeded'

}),

429, {

Copy link

Member

iamareebjamal Jan 10, 2020

Choose a reason for hiding this comment

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

{ should start from next line

Added error handler for status 429

9c60849

iamareebjamal approved these changes Jan 10, 2020

View reviewed changes

iamareebjamal changed the title ~~feat : Added error handler for status 429~~ fix: Added error handler for status 429 Jan 10, 2020

auto-label bot added fix and removed feature labels Jan 10, 2020

mrsaicharan1 approved these changes Jan 11, 2020

View reviewed changes

iamareebjamal merged commit 3fad651 into fossasia:development Jan 13, 2020

		@@ -15,7 +15,7 @@ class ErrorResponse:

		def __init__(self, source, detail, title=None, status=None):
		"""Initialize a jsonapi ErrorResponse Object

fix: Added error handler for status 429 #6678

fix: Added error handler for status 429 #6678

Conversation

Rits1272 commented Dec 19, 2019

Short description of what this resolves: Added Error handler for status 429 in the errors.py file

Rits1272 commented Dec 19, 2019

codecov bot commented Dec 19, 2019 • edited

Codecov Report

codedsun left a comment

Choose a reason for hiding this comment

Rits1272 commented Dec 19, 2019

kushthedude left a comment

Choose a reason for hiding this comment

kushthedude commented Dec 19, 2019

Rits1272 commented Dec 20, 2019

Rits1272 commented Dec 20, 2019

codedsun left a comment

Choose a reason for hiding this comment

Rits1272 commented Dec 20, 2019

Choose a reason for hiding this comment

Rits1272 Dec 20, 2019 • edited

Choose a reason for hiding this comment

iamareebjamal Dec 21, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rits1272 commented Jan 6, 2020

iamareebjamal commented Jan 9, 2020

Rits1272 commented Jan 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rits1272 commented Jan 10, 2020

Choose a reason for hiding this comment

Rits1272 commented Jan 10, 2020

mrsaicharan1 left a comment

Choose a reason for hiding this comment

Rits1272 commented Jan 11, 2020

codecov bot commented Dec 19, 2019 •

edited

Rits1272 Dec 20, 2019 •

edited

iamareebjamal Dec 21, 2019 •

edited