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.

Sign up for GitHub

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

Jump to bottom

Application error handling #42

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Application error handling #42

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion marketplace/app/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from typing import Optional

from packaging.version import parse
from requests import Response

from ..client import MarketPlaceClient
from .utils import camel_to_snake
Expand Down Expand Up @@ -35,7 +36,14 @@ def get_app(app_id, client: Optional[MarketPlaceClient] = None, **kwargs):

# Getting api version and list of capabilities for the application
app_service_path = f"api/applications/{app_id}"
app_info: dict = client.get(path=app_service_path).json()
app_info_res: Response = client.get(path=app_service_path)
if app_info_res.status_code >= 300:
raise KeyError(
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if a KeyError is the best thing to raise, since it usually applies to dictionaries. Maybe ValueError?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, I agree that the KeyError is probably too specific here. I would suggest to go with a generic RuntimeError.

Further, I think we should be a bit more specific about the kind of status codes that we actually catch. Where is the API for the application-service documented? These are the kind of codes I would expect in this particular context (despite generic 500s etc):

  • 200: The application exists and capabilities are returned.
  • 404: No application with the provided ID exists (or the application is hidden from the user)
  • 403: Missing authorization

I think catching all status codes above a certain error code and then throwing a generic error message is not really helpful to application developers (something I have pointed out before). Issue #38 was originally motivated, because the request resulted in a JSON decode error which is obviously utterly unhelpful.

Copy link
Member

@pablo-de-andres pablo-de-andres Aug 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The codes currently returned are 200, 404 and 401 for Unauthorized (not ´403´). As for documentation... working on it.

f"Querying for application with id {app_id} "
f"failed with code {app_info_res.status_code} "
f"because {app_info_res.text}"
)
app_info: dict = app_info_res.json()
app_api_version = parse(app_info.get("api_version", "0.0.1"))

capabilities = []
Expand Down