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

Ensure AsyncTAPJob.result strictly follows TAP spec result ID requirement #644

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

stvoutsin
Copy link
Contributor

@stvoutsin stvoutsin commented Jan 24, 2025

Description of Issue

In the process of using pyvo with our TAP service at the Rubin Observatory for inspecting job endpoints, I've noticed an issue when using AsyncTAPJob to fetch results for a job that does not have a result entry with id="result".

Sample Snippet:

job = pyvo.dal.AsyncTAPJob(query_url, session=s)
job.fetch_result().to_table().to_pandas()

Sample Job:

<uws:job xmlns:uws="http://www.ivoa.net/xml/UWS/v1.0" 
         xmlns:xlink="http://www.w3.org/1999/xlink" 
         version="1.1">
    <uws:jobId>ypwb7abcetg8vd</uws:jobId>
    <uws:phase>COMPLETED</uws:phase>
    ....
    <uws:parameters>
        <uws:parameter id="DELETE">True</uws:parameter>
        <uws:parameter id="LANG">ADQL</uws:parameter>
        <uws:parameter id="QUERY">SELECT TOP 10 * FROM Object</uws:parameter>
        <uws:parameter id="REQUEST">doQuery</uws:parameter>
    </uws:parameters>
    <uws:results>
        <uws:result id="diag" xlink:href="uws:executing:10"/>
        <uws:result id="diag" xlink:href="jndi:lookup:0"/>
        <uws:result id="diag" xlink:href="read:tap_schema:9"/>
        <uws:result id="diag" xlink:href="query:parse:562"/>
        <uws:result id="diag" xlink:href="jndi:connect:127"/>
        <uws:result id="diag" xlink:href="query:execute:4941"/>
        <uws:result id="diag" xlink:href="query:store:657"/>
        <uws:result id="rowcount" xlink:href="final:10"/>
    </uws:results>
</uws:job>

Currently this will produce the following output for our case, where we include informative/debugging entries in our results:

DALServiceError: No connection adapters were found for 'uws:executing:10'

This is because it tries to find a result with id="results", and if none are found it will default to the first result entry.

Summary of fix

This PR updates the AsyncTAPJob.result method to strictly follow the TAP specification requirement that ADQL query results must have id="result".

Per TAP spec section 2.2:

For query languages that produce a single result executed using the /async endpoint, the result of a successful query can be found within the result list specified by UWS; the result must be named result and thus clients can access it directly..

Changes:

  • Removes fallback logic that would return the first non-diagnostic result
  • Only returns results with id="result", otherwise returns None

Potential Impact:

  • May break compatibility with non-compliant TAP services that don't use id="result"

Testing:

  • Verified behavior with 5 (compliant) TAP services

Alternative options

If you think this change is likely to have an impact on non-compliant services that pyvo should continue to support, perhaps there could be an alternative fix I could add to check the existence of the HTTP protocol in the string we get from result_uri.

Feedback welcome on balancing strict compliance vs backwards compatibility.

I can add some tests if this looks like a valid fix for the issue described.

Copy link

codecov bot commented Jan 24, 2025

Codecov Report

Attention: Patch coverage is 50.00000% with 5 lines in your changes missing coverage. Please review.

Project coverage is 82.26%. Comparing base (3f84289) to head (7f2e114).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pyvo/dal/tap.py 50.00% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #644      +/-   ##
==========================================
- Coverage   82.30%   82.26%   -0.04%     
==========================================
  Files          72       72              
  Lines        7430     7438       +8     
==========================================
+ Hits         6115     6119       +4     
- Misses       1315     1319       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bsipocz bsipocz added this to the v1.7 milestone Jan 24, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants