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

Some missing error handling in parse_cs #4158

Closed
embray opened this issue Sep 15, 2015 · 4 comments · Fixed by #4159
Closed

Some missing error handling in parse_cs #4158

embray opened this issue Sep 15, 2015 · 4 comments · Fixed by #4159

Comments

@embray
Copy link
Member

embray commented Sep 15, 2015

In the process of investigating #2970 I ran across what I think is an unrelated buglet.

In my testing I ran

check_conesearch_sites(url_list=None)

which seems to go on to download a whole bunch of different catalog resources and look up their conesearch test queries (I'm sort of learning how this all works as I go so please correct me @pllim :) However, in the process it crashed parse_cs because at least one resource, given by the ID ivo://CDS.VizieR/J/ApJ/780/34 contains a testQuery element with only an <sr> parameter specified, and no <ra> or <dec>. Based on my reading of this, and some of the discussion here it seems the RA, DEC, and SR parameters must be provided for a valid cone search (duh), though the Resource schema itself doesn't make that entirely clear... :(

In any case I think we should:
a) Report the broken resource and
b) Fix the code to catch this kind of error (the code is currently assuming any testQuery will at least have a valid set of parameters).

@embray
Copy link
Member Author

embray commented Sep 15, 2015

I'll add, this hasn't normally come up in testing since the test uses just a subset of sites for conesearch_master_list, and I was running the full list, I guess.

@pllim
Copy link
Member

pllim commented Sep 16, 2015

You generally do not want to validate all the services. I only used that option when I was doing a "first pass" to make a list of "good" services. There are a lot of invalid services in the master list (the last I check, there were over 10k total, good and bad together, mostly bad). That is why I have a short-listed list with the relatively "good" ones, and there are functions for users to build their own trusted list.

To account for all possible kinds of errors that the registry can have because the providers did not care to update or correct their info is going to open a can of worms. And I have tried contacting various providers at the start of this project. Most of them did not reply because they don't have resources to fix the problem, or the email got bounced back.

tl;dr - I'd rather mark this as "won't fix".

EDIT: The fix for this particular case is actually trivial. I'll submit a PR.

pllim added a commit to pllim/astropy that referenced this issue Sep 16, 2015
@pllim pllim added the vo label Sep 16, 2015
@embray
Copy link
Member Author

embray commented Sep 16, 2015

@pllim Right, I'm not saying we need to be able to support every possible invalid service; just prevent the code from crashing in this instance.

@pllim
Copy link
Member

pllim commented Sep 16, 2015

@embray , yes, I agree with you now.

pllim added a commit to pllim/astropy that referenced this issue Sep 16, 2015
@embray embray added this to the v1.0.5 milestone Sep 16, 2015
dhomeier pushed a commit to dhomeier/astropy that referenced this issue Dec 17, 2015
dhomeier pushed a commit to dhomeier/astropy that referenced this issue Jun 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants