Ensure AsyncTAPJob.result strictly follows TAP spec result ID requirement

[stvoutsin]

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.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.40%. Comparing base (3f84289) to head (42c76e0).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #644      +/-   ##
==========================================
+ Coverage   82.30%   82.40%   +0.09%     
==========================================
  Files          72       72              
  Lines        7430     7438       +8     
==========================================
+ Hits         6115     6129      +14     
+ Misses       1315     1309       -6     

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

[msdemlei]

While I'm not sure if it's a good idea to have all these odd URIs in public TAP results (could you perhaps do a DAL posting for these? Perhaps you are solving problems other people have?), I am in general in favour of removing compatibility hacks of doubtful value. I am somewhat tempted to quickly see if there are any TAP services out there that need the hack; but then so many async implementations are broken in some way that this would required quite a bit of manual perusal of error messages, so I'll let it pass. I'm sure the more relevant services should do the right thing.

So, in principle, let's go.

A minor nit: The logic in L1015ff is a bit convolved, and I'd feel better if there were test coverage for it. I think doing this would require major scaffolding, but then it's a long time since I last looked at this code. Do you see a simple way to write code that exercises that bit? Or is it a false negative from Codecov in the first place (these are fairly common, unfortunately)?

If it's too hard, let's merge anyway.

[stvoutsin]

While I'm not sure if it's a good idea to have all these odd URIs in public TAP results (could you perhaps do a DAL posting for these? Perhaps you are solving problems other people have?), I am in general in favour of removing compatibility hacks of doubtful value. I am somewhat tempted to quickly see if there are any TAP services out there that need the hack; but then so many async implementations are broken in some way that this would required quite a bit of manual perusal of error messages, so I'll let it pass. I'm sure the more relevant services should do the right thing.

So, in principle, let's go.

A minor nit: The logic in L1015ff is a bit convolved, and I'd feel better if there were test coverage for it. I think doing this would require major scaffolding, but then it's a long time since I last looked at this code. Do you see a simple way to write code that exercises that bit? Or is it a false negative from Codecov in the first place (these are fairly common, unfortunately)?

If it's too hard, let's merge anyway.

Thanks for the feedback!
Since the change make sense I'd be happy to look into adding some tests to it to improve the coverage & I can also do a quick check of the registered TAP services to see if any break with this change and report back (just have to remind myself how to script this to get the list from Registry).

Regarding the URI list, we are using CADC's TAP implementation so I can talk with them for more context, but these are used for getting execution duration for various steps, though I realise most clients will just ignore them. It's likely that we could turn these off and may choose to do so if this is an option. I can do a post on the mailing list this once I have a better idea of the options and background on these.

[msdemlei]

On Mon, Jan 27, 2025 at 11:23:53AM -0800, stvoutsin wrote: and report back (just have to remind myself how to script this to get the list from Registry).

There, I can help you out from the top of my head: import pyvo for rec in pyvo.registry.search(servicetype="tap"): print(rec.get_service("tap" ).run_sync("select top 1 * from tap_schema.tables"))

[stvoutsin]

A minor nit: The logic in L1015ff is a bit convolved, and I'd feel better if there were test coverage for it. I think doing this would require major scaffolding, but then it's a long time since I last looked at this code. Do you see a simple way to write code that exercises that bit? Or is it a false negative from Codecov in the first place (these are fairly common, unfortunately)?

I've added a few tests to hopefully address the changed bits, let me know if I've missed something
Also, I ran tests across all registered TAP services, and of the failed queries none seem to be related to this change, so I think we're ok on that front.

[msdemlei]

Looks good, and thanks for going the extra mile of checking with the existing services.