Conversation
cadc-conesearch/README.md
Outdated
| @@ -0,0 +1,68 @@ | |||
| # cadc-conesearch (1.0.0) | |||
There was a problem hiding this comment.
I wouldn't mention specific versions here (or below in sample gradle build) because they will fall out of date before we know it.
cadc-conesearch/README.md
Outdated
| } | ||
| ``` | ||
|
|
||
| `${HOME}/config/cadc-conesearch.properties`: |
There was a problem hiding this comment.
I was expecting that the service woulds have the config (rather than the lib - usually, reserve library config for cases where the lib is a plugin that the app/service cannot configure)... it really doesn't help to provide this since the ctor for TAPQueryGenerator takes all the necessary config as arguments anyway (it's utility).
Also, it won't suffice for a containerised cone service in CADC/CANFAR for a variety of reasons, so it will be code we don't use. If you want the ConeSearchConfig class in then alma impl, that's fine, but I wouldn't include it here.
cadc-conesearch/README.md
Outdated
| Capture the Cone Search parameters and translate to ADQL to be sent to a TAP service. | ||
|
|
||
| ```java | ||
| final ConeSearchConfig coneSearchConfig = new ConeSearchConfig(); // Ensure the cadc-conesearch.properties exists. |
There was a problem hiding this comment.
I'm really not a fan of code samples like this. It will be wrong and not even compile in no time.
|
|
||
| package org.opencadc.conesearch; | ||
|
|
||
| public enum ParameterNames { |
There was a problem hiding this comment.
This should use the same approach as cadc-sia: ConeParamValidator extends CommonParamValidator from cadc-dali and add the SCS-specific params and methods. "common" just means "used in more than one standard"
Then maybe you need to add List<Double> validateDouble(...) to the CommonValidator instead of a whole class (and test class) here.
| protected final String midVerbositySelectList; | ||
| protected final String highVerbositySelectList; | ||
|
|
||
| public TAPQueryGenerator(final String catalog, final String lowVerbositySelectList, |
There was a problem hiding this comment.
The position column also must be a ctor arg since it is tightly coupled with the table name. catalog is pretty generic so I would use tableName and posColumnName.
The verb-dependent select lists I see as a pretty fragile way to pick columns, but I can't think of anything bulletproof so I guess simplest is the right choice now. I can imagine too many variations and they would all be complex :-)
This generator is a different style from the sia2/AdqlQueryGenerator; that class takes the input param map in the ctor and returns the TAP params from a no-arg `getParameterMap(). I can see you looked at that because both of them have the same incorrect comment mentioning the REQUEST param (which no longer exists in TAP). The sia2 style makes a single use generator, while this allows calling code to generate different queries with different inputs; there are no use cases for the latter since a cone search request is inherently single query.
Finally, the ctor should check all args for null and throw IllegalArgumentException if necessary.
| /** | ||
| * Class to produce an ADQL query from a set of Parameters | ||
| */ | ||
| public class TAPQueryGenerator extends CommonParamValidator { |
There was a problem hiding this comment.
As I mentioned in another comment, I expected something like ConeParamValidator extends CommonParamValidator and that the query generator would use it (rather than be the thing extending). It separates the two things and allows the ConeParamValidator to be used without the query generator. Given that SCS has 3 params that map to one Circle constraint, I would probably have a method like
public Circle ConeParamValidator.validateCone(Map<String, List<String>> params)
to encapsulate/manage that old style.
So, you should end up with ConeParamValidator and TAPQueryGenerator (ugh, acronyms in class names).
| final Map<String, Object> queryParameterMap = new HashMap<>(); | ||
|
|
||
| // Obtain and, if necessary, provide a default RESPONSEFORMAT. | ||
| final String requestedResponseFormat = getFirstParameter(ParameterNames.RESPONSEFORMAT.name(), parameters, |
There was a problem hiding this comment.
ConeSearch allows for output format other than VOTable?
If so, use CommonParamValidator.getResponseFormat(...)
There was a problem hiding this comment.
| final String requestedOutputVerbosity = getFirstParameter(ParameterNames.VERB.name(), parameters, | ||
| Integer.toString(MID_VERB_VALUE)); | ||
| final NumberParameterValidator outputVerbosityValidator = | ||
| new NumberParameterValidator(false, TAPQueryGenerator.MIN_VERB_VALUE, |
There was a problem hiding this comment.
rather than a separate NumberParameterValidator, just use CommonParamValidator.validateInteger(...). You'll have to deal with getting the first value from the list here (multi-valued params are the norm now so not generically supported in CommonParamValidator)
| final int outputVerbosity = outputVerbosityValidator.validate(requestedOutputVerbosity); | ||
|
|
||
| // Obtain and validate the MAXREC value with a default if necessary. | ||
| final String requestedMaxRecords = getFirstParameter(ParameterNames.MAXREC.name(), parameters, |
There was a problem hiding this comment.
MAXREC should be added to the CommonParamValidator, eg
public Integer getMaxRec(Map<>, Integer defaultValue, Integer maxValue)
where both integer args could be null (server-side choice). There is a MaxRecValidator in cadc-dali that is used in TAP, but it currently requires a Job so isn't usable here, but the code in it's validate() method is easily adapted.
|
Review work submitted. They're a little lengthy, and now include changes to the |
2 participants
No description provided.