unioslo / unioslo/mreg-cli

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

Open
#265 0 comments 0 reactions 1 assignee View on GitHub

@pederhan is already working on this.

Since Jun 12, 2024.

Dominant language
Python
Stars
2
Forks
7
Avg merge
6d 11h
Merged PRs (30d)
2

Description

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

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.