DM-37510: Make HttpResourcePath.exists() more robust for WebDAV endpoints #38
Conversation
Codecov ReportBase: 85.05% // Head: 84.74% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #38 +/- ##
==========================================
- Coverage 85.05% 84.74% -0.31%
==========================================
Files 27 27
Lines 3218 3415 +197
Branches 671 713 +42
==========================================
+ Hits 2737 2894 +157
- Misses 380 408 +28
- Partials 101 113 +12
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
python/lsst/resources/http.py
Outdated
| # single remote resource, the response must include properties for | ||
| # that single resource. So either its response status is OK or not. | ||
| tree = etree.fromstring(resp.content.strip()) | ||
| element = tree.find("./{DAV:}response/{DAV:}propstat/[{DAV:}status='HTTP/1.1 200 OK']") |
There was a problem hiding this comment.
A question for my edification: can this ever be a different HTTP version (e.g. 1.0 or 2.0)?
There was a problem hiding this comment.
My understanding of the RFC 4918is that the response to PROPFIND requires that status be of the form HTTP/1.1 <status code> <message>.
But I would say this does not preclude that the communication protocol used between client and server be HTTP2 or HTTP3.
python/lsst/resources/http.py
Outdated
| log.debug("Response:") | ||
| log.debug(f" status_code={resp.status_code}") | ||
| log.debug(f" headers={resp.headers}") | ||
| if resp.content is None: |
There was a problem hiding this comment.
mypy is convinced that resp.content can never be None. I don't know if that's true but maybe changing this line to if not resp.content would work.
There was a problem hiding this comment.
I think mypy is wrong, indeed. I changed the test to make it happy.
There was a problem hiding this comment.
The problem with requests type annotations is that they come in through the types-requests package which is a standalone external package not directly part of the requests code base. It's possible there is a bug in that annotation.
There was a problem hiding this comment.
My issue is that the results of running mypy locally in my development host are not the same as the ones ran in github actions. That makes I need several roundtrips to make mypy happy. I need to take the time to understand in detail how to make it work locally.
There was a problem hiding this comment.
are you using the mypy from rubin-env-developer conda package? I seem to have 0.991 installed and that is the version the action seems to be using.
|
@airnandez I just merged #39 which updates dependencies and runs black 23.1. You will need to rebase and I'm hoping the rebase is not problematic given you had to run black yourself. |
… and plain HTTP endpoints
timj
force-pushed
the
tickets/DM-37510
branch
from
a991948
to
6fc34ea
Compare
|
@airnandez I rebased for you -- it was not completely trivial so it seemed unfair to make you do it. I have checked that the test_http.py file did not change after rebasing. |
Thanks @timj for rebasing. I pulled the rebased branch and successfully ran the test suite. From my perspective the work I intended for this ticket is done. |
timj
force-pushed
the
tickets/DM-37510
branch
from
24fc48e
to
6fc34ea
Compare
…earer tokens are not sent for non-secure HTTP endpoints
…s are located in plain HTTP servers
|
This was far much work than I initially anticipated. But I am happy with the result. I think this code is more robust and overall better now. Thanks for your time and for this very detailed feedback. |
Checklist
doc/changesI recognise this PR involves several changes not initially intended. The changes implemented for using webDAV-specific methods for
exist()andsize()had more consequences than initially anticipated. This was an opportunity (and a necessity) for improving the test suite.