gmantele / vollt

Java libraries implementing the IVOA protocol: ADQL, UWS and TAP
http://cdsportal.u-strasbg.fr/taptuto/
29 stars 28 forks source link

No adqlgeo features written into `capabilities` endpoint despite support being available for all #144

Open jontxu opened 1 year ago

jontxu commented 1 year ago

I was notified by @mbtaylor that the capabilities endpoint from our TAP service does not show information regarding ADQL language features related to geometries (or adqlgeo). I was surprised because all of them are supported, since we follow the tap_full.properties configuration file, where it states that

If the list is empty (no item), all functions are allowed.

So far so good. Although still not the whole picture.

Looking at the code, the issue seems to be that:

  1. The geometries list from ConfigurableServiceConnection will be null whenever its configuration counterpart is either implicitly (i.e., geometries=) or explicitly (i.e., geometries=ANY) set to allow all geometrical features according to ConfigurableServiceConnection#initADQLGeometries().
  2. When obtaining the capabilities of the TAP service via TAP#getCapability(), it only looks for a non-empty list. In other words, it will print the geometry features if and only if explicitly stated in the configuration file.

Luckily the fix is straightforward: split the regular expression containing the allowed language features (ConfigurableServiceConnection.GEOMETRY_REGEXP) and setting geometries like so:

geometries = new ArrayList<String>(Arrays.asList(GEOMETRY_REGEXP.substring(1, GEOMETRY_REGEXP.length() - 1).split("\\|")));

As the geometries list will not be null anymore, the next line from TAP#getCapability()

if (service.getGeometries() != null && service.getGeometries().size() > 0){

Could also be replaced with a more concise

if (!service.getGeometries().isEmpty()) {
gmantele commented 1 year ago

The branch adql2.1 implements completely ADQL-2.1 standard (minus some finalization here and there). However, the TAP library is not yet compatible with this new ADQL implementation. Listing all optional features in the Capabilities entrypoint is one of the many things I have to take care to allow this full compatibility. So, it is perfectly normal for the moment that a TAP service running with this adql-2.1 branch may be incomplete and even unstable. That's why ADQL-2.1 implementation is still in a branch.

@jontxu , be free to apply your fix if you want geometries to appear in the capabilities. But be aware that I will surely do this differently as I will have to declare in a generic way all optional features added in ADQL-2.1.

Again, sorry for being so slow finishing upgrading VOLLT (with TAP-1.1 and ADQL-2.1). I am still not able to dedicate all my time on VOLLT, though it has now a higher priority with the upcoming RFC of ADQL-2.1.

jontxu commented 1 year ago

Hi @gmantele, no need to feel sorry, you're doing a great job with VOLLT and I don't want you to feel pressured (neither was my intention). I am just informing you of a bug @mbtaylor and I encountered.

My fork is based on the master branch, which is obviously behind the currently unstable adql-2.1 branch and will eventually be superseded by it (it's around ~70 commits ahead already). That's why I looked into adql-2.1 just in case... and the same happened, as they share that codebase at this point. Even if you eventually change it, I think noting it can be helpful, as people might clone the project on the meantime.

@jontxu , be free to apply your fix if you want geometries to appear in the capabilities. But be aware that I will surely do this differently as I will have to declare in a generic way all optional features added in ADQL-2.1.

Yeah, I'm fully aware of this, and that's why I reported an issue before doing a pull request. I could do it on master if you'd like (people might clone it on the meantime), but I'm not sure about the roadmap as you're focused on ADQL 2.1 (and rightfully so). I did as written above and it work well at the moment... except VOLLT still assumes NULL arrays to support all geometries -- I think. I'll look into this

By the way, if you'd somehow need help on this rewrite I'd be up for it. Speeding up the process ought to be helpful on the long run, and your workload would also be reduced allowing you to focus in other things.