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

[refactor] Replace conditional ladder in requests utils #237

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

[refactor] Replace conditional ladder in requests utils #237

Changes from 12 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
562eab5
Replace conditional ladder
nikochiko Dec 30, 2019
4a121dc
Fix unused variables
nikochiko Dec 30, 2019
964652a
Fix unused import
nikochiko Dec 30, 2019
c696825
Add debugging block
nikochiko Dec 30, 2019
5ac3f5b
Debug further
nikochiko Dec 30, 2019
3f3563e
Remove debug block
nikochiko Dec 30, 2019
76cb049
DEBUG
nikochiko Dec 30, 2019
52fa64f
Fix typo
nikochiko Dec 30, 2019
549fb29
Edit debug block
nikochiko Dec 30, 2019
561bcfa
Fix debug
nikochiko Dec 30, 2019
d8e4ec7
Fix error
nikochiko Dec 30, 2019
1f3f431
Remove TODO
nikochiko Dec 30, 2019
a566886
Exit when passed method is unsupoorted
nikochiko Dec 31, 2019
4359f61
Fix linting
nikochiko Dec 31, 2019
6f158d2
Change echo + SystemExit -> ValueError
nikochiko Dec 31, 2019
667926a
Add new line
nikochiko Jan 1, 2020
a0fac77
Update Exception for unsupported method
nikochiko Jan 1, 2020
611188b
Update error handling
nikochiko Jan 1, 2020
57fa1cc
Generalize the handling message
nikochiko Jan 1, 2020
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
100 changes: 31 additions & 69 deletions evalai/utils/requests.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import json
import requests
import sys

Expand All @@ -13,73 +12,46 @@
def make_request(path, method, files=None, data=None):
url = "{}{}".format(get_host_url(), path)
headers = get_request_header()
if method == "POST":
nikochiko marked this conversation as resolved.
Show resolved Hide resolved
files = {"input_file": open(files, "rb")} if files else None
data = {"status": "submitting"}

if method == "GET":
try:
response = requests.get(url, headers=headers)
response.raise_for_status()
except requests.exceptions.HTTPError as err:
if response.status_code in EVALAI_ERROR_CODES:
validate_token(response.json())
echo(
style(
"\nError: {}\n".format(response.json().get("error")),
fg="red",
bold=True,
)
)
else:
echo(err)
sys.exit(1)
except requests.exceptions.RequestException:
try:
response = requests.request(method, url, data=data, headers=headers, files=files)
response.raise_for_status()
except requests.exceptions.HTTPError as e:
if response.status_code in EVALAI_ERROR_CODES:
validate_token(response.json())
echo(
style(
"\nCould not establish a connection to EvalAI."
" Please check the Host URL.\n",
bold=True,
"\nError: {}\n".format(response.json().get("error")),
nikochiko marked this conversation as resolved.
Show resolved Hide resolved
fg="red",
bold=True,
)
)
sys.exit(1)
return response.json()
elif method == "POST":
if files:
files = {"input_file": open(files, "rb")}
else:
files = None
data = {"status": "submitting"}
try:
response = requests.post(
url, headers=headers, files=files, data=data
)
response.raise_for_status()
except requests.exceptions.HTTPError as err:
if response.status_code in EVALAI_ERROR_CODES:
validate_token(response.json())
echo(
style(
"\nError: {}\n"
"\nUse `evalai challenges` to fetch the active challenges.\n"
"\nUse `evalai challenge CHALLENGE phases` to fetch the "
"active phases.\n".format(response.json()["error"]),
fg="red",
bold=True,
)
)
else:
echo(err)
sys.exit(1)
except requests.exceptions.RequestException:
echo(
style(
"\nCould not establish a connection to EvalAI."
" Please check the Host URL.\n",
"Use `evalai challenges` to fetch the active challenges."
Copy link
Contributor Author

@nikochiko nikochiko Dec 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@vkartik97 As in, it wasn't there in the code previously. It is not fully supported. Like, it might not make sense to show this message in that case (for example for DELETE method). Also, I'm not sure if we have the support for this in the backend (please correct me if I missed something).

Copy link
Contributor Author

@nikochiko nikochiko Dec 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@vkartik97 It would be better to have some error messages or blocking requests for security reasons (example using DELETE method). It can still be possible with a custom script, but a warning block here can make everything clearer. For reference: https://www.owasp.org/index.php/Test_HTTP_Methods_(OTG-CONFIG-006)

Copy link
Member

Choose a reason for hiding this comment

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

I am thinking of having something like:

if method in [PUT,PATCH, DELETE] then:
    return or/and throw error
else:
.
.
.

What are your opinions on this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, looks good to me.
I checked the exceptions list provided by requests package but it doesn't include an InvalidMethodError. So I was thinking we could echo a message saying the method is unsupported and then perform a sys.exit.
That is:

if method in ["PUT", "PATCH", "DELETE"]:
  echo("Method not supported ...")
  sys.exit(1)
else:
.
.
.

Copy link
Member

Choose a reason for hiding this comment

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

Instead of echo and exit, what are opinions on using raise Exception('{METHOD} not supported by make_request')? The reason for using exceptions is that the method make_request should have a behavior to inform the user/programmer that a exceptional behavior has occurred, here it is "PUT", "PATCH", "DELETE" is not supported.
Please let me know if this doesn't seem correct way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vkartik97 I think that is a valid point. I agree raising an Exception will be better 👍
I will update the PR with the same.

"Use `evalai challenge CHALLENGE phases` to fetch the "
"active phases.",
bold=True,
fg="red",
)
)
sys.exit(1)
response = json.loads(response.text)
else:
echo(e)
sys.exit(1)
except requests.exceptions.RequestException:
echo(
style(
"\nCould not establish a connection to EvalAI."
" Please check the Host URL.\n",
bold=True,
fg="red",
)
)
sys.exit(1)

if method == "POST":
echo(
style(
"\nYour docker file is successfully submitted.\n",
Expand All @@ -90,19 +62,9 @@ def make_request(path, method, files=None, data=None):
echo(
style(
"You can use `evalai submission {}` to view this submission's status.\n".format(
response.get("id")
response.json().get("id")
),
bold=True,
fg="white"
)
)
return response
elif method == "PUT":
# TODO: Add support for PUT request
pass
elif method == "PATCH":
# TODO: Add support for PATCH request
pass
elif method == "DELETE":
# TODO: Add support for DELETE request
pass
return response.json()