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

Implement support for using a JSON file as request body. #43

Closed
wants to merge 2 commits into from
Closed

Implement support for using a JSON file as request body. #43

wants to merge 2 commits into from

Conversation

sehnsucht13
Copy link

@sehnsucht13 sehnsucht13 commented Mar 29, 2021

[Feature]: Support for using files as request bodies. May resolve issue #8

Description

This pull request adds support for using the contents of a file as the JSON body of a request. This is currently done by providing a bodyPath variable in the zum.toml file. If the file path exists and contains a file, the contents of the file are read and parsed as JSON. The contents of the file are then sent with the request.

If the user provides both a body and bodyPath variable like:

[endpoints.test-post]
route = "/test-post"
method = "post"
body = ["arg1", "arg2"]
bodyPath = "/some/path/to/request_body_file"

the contents of body are ignored and the file located at bodyPath is used instead.

Requirements

None.

Additional changes

Added tests for the new functionality.

- Refactor zum to accept a filepath to the contents of the body of a
  request through a command line argument.
- Extract logic to check if the file path provided is valid into a
  separate function.
Copy link
Owner

@daleal daleal left a comment

Choose a reason for hiding this comment

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

Hi! First of all, thank you again for contributing! I really appreciate it, and I have really enjoyed reviewing your PR. As a general working piece of code, I think that it works marvelously, but I will suggest several (mainly structural) changes to improve testability and room for improvement. I also think that each new method should be individually tested, to be sure about exactly what got broken on new features.

Comment on lines +19 to +40
if len(arguments) >= 1:
body_file: Path = Path(arguments.pop())
if body_file.exists() and body_file.is_file():
with body_file.open("r") as body_contents:
return load(body_contents)

else:
if not body_file.exists():
raise InvalidRequestBodyFileError(
f"Request body file located at '{body_file.absolute()}' does"
+ " not exist!"
)

if not body_file.is_file():
raise InvalidRequestBodyFileError(
f"Request body file located at '{body_file.absolute()}' is not"
+ " a file!"
)

raise InvalidRequestBodyFileError(
"Please provide the file path to the request body."
)
Copy link
Owner

Choose a reason for hiding this comment

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

I think that this should be written the other way around to remove indentations and branching. For instance, I would write something like:

    if not arguments:
        raise InvalidRequestBodyFileError(
            "Please provide the file path to the request body."
        )

    body_file = Path(arguments.pop())
    if not body_file.exists():
        raise InvalidRequestBodyFileError(
            f"Request body file located at '{body_file.absolute()}' does "
            "not exist!"
         )
    if not body_file.is_file():
        raise InvalidRequestBodyFileError(
            f"Request body file located at '{body_file.absolute()}' is not "
            "a file!"
         )

    with body_file.open("r") as raw_body_contents:
        return json.load(raw_bady_contents)

That way, you can also remove the Union[X, NoReturn] return type and just specify the return type (because clearly that method will always return a dictionary or raise an exception).

@@ -2,12 +2,44 @@
Module for the core requests logic of zum.
"""

from typing import Any, Dict, List
from json import load
Copy link
Owner

Choose a reason for hiding this comment

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

For the json library, just to keep it consistent with the rest of the codebase, I would prefer if you imported just json and then used the json.load method ❤️

Parse the JSON request body into a dictionary and return it if the
specified file exists.
"""
if len(arguments) >= 1:
Copy link
Owner

Choose a reason for hiding this comment

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

I think that if arguments: is a bit more elegant on this situation

if not body_file.exists():
raise InvalidRequestBodyFileError(
f"Request body file located at '{body_file.absolute()}' does"
+ " not exist!"
Copy link
Owner

Choose a reason for hiding this comment

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

I would loose the + operator (as the parentheses will concatenate the strings) and leave the space on the upper line, for consistency's sake

Comment on lines +55 to +63
body: Dict[str, Any] = dict()
if (
raw_endpoint.get("body") is not None
and isinstance(raw_endpoint.get("body"), str)
and raw_endpoint.get("body") == "json"
):
body = load_request_body(remaining_arguments)
else:
body, _ = reduce_arguments(raw_endpoint.get("body"), remaining_arguments)
Copy link
Owner

Choose a reason for hiding this comment

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

I think that this whole piece of code could be decoupled and implemented as a method that receives the output of calling raw_endpoint.get("body") and the remaining_arguments and returns the corresponding body. That way, this new function could operate all the conditionals and all that stuff then execute the load_request_body method directly with the first argument (if it exists)

from zum.requests.helpers import reduce_arguments
from zum.requests.models import Request


def load_request_body(arguments: List[str]) -> Union[Dict[str, Any], NoReturn]:
Copy link
Owner

Choose a reason for hiding this comment

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

I think that this method could explicitly say file, so that it is obvious that it retrieves the contents of a file. For example, it could be named load_request_body_from_file. Also, I think that it should receive only the first element of the arguments list (pre-processing that information outside this method). I don't know what should happen if arguments were to be empty, maybe you should pass None or directly raise the error from outside the method. I leave that to your judgement. Lastly, the return type should be only Dict[str, Any]. On another comment I left I explained how to stop mypy from whining about the return type by refactoring the method's code

if not body_file.is_file():
raise InvalidRequestBodyFileError(
f"Request body file located at '{body_file.absolute()}' is not"
+ " a file!"
Copy link
Owner

Choose a reason for hiding this comment

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

I would loose the + operator (as the parentheses will concatenate the strings) and leave the space on the upper line, for consistency's sake

@daleal daleal added the feature New feature or request label Mar 30, 2021
@daleal daleal linked an issue Mar 30, 2021 that may be closed by this pull request
@sehnsucht13
Copy link
Author

Thanks for the review! I will implement all the suggested changes.

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

Successfully merging this pull request may close these issues.

Add support for a json file as the request body
2 participants