-
Notifications
You must be signed in to change notification settings - Fork 5
adding RESPONSEFORMAT, DALI error responses & OVERFLOW usage #64
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the text after this PR as it stands is very clear; the item 2.2 starts off by saying the service must respond with a VOTable, then says it can be some other format, then talks about the detailed format that the VOTable MUST have. The next item (2.3) then says that in case of error a service must respond with a VOTable error document, which is probably undesirable for a client requesting e.g. RESPONSEFORMAT=text/csv
.
I think it would be clearer to say in 2.2:
- a successful response must be a table containing ID, RA, Dec with certain conditions on the content
- table format is VOTable by default, but if RESPONSEFORMAT is set it has to match that
- if it's a VOTable, then it must obey particular rules (UCDs, types, RESOURCE structure etc).
and in 2.3:
- by default, errors are reported using a VOTable stub with a PARAM/INFO having name="Error"
- if a non-VOTable RESPONSEFORMAT is requested, the response SHOULD be a plain text error document instead
However, I'm not sure what the scope of this revision is supposed to be. If the idea is to make the minimum possible changes to the text while introducing more DALI-compatible options, maybe the PR as it stands would be OK.
The PR also includes hard-coded section/item numbers, though that also follows practice in the existing text (should we open a separate issue to address this?)
Fair points. As for the hardcoded sections, I'll remove them as soon as I realise how to have \ref+\label work in that case. BTW: this won't end up as is in a proper WD before issues #62 and #18, at least, are covered, but possible also others. That is, there's time to fix text. |
Thanks Marco. I seem to recall that LaTeX enumeration items can get cross-referenced by using |
I'm hesitating at moving those bullets into sections, that's what I tried for the "old" v.1.1 and it was deemed to be difficult when comparing 1.03 to 1.1. I can retry, now that there won't be the deprecation sections (that was really confusing). Maybe also limiting to subsections, rather than fully exploding into subsubsections for the parameters and fields. Up for discussion. |
I feel like just going to 2.* subsections for that one enumeration in Section 2 would help, since the text already refers to those parts by sec.subsec number, without need to go further. But if you've tried it and it doesn't work out, I'm happy to defer to your judgement. |
Following @mbtaylor comments and suggestions, I promoted numbered listings in Sec.2 to sub-sections and re-ordered the text there to make it more clear. I should also have removed the hard-coded references. Additionally, I ported here the re-writing of the error responses, following DALI rec. This means that, when this is PR is clean and merged, it will close issues #61, #45, and #54 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@molinaro-m thanks for the changes. There are still a few items I think should be addressed as per my comments.
ConeSearch.tex
Outdated
@@ -167,11 +170,17 @@ \section{Service Interface Requirements} | |||
\item[Example] | |||
\url{http://mycone.org/cgi-bin/search?RA=180.567&DEC=-30.45&SR=0.0125} | |||
\end{bigdescription} | |||
\item As defined by DALI a service SHOULD also understand the following parameter: | |||
\item As defined by DALI a service SHOULD also understand the following parameters: | |||
\begin{description} | |||
\item[\textbf{MAXREC}] to let the client limit the number of records returned | |||
or require a service metadata response. Its usage is encouraged and preferred | |||
to the SR=0 solution for metadata discovery. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably say "its usage with a value of 0" rather than "its usage".
Note however it's a bit problematic preferring MAXREC=0 to SR=0 for metadata probing, since services that don't support the optional MAXREC parameter (including all pre-v1.1 services?) will ignore it and potentially give you a lot of data. Preferred usage for metadata probing would therefore be MAXREC=0&SR=0&
. But maybe that's either obvious or too much detail to put in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I should have fixed this, adding a footnote for the additional consideration on MAXREC being optional
ConeSearch.tex
Outdated
formats other than the mandatory VOTable one. Indeed, a Simple | ||
Cone Search service MUST provide responses in VOTable format \citep{2025ivoa.spec.0116O}, | ||
compliant with respect to what will | ||
be detailed in the next subsection (Sec. 2.2), but should also |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded xref "Sec 2.2" should be "Sec \ref{subsec:response}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for spotting this! fixed.
\item one numerical column representing the main value for the | ||
right-ascension of the source in the ICRS coordinate system; | ||
\item one numerical column representing the main value for the | ||
declination of the source in the ICRS coordinate system. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Specify decimal degrees for RA & Dec output columns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I have added a sentence but this wasn't in the 1.03 version... I'll add it as a discussion point for the dedicated running meeting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see what you mean, this could in principle invalidate an existing CS 1.03-compliant service that returns RA and Dec in radians, so is maybe questionable. But I doubt that there are any such services, and being able to assume degrees here simplifies rigorous client processing a lot, so I'd say it's worth that small risk. As you say: let's discuss.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From discussion at DAL Running Meeting 21 the requirement to have RA and Dec in decimal degrees stays as is. No major issues foreseen from a quick investigation of the existing services behaviour.
ConeSearch.tex
Outdated
VOTable\citep{2025ivoa.spec.0116O} format. However, if RESPONSEFORMAT is | ||
set (see Sec.~\ref{subsec:baseurl}) the response table \textbf{should} | ||
match the requested format, depending on the service capabilities. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to DALI 1.1 sec 3.4.3 a service
"should fail (Section 4.2 ) where the RESPONSEFORMAT parameter specifies a format not supported by the service implementation."
So maybe reword
the response table \textbf{should} match the requested format; if this format is unsupported the request \textbf{should} fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
check if it's fine now.
\item There must be a single RESOURCE with \texttt{type=''results''} in the VOTable, | ||
and that RESOURCE must contain a single TABLE. Additional RESOURCE(s) are | ||
allowed to enable, e.g., DataLink service descriptors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OVERFLOW signalling as described in DALI 1.1 section 4.4 should be mentioned here as well. Alternatively, just defer to DALI 1.1 sec 4.4 for the whole description of how to write DALI-compliant VOTable results.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Referenced DALI 1.1 Sec.4.4, even if I must say, these sort of references never look perfect to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed referencing fixed sections of fixed versions in other standards is not great, though it does a usability service to readers for as long as the version in question is current. It may be that just referring to the DALI standard without quoting section or version is better, I don't have a strong opinion.
ConeSearch.tex
Outdated
In the case the error is expressed in VOTable format, recommendation | ||
expressed in Section 4.4 of DALI \textbf{should} be followed, | ||
inluding the OVERFLOW behaviour in case the MAXREC parameter is set. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OVERFLOW is not an error condition, so it should not be mentioned here. OVERFLOW is marked as described in DALI 1.1 sec 4.4 as part of a successful response.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed, DALI is also already mentioned wit this respect.
@mbtaylor , please review changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is clearly an improvement compared to ConeSearch 1.03.
I just have a single comment in addition to what @mbtaylor has originally said about the metadata probing mechanism.
ConeSearch.tex
Outdated
or require a service metadata response. Its usage with a value of 0 is encouraged | ||
and preferred to the SR=0 solution for metadata discovery | ||
\footnote{However, keep in mind that, for back-compatibility reasons, MAXREC | ||
is optional and thus SR=0 needs to be accepted for metadata probing.}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I guess that MAXREC=0&SR=0
is equivalent SR=0
; the user will get the metadata.
But what happens if we have just MAXREC=0
? Should we get an error (because SR
is not 0) or just the metadata?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a benefit in designating MAXREC=0
on its own an error. But clients doing this may find (if they're talking to a non-MAXREC-aware service) that they get more data than they want. The same applies to other values of MAXREC
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would leave this question open.
I added a bullet to the running meeting discussion.
My view:
- MAXREC=0 and missing SR is an error since SR is mandatory
- MAXREC=0 and SR not 0 depends on the version the server complies to; so far v.103 only it should answer with records depending on SR value ignoring MAXREC (that cannot be understood), otherwise (let's say v1.1+) it's undefined as of now, it could be a flag for conflict or metadata tailored to the requested SR (if it makes sense)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@molinaro-m you are quite right; my comment missed the point that SR is mandatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit 9366d93 should fix this with respect to DAL Running Meeting 21 discussiondiscussion
ConeSearch.tex
Outdated
% remove the constraint of one RESOURCE only, clarifying it was there | ||
% only for the results | ||
% | ||
% added DALI MAXREC and RESPONSEFORMAT parameters, for response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These items of the Change log are commented. Should some of them be un-commented now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, I'll update the changelog, maybe I'll wait the solutions to the above comments (i.e. the running meeting at least) before making this last update before merging.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
commit c293414 should have fixed this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @molinaro-m, that looks good to me now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes for me this is OK, except that one of the recent commits deleted the preceding backslash from a "textbf{should}", should be fixed.
The change lists in Appendix C are in need of some attention, since with the newly uncommented lines there are items duplicated between headings "Changes from WD-1.1-20200828" and "Changes from REC-1.03"; probably these headings should be consolidated but it doesn't need to be done in this PR.
Should be fixed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems all good to me.
Covering issue #61, adding DALI RESPONSEFORMAT as optional to the ConeSearch query interface.
Consequently clarifying in the succesfull response behaviour that optional response formats are possible, while VOTable 1.x stays as the mandatory one.
Updating ivoatex to include also VOTable-1.5 reference.