Canonicalize case of TAP query parameter id #357
Conversation
Codecov Report
@@ Coverage Diff @@
## main #357 +/- ##
==========================================
+ Coverage 78.27% 78.35% +0.07%
==========================================
Files 46 46
Lines 5497 5497
==========================================
+ Hits 4303 4307 +4
+ Misses 1194 1190 -4
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Could you please add a one liner to the CHANGELOG.rst? Thanks! |
| @@ -175,11 +175,11 @@ def parameters(self, request, context): | |||
| if 'QUERY' in data: | |||
| assert data['QUERY'] == 'SELECT TOP 42 * FROM ivoa.obsCore' | |||
| for param in job.parameters: | |||
| if param.id_ == 'query': | |||
| if param.id_.lower() == 'query': | |||
There was a problem hiding this comment.
I'm not sure any of these should be handled in the tests itself, instead, add an example that uses different cases, so we in fact test for the fact that the code handles it in a case-insensitive way.
There was a problem hiding this comment.
The changes to the MockAsyncTAPServer have to be done to prevent it from lowercasing all incoming job parameters, thus making it impossible to test this fix. Once it no longer lowercases parameter ids, the other places where it references parameter ids also have to canonicalize. I didn't make that very clear in the PR, now fixed.
When retrieving the contents of a TAP query from an AsyncTAPJob, lowercase the identifier of the parameter before comparing it to "query". Some TAP servers reflect the case of the parameter in the job XML and DALI says that parameter keys are case-insensitive, so the parameter ID may be "QUERY" or some other case. test_submit_job was using the wrong arguments to submit_job() (passing a URL first instead of the query), uncovered by testing this change. Fix that as well. In MockAsyncTAPServer, stop lowercasing incoming parameters and instead reflect the provided case in the parameter ID, making it possible to test this fix. Fixes #356
|
If one disables the test suite's mock server's lowercasing of parameter names, the existing tests will already test this if one adds an assert with the query accessor, but I added a test that explicitly uses a mixed-case parameter name just to make the testing clearer (and added a comment about what's going on here). |
|
Oh, hm, I made an assumption about why there are separate 1.3.1 and 1.4 entries in |
Thanks. At this point I don't think there will be a 1.3.1, but we'll go straight to 1.4 but that is something we'll fix in the release process. |
When retrieving the contents of a TAP query from an AsyncTAPJob, lowercase the identifier of the parameter before comparing it to "query". Some TAP servers reflect the case of the parameter in the job XML and DALI says that parameter keys are case-insensitive, so the parameter ID may be "QUERY" or some other case.
test_submit_job was using the wrong arguments to submit_job() (passing a URL first instead of the query), uncovered by testing this change. Fix that as well.
In MockAsyncTAPServer, stop lowercasing incoming parameters and instead reflect the provided case in the parameter ID, making it possible to test this fix.
Fixes #356