[refactor] Replace conditional ladder in requests utils

[nikochiko]

As a Google Code-In task.

Changes proposed:

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

[krtkvrm]

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

[nikochiko]

Okay. That's done! :D 👍

[nikochiko]

@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]

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?

[nikochiko]

@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).

[nikochiko]

@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)

[krtkvrm]

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?

[nikochiko]

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:
.
.
.

[krtkvrm]

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.

[nikochiko]

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

[nikochiko]

@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.

[Ram81]

@nikochiko nice work LGTM 👍

[nikochiko]

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