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

DM-37917: Add testing against real webDAV server for HttpResourcePath #40

Merged
merged 22 commits into from
Feb 15, 2023
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
a9b6f21
Improve handling of DELETE requests
airnandez Feb 7, 2023
0469e84
Test deletion of a directory raises
airnandez Feb 7, 2023
a5a5f12
Add method to explicitly close sessions to avoid warnings when unit t…
airnandez Feb 7, 2023
e0e5ac1
Improve comments for session retries
airnandez Feb 8, 2023
9ac5a1d
Comply with conventions for documenting methods
airnandez Feb 8, 2023
632155b
Add test case using a local webDAV server
airnandez Feb 8, 2023
5bbe8c0
Minor improvements and corrections of typos
airnandez Feb 8, 2023
108c893
Add missing return type annotation
airnandez Feb 8, 2023
f7fe03f
Raise NotADirectoryError rather than ValueError when not a directory
timj Feb 9, 2023
3ce43db
Fix the confusing test case class name for S3 tests
timj Feb 9, 2023
b79d643
Enable standard testing of HTTP WebDav interface
timj Feb 9, 2023
d52dd73
The root URI to test can include a non-empty path
airnandez Feb 10, 2023
554afff
Ensure the parent directory exists before actually uploading data
airnandez Feb 10, 2023
d56425f
Restructure tests in several ways:
airnandez Feb 10, 2023
459bccf
Give priority to test against real servers in development setting if …
airnandez Feb 13, 2023
260b147
Ensure all webDAV requests download the response body immediately
airnandez Feb 13, 2023
a7a3d9d
Parse status element of PROPFIND response for making easier inspectio…
airnandez Feb 13, 2023
3d556fc
Simplify path building
airnandez Feb 13, 2023
67e387f
Ensure absolute path starts by single slash
airnandez Feb 13, 2023
db184ba
Internal method _close_sessions() no longer needed
airnandez Feb 14, 2023
6b8a6bb
Restructure tests and remove testing with mocked responses.
airnandez Feb 14, 2023
4087c3e
Keep naming of test cases consistent
airnandez Feb 14, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
60 changes: 51 additions & 9 deletions python/lsst/resources/http.py
Original file line number Diff line number Diff line change
Expand Up @@ -251,12 +251,26 @@ def _make_session(self, rpath: ResourcePath, persist: bool) -> requests.Session:
log.debug("Creating new HTTP session for endpoint %s (persist connection=%s)...", root_uri, persist)

retries = Retry(
# Total number of retries to allow. Takes precedence over other
# counts.
total=3,
# How many connection-related errors to retry on.
connect=3,
# How many times to retry on read errors.
read=3,
# Backoff factor to apply between attempts after the second try
# (seconds)
backoff_factor=5.0 + random.random(),
# How many times to retry on bad status codes
status=3,
status_forcelist=[429, 500, 502, 503, 504],
# HTTP status codes that we should force a retry on
status_forcelist=[
requests.codes.too_many_requests, # 429
requests.codes.internal_server_error, # 500
requests.codes.bad_gateway, # 502
requests.codes.service_unavailable, # 503
requests.codes.gateway_timeout, # 504
],
)

# Persist a single connection to the front end server, if required
Expand Down Expand Up @@ -464,10 +478,12 @@ def mkdir(self) -> None:
"""Create the directory resource if it does not already exist."""
# Creating directories is only available on WebDAV backends.
if not self.is_webdav_endpoint:
raise NotImplementedError("Endpoint does not implement WebDAV functionality")
raise NotImplementedError(
f"Creation of directory {self} is not implemented by plain HTTP servers"
)

if not self.dirLike:
raise ValueError(f"Can not create a 'directory' for file-like URI {self}")
raise NotADirectoryError(f"Can not create a 'directory' for file-like URI {self}")

if not self.exists():
# We need to test the absence of the parent directory,
Expand Down Expand Up @@ -522,6 +538,10 @@ def write(self, data: bytes, overwrite: bool = True) -> None:
if self.exists():
raise FileExistsError(f"Remote resource {self} exists and overwrite has been disabled")

# Ensure the parent directory exists
self.parent().mkdir()

# Upload the data
with time_this(log, msg="Write to remote %s (%d bytes)", args=(self, len(data))):
self._put(data=data)

Expand Down Expand Up @@ -718,17 +738,27 @@ def _mkcol(self) -> None:

def _delete(self) -> None:
"""Send a DELETE webDAV request for this resource."""
# TODO: should we first check that the resource is not a directory?
# TODO: we should remove Depth header which should not be used for
# directories and systematically check if the return code is
# multistatus, in which case we need to look for errors in the
# response.
resp = self._send_webdav_request("DELETE", headers={"Depth": "0"})

log.debug("Deleting %s ...", self.geturl())

# If this is a directory, ensure the remote is a webDAV server because
# plain HTTP servers don't support DELETE requests on non-file
# paths.
if self.dirLike and not self.is_webdav_endpoint:
raise NotImplementedError(
f"Deletion of directory {self} is not implemented by plain HTTP servers"
)

resp = self._send_webdav_request("DELETE")
if resp.status_code in (requests.codes.ok, requests.codes.accepted, requests.codes.no_content):
return
elif resp.status_code == requests.codes.not_found:
raise FileNotFoundError(f"Resource {self} does not exist, status code: {resp.status_code}")
else:
# TODO: the response to a DELETE request against a webDAV server
# may be multistatus. If so, we need to parse the reponse body to
# determine more precisely the reason of the failure (e.g. a lock)
# and provide a more helpful error message.
raise ValueError(f"Unable to delete resource {self}; status code: {resp.status_code}")

def _copy_via_local(self, src: ResourcePath) -> None:
Expand Down Expand Up @@ -838,6 +868,18 @@ def _put(self, data: Union[BinaryIO, bytes]) -> None:
if resp.status_code not in (requests.codes.ok, requests.codes.created, requests.codes.no_content):
raise ValueError(f"Can not write file {self}, status code: {resp.status_code}")

def _close_sessions(self) -> None:
"""Close sockets used underlying sessions.

Notes
-----
This method is intended exclusively to avoid warning messages when unit
testing this class. Don't call it explicitly: sessions are
automatically closed in normal execution conditions.
"""
self.session.close()
self.put_session.close()

@contextlib.contextmanager
def _openImpl(
self,
Expand Down
13 changes: 12 additions & 1 deletion python/lsst/resources/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,8 @@
import logging
import os
import pathlib
import random
import string
import unittest
import urllib.parse
import uuid
Expand Down Expand Up @@ -119,6 +121,7 @@ class _GenericTestCase:

scheme: Optional[str] = None
netloc: Optional[str] = None
base_path: Optional[str] = None
path1 = "test_dir"
path2 = "file.txt"

Expand All @@ -142,8 +145,13 @@ def _make_uri(self, path: str, netloc: Optional[str] = None) -> str:
if self.scheme is not None:
if netloc is None:
netloc = self.netloc

path = path.lstrip("/")
if self.base_path is not None:
path = self.base_path.strip("/") + "/" + path
Copy link
Member

Choose a reason for hiding this comment

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

Probably clearer to do:

path = os.path.join(self.base_path, path)

since that will take care of the trailing slash automatically.

if path.startswith("/"):
Copy link
Member

Choose a reason for hiding this comment

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

The path.lstrip at the top makes this line a no-op I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See comment above.

path = path[1:]

return f"{self.scheme}://{netloc}/{path}"
else:
return path
Expand Down Expand Up @@ -428,7 +436,10 @@ def setUp(self) -> None:
self.tmpdir = ResourcePath(makeTestTempDir(self.testdir))
else:
# Create tmp directory relative to the test root.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe change to "Create random tmp directory"

self.tmpdir = self.root_uri.join("TESTING/")
self.tmpdir = self.root_uri.join(
"TESTING-" + "".join(random.choices(string.ascii_lowercase + string.digits, k=8)),
forceDirectory=True,
)
self.tmpdir.mkdir()

def tearDown(self) -> None:
Expand Down