Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions src/poetry/repositories/legacy_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,9 @@
from html import unescape
from pathlib import Path
from typing import TYPE_CHECKING
from typing import Any
from typing import Dict
from typing import Iterator
from typing import List
from typing import Mapping
from typing import Optional
from urllib.parse import quote

Expand Down Expand Up @@ -62,7 +61,7 @@ class Page:
".tar",
]

def __init__(self, url: str, content: str, headers: Dict[str, Any]) -> None:
def __init__(self, url: str, content: bytes, headers: Mapping) -> None:
if not url.endswith("/"):
url += "/"

Expand Down
21 changes: 3 additions & 18 deletions src/poetry/repositories/pypi_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@

import requests

from cachecontrol import CacheControl
from cachecontrol.caches.file_cache import FileCache
from cachecontrol.controller import logger as cache_control_logger
from cachy import CacheManager
Expand Down Expand Up @@ -72,18 +71,11 @@ def __init__(
)

self._cache_control_cache = FileCache(str(release_cache_dir / "_http"))
self._session = CacheControl(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not a 100% convinced this is the right solution for this. I'd much rather have e-tag used in the cache key.

Skipping cache for empty responses or invalid cache values, should be handled by gracefully handling failure - ie. clear the invalid cache and retry or fallback.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I guess that's the naive approach here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That approach is at the request level and not really at the cache level. I think an intermediate approach (e.g. layered on top of CacheControl and factoring in ETAGS) is the way to go. The caching is an important optimization, and adding a retry system at the high-level HTTP (e.g. .get()) API seems more a hack than a fix.

requests.session(), cache=self._cache_control_cache
)

self._name = "PyPI"

@property
def session(self) -> CacheControl:
return self._session

def __del__(self) -> None:
self._session.close()
def session(self) -> requests.Session:
return requests.session()

def find_packages(self, dependency: Dependency) -> List[Package]:
"""
Expand Down Expand Up @@ -323,14 +315,7 @@ def _get_release_info(self, name: str, version: str) -> dict:
return data.asdict()

def _get(self, endpoint: str) -> Union[dict, None]:
try:
json_response = self.session.get(self._base_url + endpoint)
except requests.exceptions.TooManyRedirects:
# Cache control redirect loop.
# We try to remove the cache and try again
self._cache_control_cache.delete(self._base_url + endpoint)
json_response = self.session.get(self._base_url + endpoint)

json_response = self.session.get(self._base_url + endpoint)
if json_response.status_code == 404:
return None

Expand Down
19 changes: 0 additions & 19 deletions tests/repositories/test_legacy_repository.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
from typing import Type

import pytest
import requests

from poetry.core.packages.dependency import Dependency
from poetry.factory import Factory
Expand All @@ -25,8 +24,6 @@
if TYPE_CHECKING:
import httpretty

from _pytest.monkeypatch import MonkeyPatch


class MockRepository(LegacyRepository):

Expand Down Expand Up @@ -357,19 +354,3 @@ def test_get_5xx_raises(http: Type["httpretty.httpretty"]):

with pytest.raises(RepositoryError):
repo._get_page("/foo")


def test_get_redirected_response_url(
http: Type["httpretty.httpretty"], monkeypatch: "MonkeyPatch"
):
repo = MockHttpRepository({"/foo": 200}, http)
redirect_url = "http://legacy.redirect.bar"

def get_mock(url: str) -> requests.Response:
response = requests.Response()
response.status_code = 200
response.url = redirect_url + "/foo"
return response

monkeypatch.setattr(repo.session, "get", get_mock)
assert repo._get_page("/foo")._url == "http://legacy.redirect.bar/foo/"
26 changes: 0 additions & 26 deletions tests/repositories/test_pypi_repository.py
Original file line number Diff line number Diff line change
@@ -1,25 +1,15 @@
import json
import shutil

from io import BytesIO
from pathlib import Path
from typing import TYPE_CHECKING
from typing import Dict
from typing import Optional

import pytest

from requests.exceptions import TooManyRedirects
from requests.models import Response

from poetry.core.packages.dependency import Dependency
from poetry.factory import Factory
from poetry.repositories.pypi_repository import PyPiRepository
from poetry.utils._compat import encode


if TYPE_CHECKING:
from pytest_mock import MockerFixture


class MockRepository(PyPiRepository):
Expand Down Expand Up @@ -213,22 +203,6 @@ def test_invalid_versions_ignored():
assert len(packages) == 1


def test_get_should_invalid_cache_on_too_many_redirects_error(mocker: "MockerFixture"):
delete_cache = mocker.patch("cachecontrol.caches.file_cache.FileCache.delete")

response = Response()
response.encoding = "utf-8"
response.raw = BytesIO(encode('{"foo": "bar"}'))
mocker.patch(
"cachecontrol.adapter.CacheControlAdapter.send",
side_effect=[TooManyRedirects(), response],
)
repository = PyPiRepository()
repository._get("https://pypi.org/pypi/async-timeout/json")

assert delete_cache.called


def test_urls():
repository = PyPiRepository()

Expand Down