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

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

Open
jontxu opened this issue Mar 29, 2023 · 2 comments
Assignees

Comments

@jontxu
Copy link

jontxu commented Mar 29, 2023

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()) {
@jontxu jontxu changed the title No adqlgeo features written into capabilities endpoint despite support for all being available No adqlgeo features written into capabilities endpoint despite support being available for all Mar 29, 2023
@gmantele
Copy link
Owner

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.

@gmantele gmantele self-assigned this Mar 29, 2023
@jontxu
Copy link
Author

jontxu commented Mar 29, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants