diff --git a/.github/workflows/ci_devtests.yml b/.github/workflows/ci_devtests.yml index 4f9afba2..f3ca5188 100644 --- a/.github/workflows/ci_devtests.yml +++ b/.github/workflows/ci_devtests.yml @@ -23,7 +23,7 @@ jobs: steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - name: Set up Python 3.12 - uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5.3.0 + uses: actions/setup-python@42375524e23c412d93fb67b49958b491fce71c38 # v5.4.0 with: python-version: "3.12" - name: Install tox @@ -32,7 +32,7 @@ jobs: run: tox -e py312-test-devdeps-alldeps-cov - name: Upload coverage to codecov - uses: codecov/codecov-action@1e68e06f1dbfde0e4cefc87efeba9e4643565303 # v5.1.2 + uses: codecov/codecov-action@13ce06bfc6bbe3ecf90edbbf1bc32fe5978ca1d3 # v5.3.1 with: file: ./coverage.xml verbose: true @@ -42,7 +42,7 @@ jobs: steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - name: Set up Python 3.13 - uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5.3.0 + uses: actions/setup-python@42375524e23c412d93fb67b49958b491fce71c38 # v5.4.0 with: python-version: "3.13-dev" - name: Install tox diff --git a/.github/workflows/ci_tests.yml b/.github/workflows/ci_tests.yml index 77180b52..09a4f2d8 100644 --- a/.github/workflows/ci_tests.yml +++ b/.github/workflows/ci_tests.yml @@ -40,7 +40,7 @@ jobs: with: fetch-depth: 0 - name: Set up Python - uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5.3.0 + uses: actions/setup-python@42375524e23c412d93fb67b49958b491fce71c38 # v5.4.0 with: python-version: ${{ matrix.python-version }} - name: Install tox @@ -61,7 +61,7 @@ jobs: with: fetch-depth: 0 - name: Set up Python - uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5.3.0 + uses: actions/setup-python@42375524e23c412d93fb67b49958b491fce71c38 # v5.4.0 with: python-version: '3.12' - name: Install tox @@ -77,7 +77,7 @@ jobs: steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - name: Set up Python 3.12 - uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5.3.0 + uses: actions/setup-python@42375524e23c412d93fb67b49958b491fce71c38 # v5.4.0 with: python-version: '3.12' - name: Install tox @@ -93,7 +93,7 @@ jobs: steps: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 - name: Set up Python 3.10 - uses: actions/setup-python@0b93645e9fea7318ecaed2b359559ac225c90a2b # v5.3.0 + uses: actions/setup-python@42375524e23c412d93fb67b49958b491fce71c38 # v5.4.0 with: python-version: '3.10' - name: Install tox diff --git a/CHANGES.rst b/CHANGES.rst index db02e8af..6232c8aa 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -6,6 +6,9 @@ Enhancements and Fixes - Make deletion of TAP jobs optional via a new ``delete`` kwarg. [#640] +- Change AsyncTAPJob.result to return None if no result is found explicitly [#644] + + Deprecations and Removals ------------------------- diff --git a/docs/registry/index.rst b/docs/registry/index.rst index 8e50a7f0..65c16fe7 100644 --- a/docs/registry/index.rst +++ b/docs/registry/index.rst @@ -240,7 +240,7 @@ constraint on the description ``get_service(service_type='conesearch', keyword=' ... print(interface) Interface(type='tap#aux', description='', url='http://tapvizier.cds.unistra.fr/TAPVizieR/tap') Interface(type='vr:webbrowser', description='', url='http://vizier.cds.unistra.fr/viz-bin/VizieR-2?-source=J/ApJ/727/14') - Interface(type='conesearch', description='Cone search capability for table J/ApJ/727/14/table2 (AKARI IRC 3-24{mu}m, and Spitzer MIPS 24/70{mu}m photometry of Abell 2255 member galaxies)', url='http://vizier.cds.unistra.fr/viz-bin/conesearch/J/ApJ/727/14/table2?') + Interface(type='conesearch', description='Cone search capability for table J/ApJ/727/14/table2 (AKARI IRC 3-24{mu}m, and Spitzer MIPS 24/70{mu}m photometry of Abell 2255 member galaxies)', url='https://vizier.cds.unistra.fr/viz-bin/conesearch/J/ApJ/727/14/table2?') Or construct the service object directly from the list of interfaces with: diff --git a/pyvo/dal/query.py b/pyvo/dal/query.py index b56e9caf..e135ffcb 100644 --- a/pyvo/dal/query.py +++ b/pyvo/dal/query.py @@ -20,7 +20,8 @@ standard data model. Usually the field names are used to uniquely identify table columns. """ -__all__ = ["DALService", "DALQuery", "DALResults", "Record"] +__all__ = ["DALService", "DALServiceError", "DALQuery", "DALQueryError", + "DALResults", "Record"] import os import shutil @@ -64,7 +65,7 @@ def __init__(self, baseurl, *, session=None, capability_description=None,): the base URL that should be used for forming queries to the service. session : object optional session to use for network requests - description : str, optional + capability_description : str, optional the description of the service. """ self._baseurl = baseurl diff --git a/pyvo/dal/tap.py b/pyvo/dal/tap.py index 5e4a2d38..365002a6 100644 --- a/pyvo/dal/tap.py +++ b/pyvo/dal/tap.py @@ -118,14 +118,18 @@ def __init__(self, baseurl, *, capability_description=None, session=None): session : object optional session to use for network requests """ - super().__init__(baseurl, session=session, capability_description=capability_description) - - # Check if the session has an update_from_capabilities attribute. - # This means that the session is aware of IVOA capabilities, - # and can use this information in processing network requests. - # One such use case for this is auth. - if hasattr(self._session, 'update_from_capabilities'): - self._session.update_from_capabilities(self.capabilities) + try: + super().__init__(baseurl, session=session, capability_description=capability_description) + + # Check if the session has an update_from_capabilities attribute. + # This means that the session is aware of IVOA capabilities, + # and can use this information in processing network requests. + # One such use case for this is auth. + if hasattr(self._session, 'update_from_capabilities'): + self._session.update_from_capabilities(self.capabilities) + except DALServiceError as e: + raise DALServiceError(f"Cannot find TAP service at '" + f"{baseurl}'.\n\n{str(e)}") from None def get_tap_capability(self): """ @@ -373,8 +377,6 @@ def create_query( Parameters ---------- - baseurl : str - the base URL for the TAP service query : str the query string / parameters mode : str @@ -861,14 +863,14 @@ def results(self): @property def result(self): """ - The job result if exists + Returns the UWS result with id='result' if it exists, otherwise None. """ try: for r in self._job.results: if r.id_ == 'result': return r - return self._job.results[0] + return None except IndexError: return None @@ -885,7 +887,10 @@ def result_uri(self): the uri of the result """ try: - uri = self.result.href + result = self.result + if result is None: + return None + uri = result.href if not urlparse(uri).netloc: uri = urljoin(self.url, uri) return uri @@ -940,6 +945,8 @@ def wait(self, *, phases=None, timeout=600.): ---------- phases : list phases to wait for + timeout : float + maximum time to wait in seconds Raises ------ @@ -1007,6 +1014,12 @@ def fetch_result(self): """ returns the result votable if query is finished """ + result_uri = self.result_uri + if result_uri is None: + self._update() + self.raise_if_error() + raise DALServiceError("No result URI available", self.url) + try: response = self._session.get(self.result_uri, stream=True) response.raise_for_status() diff --git a/pyvo/dal/tests/test_tap.py b/pyvo/dal/tests/test_tap.py index 2e69b7bb..ab4d4307 100644 --- a/pyvo/dal/tests/test_tap.py +++ b/pyvo/dal/tests/test_tap.py @@ -11,6 +11,7 @@ import tempfile import pytest +import requests import requests_mock from pyvo.dal.tap import escape, search, AsyncTAPJob, TAPService @@ -355,6 +356,12 @@ def async_fixture(mocker): yield from mock_server.use(mocker) +@pytest.fixture() +def async_fixture_with_timeout(mocker): + mock_server = MockAsyncTAPServer() + yield from mock_server.use(mocker) + + @pytest.fixture() def tables(mocker): def callback_tables(request, context): @@ -737,6 +744,103 @@ def match_request_text(request): finally: prototype.deactivate_features('cadc-tb-upload') + @pytest.mark.usefixtures('async_fixture') + def test_job_no_result(self): + service = TAPService('http://example.com/tap') + job = service.submit_job("SELECT * FROM ivoa.obscore") + with pytest.raises(DALServiceError) as excinfo: + job.fetch_result() + + assert "No result URI available" in str(excinfo.value) + job.delete() + + @pytest.mark.usefixtures('async_fixture') + def test_fetch_result_network_error(self): + service = TAPService('http://example.com/tap') + job = service.submit_job("SELECT * FROM ivoa.obscore") + job.run() + job.wait() + status_response = ''' + + 1 + COMPLETED + + + + ''' + + with requests_mock.Mocker() as rm: + rm.get(f'http://example.com/tap/async/{job.job_id}', + text=status_response) + rm.get( + f'http://example.com/tap/async/{job.job_id}/results/result', + exc=requests.exceptions.ConnectTimeout + ) + + with pytest.raises(DALServiceError) as excinfo: + job.fetch_result() + + assert "Unknown service error" in str(excinfo.value) + + job.delete() + + @pytest.mark.usefixtures('async_fixture') + def test_job_no_result_uri(self): + status_response = ''' + + 1 + COMPLETED + + + + ''' + + service = TAPService('http://example.com/tap') + job = service.submit_job("SELECT * FROM ivoa.obscore") + job.run() + job.wait() + + with requests_mock.Mocker() as rm: + rm.get(f'http://example.com/tap/async/{job.job_id}', + text=status_response) + job._update() + with pytest.raises(DALServiceError) as excinfo: + job.fetch_result() + + assert "No result URI available" in str(excinfo.value) + + job.delete() + + @pytest.mark.usefixtures('async_fixture') + def test_job_with_empty_error(self): + error_response = ''' + + 1 + ERROR + + + + + ''' + + service = TAPService('http://example.com/tap') + job = service.submit_job("SELECT * FROM ivoa.obscore") + job.run() + job.wait() + + with requests_mock.Mocker() as rm: + rm.get(f'http://example.com/tap/async/{job.job_id}', + text=error_response) + job._update() + with pytest.raises(DALQueryError) as excinfo: + job.fetch_result() + + assert "" in str(excinfo.value) + @pytest.mark.usefixtures("tapservice") class TestTAPCapabilities: @@ -804,6 +908,197 @@ def test_get_endpoint_candidates(): assert svc._get_endpoint_candidates("capabilities") == expected_urls +def test_timeout_error(): + service = TAPService('http://example.com/tap') + + with requests_mock.Mocker() as rm: + rm.register_uri( + 'GET', + 'http://example.com/tap/capabilities', + exc=requests.Timeout("Request timed out") + ) + rm.register_uri( + 'GET', + 'http://example.com/capabilities', + exc=requests.Timeout("Request timed out") + ) + + with pytest.raises(DALServiceError) as excinfo: + _ = service.capabilities + + error_message = str(excinfo.value) + assert "Request timed out" in error_message + + +def test_generic_request_exception(): + service = TAPService('http://example.com/tap') + + with requests_mock.Mocker() as rm: + rm.register_uri( + 'GET', + 'http://example.com/tap/capabilities', + exc=requests.RequestException("Some request error") + ) + rm.register_uri( + 'GET', + 'http://example.com/capabilities', + exc=requests.RequestException("Some request error") + ) + + with pytest.raises(DALServiceError) as excinfo: + _ = service.capabilities + + error_message = str(excinfo.value) + assert "Some request error" in error_message + + +def test_unexpected_exception(): + service = TAPService('http://example.com/tap') + + class CustomException(Exception): + pass + + with requests_mock.Mocker() as rm: + rm.register_uri( + 'GET', + 'http://example.com/tap/capabilities', + exc=CustomException("Unexpected error occurred") + ) + rm.register_uri( + 'GET', + 'http://example.com/capabilities', + exc=CustomException("Unexpected error occurred") + ) + + with pytest.raises(DALServiceError) as excinfo: + _ = service.capabilities + + error_message = str(excinfo.value) + assert "Unable to access the capabilities endpoint at:" in error_message + + +def test_tap_service_initialization_error(): + with requests_mock.Mocker() as rm: + rm.register_uri( + 'GET', + 'http://example.com/tap/capabilities', + status_code=404 + ) + rm.register_uri( + 'GET', + 'http://example.com/capabilities', + status_code=404 + ) + + service = TAPService('http://example.com/tap') + with pytest.raises(DALServiceError) as excinfo: + _ = service.capabilities + + error_message = str(excinfo.value) + assert "Unable to access the capabilities endpoint at:" in error_message + assert "404" in error_message + + +def test_endpoint_connection_errors(): + service = TAPService('http://example.com/tap') + + with requests_mock.Mocker() as rm: + rm.register_uri( + 'GET', + 'http://example.com/tap/capabilities', + status_code=404 + ) + rm.register_uri( + 'GET', + 'http://example.com/capabilities', + status_code=404 + ) + + with pytest.raises(DALServiceError) as excinfo: + _ = service.capabilities + + error_message = str(excinfo.value) + assert "Unable to access the capabilities endpoint at:" in error_message + assert "404" in error_message + assert "The service URL is incorrect" in error_message + + +def test_invalid_tap_url(): + service = TAPService('not-a-url') + + with requests_mock.Mocker() as rm: + rm.register_uri( + 'GET', + requests_mock.ANY, + exc=requests.exceptions.ConnectionError( + "Failed to establish connection") + ) + + with pytest.raises(DALServiceError) as excinfo: + _ = service.capabilities + + error_message = str(excinfo.value) + assert "Unable to access the capabilities endpoint at:" in error_message + assert "Connection failed" in error_message + + +def test_http_error_responses(): + error_codes = { + 403: "Forbidden", + 500: "Internal Server Error", + 502: "Bad Gateway", + 503: "Service Unavailable" + } + + for code, reason in error_codes.items(): + with requests_mock.Mocker() as rm: + rm.register_uri( + 'GET', + 'http://example.com/tap/capabilities', + status_code=code, + reason=reason + ) + rm.register_uri( + 'GET', + 'http://example.com/capabilities', + status_code=code, + reason=reason + ) + + service = TAPService('http://example.com/tap') + with pytest.raises(DALServiceError) as excinfo: + _ = service.capabilities + + error_message = str(excinfo.value) + assert "Unable to access the capabilities endpoint at:" in error_message + assert f"{code} {reason}" in error_message + + +def test_network_error(): + service = TAPService('http://example.com/tap') + + with requests_mock.Mocker() as rm: + rm.register_uri( + 'GET', + 'http://example.com/tap/capabilities', + exc=requests.exceptions.ConnectionError( + "Failed to establish connection") + ) + rm.register_uri( + 'GET', + 'http://example.com/capabilities', + exc=requests.exceptions.ConnectionError( + "Failed to establish connection") + ) + + with pytest.raises(DALServiceError) as excinfo: + _ = service.capabilities + + error_message = str(excinfo.value) + assert "Unable to access the capabilities endpoint at:" in error_message + assert "Connection failed" in error_message + + @pytest.mark.remote_data @pytest.mark.parametrize('stream_type', [BytesIO, StringIO]) def test_tap_upload_remote(stream_type): diff --git a/pyvo/dal/vosi.py b/pyvo/dal/vosi.py index c5367f0c..2b6f34fe 100644 --- a/pyvo/dal/vosi.py +++ b/pyvo/dal/vosi.py @@ -19,6 +19,7 @@ class EndpointMixin(): def _get_endpoint_candidates(self, endpoint): + """Construct endpoint URLs from base URL and endpoint""" urlcomp = urlparse(self.baseurl) # Include the port number if present netloc = urlcomp.hostname @@ -31,18 +32,92 @@ def _get_endpoint_candidates(self, endpoint): return [f'{curated_baseurl}/{endpoint}', url_sibling(curated_baseurl, endpoint)] + @staticmethod + def _build_error_message(endpoint, attempted_urls): + """Build error message from attempted URLs and their failures""" + return ( + f"Unable to access the {endpoint} endpoint at:\n" + f"{chr(10).join(attempted_urls)}\n\n" + f"This could mean:\n" + f"1. The service URL is incorrect\n" + f"2. The service is temporarily unavailable\n" + f"3. The service doesn't support this protocol\n" + f"4. If a 503 was encountered, retry after the suggested delay.\n" + ) + + @staticmethod + def _get_response_body(response): + """Extract response body for error messagess""" + + max_length = 500 + # Truncate if too long (500 chars), to avoid overwhelming the user + if response is None or not response.headers.get("Content-Type", + "").startswith("text"): + return "" + + return f" Response body: {response.text[:max_length]}" + + def _handle_http_error(self, error, url, attempted_urls): + """Handle HTTP errors and update attempted_urls list""" + + response_body = self._get_response_body(error.response) + + if error.response.status_code == 503: + # Handle Retry-After header, if present + retry_after = None + for header in error.response.headers: + if header.lower() == 'retry-after': + retry_after = error.response.headers[header] + break + + if retry_after: + attempted_urls.append( + f"- {url}: 503 Service Unavailable (Retry-After: " + f"{retry_after}){response_body}") + return True + elif error.response.status_code == 404: + attempted_urls.append(f"- {url}: 404 Not Found") + return True + + status_code = error.response.status_code + attempted_urls.append( + f"- {url}: HTTP Code: {status_code} " + f"{error.response.reason}{response_body}") + return False + def _get_endpoint(self, endpoint): - for ep_url in self._get_endpoint_candidates(endpoint): + """Attempt to connect to service endpoint""" + attempted_urls = [] + try: + candidates = self._get_endpoint_candidates(endpoint) + except (ValueError, AttributeError) as e: + raise DALServiceError( + f"Cannot construct endpoint URL from base '{self.baseurl}' and endpoint '{endpoint}'. " + f"{type(e).__name__}: {str(e)}" + ) from e + + for ep_url in candidates: try: response = self._session.get(ep_url, stream=True) response.raise_for_status() + return response.raw + except requests.HTTPError as e: + if not self._handle_http_error(e, ep_url, attempted_urls): + break + except requests.ConnectionError: + attempted_urls.append( + f"- {ep_url}: Connection failed " + f"(Possible causes: incorrect URL, DNS issue, or service is down)") + break + except requests.Timeout: + attempted_urls.append( + f"- {ep_url}: Request timed out (Service may be overloaded or slow)") + except (requests.RequestException, Exception) as e: + attempted_urls.append(f"- {ep_url}: {str(e)}") break - except requests.RequestException: - continue - else: - raise DALServiceError(f"No working {endpoint} endpoint provided") - return response.raw + raise DALServiceError( + self._build_error_message(endpoint, attempted_urls)) @deprecated(since="1.5") @@ -88,8 +163,7 @@ class CapabilityMixin(EndpointMixin): @stream_decode_content def _capabilities(self): """ - Returns capabilities as a - py:class:`~pyvo.io.vosi.availability.Availability` object + Retrieve the raw capabilities document from the service. """ return self._get_endpoint('capabilities') diff --git a/pyvo/io/vosi/endpoint.py b/pyvo/io/vosi/endpoint.py index 00095b0a..4340cdf2 100644 --- a/pyvo/io/vosi/endpoint.py +++ b/pyvo/io/vosi/endpoint.py @@ -67,6 +67,9 @@ def parse_tables(source, *, pedantic=None, filename=None, then *source* will be used as a filename for error messages. Therefore, *filename* is only required when source is a file-like object. + _debug_python_based_parser : bool, optional + If `True`, use the Python-based parser. This is useful for + debugging purposes. Defaults to False. Returns ------- @@ -113,6 +116,9 @@ def parse_capabilities(source, *, pedantic=None, filename=None, then *source* will be used as a filename for error messages. Therefore, *filename* is only required when source is a file-like object. + _debug_python_based_parser : bool, optional + If `True`, use the Python-based parser. This is useful for + debugging purposes. Defaults to False. Returns ------- @@ -159,6 +165,11 @@ def parse_availability(source, *, pedantic=None, filename=None, then *source* will be used as a filename for error messages. Therefore, *filename* is only required when source is a file-like object. + _debug_python_based_parser : bool, optional + If `True`, use the Python-based parser. This is useful for + debugging purposes. Defaults to False. + + Returns -------