unioslo / mreg-cli

Command Line Interface for Mreg
GNU General Public License v3.0
2 stars 7 forks source link

Optimization: Response.text is recomputed every time it's accessed #265

Open pederhan opened 3 months ago

pederhan commented 3 months ago

This issue is related to the rewritten API code in #214.

We access the property Response.text multiple times in get_list_generic() (directly and through validate_*_response()):

https://github.com/unioslo/mreg-cli/blob/eb94574f29d43717788f4afc80fb7a3eb03d2f36/mreg_cli/utilities/api.py#L478-L503

https://github.com/unioslo/mreg-cli/blob/eb94574f29d43717788f4afc80fb7a3eb03d2f36/mreg_cli/utilities/api.py#L405-L429

This is inefficient, because the text is recomputed each time we access the property:

https://github.com/psf/requests/blob/0e322af87745eff34caffe4df68456ebc20d9068/src/requests/models.py#L909-L945

Furthermore, when not specifying encoding, it also has to run chardet.detect() on the text to determine the encoding:

https://github.com/psf/requests/blob/0e322af87745eff34caffe4df68456ebc20d9068/src/requests/models.py#L789-L797

HTTPX does not do this, which is why this came as a surprise to me.

https://github.com/encode/httpx/blob/92e9dfb3997ab07353bbe98f9b5a6e69b7701c70/httpx/_models.py#L575-L584


Possible solution

We can make sure we only access Response.text once, and bind the result to some variable, so that we don't re-compute it each time we want to access it. This will require some changes to the validate_*_response() functions, and will make their signatures messier, as we still want to pass the Response object to the functions in order to produce good exception messages:

-def validate_list_response(response: Response) -> list[Json]:
+def validate_list_response(response: Response, text: str) -> list[Json]:
    """Parse and validate that a response contains a JSON array.

    :param response: The response to validate.
+   :param text: The response content as text.
    :raises ValidationError: If the response does not contain a valid JSON array.
    :returns: Parsed response data as a list of Python objects.
    """
    try:
-       return ListResponse.validate_json(text)
+       return ListResponse.validate_json(text)
    except ValueError as e:
        raise ValidationError(f"{response.url} did not return a valid JSON array") from e