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

Fix API v2 exception on incorrect API key. Fixes #5425 #6703

Merged
merged 8 commits into from
May 20, 2019

Conversation

sharkykh
Copy link
Contributor

@sharkykh sharkykh commented May 20, 2019

Fixes #5425, added 2 small tests for that
Fixes RESTful HTTPErrors causing real exceptions.

@ghost
Copy link

ghost commented May 20, 2019

DeepCode analyzed this pull request.
There are no new issues. 1 info report was fixed.

Click to see more details.

@sharkykh
Copy link
Contributor Author

Working on implementing a couple of tests, and another small bug:

Traceback (most recent call last):
  File "C:\Dev\Medusa\ext\tornado\web.py", line 1559, in _execute
    raise HTTPError(405)
tornado.web.HTTPError: HTTP 405: Method Not Allowed

p0psicles
p0psicles previously approved these changes May 20, 2019
@p0psicles
Copy link
Contributor

Ooh then ignore my approval

@sharkykh
Copy link
Contributor Author

sharkykh commented May 20, 2019

Sorry, took me a while to figure all of this stuff out. I think the code for the multi-threaded API looks much better now.
For the record -
#5425 - The issue was with the prepare method.
HTTPError - A combination of things, I rewrote the error handling code. Like I said above, should be much better now.

edit: oh come on...

This reverts commit b291939.
sharkykh added 2 commits May 20, 2019 19:21
- Fix requests getting logged twice.
- Fix API v2 HTTPErrors getting logged as warnings.
@sharkykh sharkykh requested a review from p0psicles May 20, 2019 16:39
@sharkykh
Copy link
Contributor Author

All good now.

except Exception as error:
self._handle_request_exception(error)
result = self._check_authentication()
return method(*args, **kwargs) if result is None else result
Copy link
Contributor

Choose a reason for hiding this comment

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

Your kinda asuming that the method call cant raise exceptions?

Copy link
Contributor Author

@sharkykh sharkykh May 20, 2019

Choose a reason for hiding this comment

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

I'll try to explain as best I can.
Basically - yes.
But the difference is the RequestHandler class has an exception handler, and we handle everything in write_error now.

def write_error(self, status_code, *args, **kwargs):
"""Only send traceback if app.DEVELOPER is true."""
response = None
exc_info = kwargs.get('exc_info', None)
if exc_info and isinstance(exc_info[1], HTTPError):
error = exc_info[1].log_message or exc_info[1].reason
response = self.api_response(status=status_code, error=error)
elif app.DEVELOPER and exc_info:
self.set_header('content-type', 'text/plain')
self.set_status(500)
for line in traceback.format_exception(*exc_info):
self.write(line)
else:
response = self._internal_server_error()

HTTPErrors are handled as RESTful responses,
And anything other than HTTPError is handled as an actual exception and sends "Internal Server Error" as the response (or the traceback, if app.DEVELOPER is enabled, like before)

Copy link
Contributor Author

@sharkykh sharkykh May 20, 2019

Choose a reason for hiding this comment

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

Your kinda asuming that the method call cant raise exceptions?

Sorry, I actually misread that.
Quite the opposite, I plan on it, but as I explained in the comment above, it's all being handled (just in one location and not everywhere)

@sharkykh
Copy link
Contributor Author

Retry Deepcode

@ghost
Copy link

ghost commented May 20, 2019

DeepCode analyzed this pull request.
There is 1 new info report. 1 warning and 2 info reports were fixed.

Click to see more details.

@p0psicles p0psicles merged commit c1b2e68 into develop May 20, 2019
@p0psicles p0psicles deleted the bugfix/api-v2-internal-error branch May 20, 2019 18:03
Thilas pushed a commit to Thilas/Medusa that referenced this pull request Jun 5, 2019
…usa#6703)

* Fix API v2 exception on incorrect API key.

Fixes pymedusa#5425

* Update changelog

* Fix authentication endpoint

* Test bad auth conditions

* Fix RESTful and non-RESTful error handling

* Fix API pagination

This reverts commit b291939.

* Fix Tornado request logging

- Fix requests getting logged twice.
- Fix API v2 HTTPErrors getting logged as warnings.

* Fix `write_error` for `raise HTTPError(code, message)`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants