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

[refactor] Replace conditional ladder in requests utils #237

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

Conversation

nikochiko
Copy link
Contributor

@nikochiko nikochiko commented Dec 30, 2019

As a Google Code-In task.

Changes proposed:

Replace conditional ladder in requests utils, with a single requests.request call.

Comment on lines 70 to 72
# TODO: Add support for PUT request
# TODO: Add support for PATCH request
# TODO: Add support for DELETE request
Copy link
Member

Choose a reason for hiding this comment

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

Please remove these TODOs as the requests.request will implement this as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay. That's done! :D 👍

@nikochiko
Copy link
Contributor Author

@vkartik97 @RishabhJain2018 Shall I explicitly remove support for PUT, PATCH, DELETE requests?
I realized in the last comment that it accepts all methods and that could give unexpected results.

@krtkvrm
Copy link
Member

krtkvrm commented Dec 30, 2019

I realized in the last comment that it accepts all methods and that could give unexpected results.

Can you please provide detailed information regarding this?

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

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

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.

evalai/utils/requests.py Outdated Show resolved Hide resolved
evalai/utils/requests.py Outdated Show resolved Hide resolved
evalai/utils/requests.py Outdated Show resolved Hide resolved
evalai/utils/requests.py Show resolved Hide resolved
@nikochiko

This comment has been minimized.

@nikochiko
Copy link
Contributor Author

nikochiko commented Jan 1, 2020

@Ram81 @vkartik97 I have changed the handling mechanism a bit, but the behavior of the function is the same, and with the suggested additions.

  1. Replace the conditional ladder using a single requests.request call.
  2. Generalize all exceptions caused by requests into a single block.
  3. Use same template for all error messages. This will help keep it consistent.
  4. Shorten the code (78 lines ->23 lines)
  5. Remove multiple except blocks.

The efficiency and readability of the code has increased.
Please review.

Copy link
Member

@Ram81 Ram81 left a comment

Choose a reason for hiding this comment

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

@nikochiko nice work LGTM 👍

@nikochiko
Copy link
Contributor Author

@Ram81 Can I get an approval on the GCI page?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants