[jira] Commented: (SOLR-183) add getRequiredParameter() to SolrParams
[
https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12482686
]
J.J. Larrea commented on SOLR-183:
--
Thanks for clarifying the semantics and the implementation, Ryan.
It's fine by me to remove the strictField logic from getFieldParam; as I
said, I wasn't sure there would be any cases where a developer considered
defining a non-field-limited value (facet.limit) an insufficient means to
fulfill definition of a field-specific value (f.xxx.facet.limit). Should such
a case ever arise, they could subclass RequiredSolrParams to override
getFieldParam and accomplish that themself.
add getRequiredParameter() to SolrParams
Key: SOLR-183
URL: https://issues.apache.org/jira/browse/SOLR-183
Project: Solr
Issue Type: Wish
Reporter: Ryan McKinley
Priority: Trivial
Attachments: RequiredSolrParams.java, SOLR-183-required-param.patch,
SOLR-183-required-param.patch, SOLR-183-required-param.patch,
SOLR-183-required-param.patch, SOLR-183-required-param.patch
I find myself including this with every patch, so i'll just separate it out.
This simply adds a utilty function to SolrParams that throws a 400 if the
parameter is missing:
/** returns the value of the param, or throws a 400 exception if missing */
public String getRequiredParameter(String param) throws SolrException {
String val = get(param);
if( val == null ) {
throw new SolrException( 400, Missing parameter: +param );
}
return val;
}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-183) add getRequiredParameter() to SolrParams
[
https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12482455
]
J.J. Larrea commented on SOLR-183:
--
I totally agree with Ryan that the question I raised about the value of
specifying required params in solrconfig.xml RH definitions should be separated
from this simpler programmer-API case. I will speak no more of that on
SOLR-183.
Ryan, after looking at your patch #4 I've had a change of heart about the
getRequiredXXX approach. To do it properly would require reduplication of
every method signature, e.g. getFieldInt() and so forth, and wouldn't make any
use of the bottleneck imposed by get/getParams. Hoss' decorator approach
coupled with your improved error handling automagically makes everything work
with a trivial subclass.
This time I implemented and tested everything (attachment #5).
RequiredSolrParams is kept as a freestanding class which can be externally
instantiated, but is also returned by a SolrParams.required() convenience
method so we could stash a reference if desired, e.g.
params.required().getInt(xxx)
params.required().getBoolean(yyy)
(but the wasted cycles and amount of garbage created from allocating a new one
is pretty trivial, so perhaps it's best not to add a slot to SolrParams)
In the bottleneck approach the inline-default methods e.g. getBool(xxx, true)
will fail when called on requires - but I think that is not such a bad thing.
Could be fixed if so desired with a _get().
One open question is getFieldParam: Should the semantics of
required.getFieldParam(facet.limit, abc) be to fail if the parameter is not
supplied for the field (e.g. f.abc.facet.limit), or not supplied for either the
field or as a general default (e.g. facet.limit)? In the former case we don't
need to override getFieldParam. I can't think of a reason that one would want
to require explicit field values and disallow general values, but perhaps
someone else could, and a 'field strictness flag should be supplied in the
RequiredSolrParams constructor. For the moment I made it non-strict, but put
in a public value allowing that to be controlled.
I changed the order of operations in SolrParamTest so it starts at the simplest
cases (present and non-required and inline defaults), then malformed values,
then required values. I added the fall-through case for getFieldXXX. I also
started some tests of DefaultSolrParams, to be extended to to
AppendedSolrParams (getParams needs testing as well).
add getRequiredParameter() to SolrParams
Key: SOLR-183
URL: https://issues.apache.org/jira/browse/SOLR-183
Project: Solr
Issue Type: Wish
Reporter: Ryan McKinley
Priority: Trivial
Attachments: RequiredSolrParams.java, SOLR-183-required-param.patch,
SOLR-183-required-param.patch, SOLR-183-required-param.patch
I find myself including this with every patch, so i'll just separate it out.
This simply adds a utilty function to SolrParams that throws a 400 if the
parameter is missing:
/** returns the value of the param, or throws a 400 exception if missing */
public String getRequiredParameter(String param) throws SolrException {
String val = get(param);
if( val == null ) {
throw new SolrException( 400, Missing parameter: +param );
}
return val;
}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-183) add getRequiredParameter() to SolrParams
[
https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12478895
]
J.J. Larrea commented on SOLR-183:
--
Er, sorry to be contrary, but to me it seems a bit excessive to go through so
many hoops to support the getXXX(param, default) methods, which contradicts
the very nature of the class, which is to require parameters.
If one wanted to stick with Hoss' preference for a decorator, and kept the
getXXX(param, default) method signatures defined in SolrParams, one could argue
that it would make sense to make those methods simply return SolrExceptions, on
the assumption that requires.getInt(param, 0) must be a programmer error. That
is of course automatically achieved if only get and getParams are overridden,
as was proposed earlier. It's not so terrible to maintain parallel params and
requires references to the same underlying param list.
But if one is going to bother adding real implementations for every method
signature in SolrParams, then why not simply dispense with the decorator and
add getRequiredXXX(param) methods with default implementations directly to
SolrParams, e.g.
getRequiredParam(String param)
getRequiredParams(String param)
getRequiredBool(String param)
getRequiredFieldBool(String field, String param)
... etc.
That seems simpler, straightforward, and unambiguous.
add getRequiredParameter() to SolrParams
Key: SOLR-183
URL: https://issues.apache.org/jira/browse/SOLR-183
Project: Solr
Issue Type: Wish
Reporter: Ryan McKinley
Priority: Trivial
Attachments: SOLR-183-required-param.patch,
SOLR-183-required-param.patch
I find myself including this with every patch, so i'll just separate it out.
This simply adds a utilty function to SolrParams that throws a 400 if the
parameter is missing:
/** returns the value of the param, or throws a 400 exception if missing */
public String getRequiredParameter(String param) throws SolrException {
String val = get(param);
if( val == null ) {
throw new SolrException( 400, Missing parameter: +param );
}
return val;
}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-183) add getRequiredParameter() to SolrParams
[
https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12478930
]
J.J. Larrea commented on SOLR-183:
--
Modest proposal: If one is going to come up with a programmer-facing mechanism
for required parameters (using any of the abovementioned schemes), why not also
make it configuration-facing as well. That is, in solrparams.xml:
requestHandler name=blah class=solr.DisMaxRequestHandler
lst name=defaults
str name=version2.1/str
int name=rows0/int
...
/lst
lst name=requires
str name=qA query must be specified using the q parameter/str
str name=versionThis handler depends on version being explicitly
set/str
/lst
...
/requestHandler
RequestHandlerBase would add to the definition and initialization of defaults,
extends, and invariants, a fourth SolrParams called requires. Then when the
init is building the (invariants -- ((request -- defaults) + appends))) chain
with DefaultSolrParams and AppendedSolrParams (delegated to method
SolrPluginUtils.setDefaults), it could interpose a new class RequiredSolrParams
which acts like DefaultSolrParams except it accepts the 'requires' SolrParams
defined in the handler config, which in my proposal defines a param
name/message pair. If a param not found in the target SolrParams is defined in
'requires', the exception is thrown. Otherwise the RequiredSolrParams behaves
similarly to DefaultSolrParams (which it extends) by delegating the request up
the chain, or if no chain is defined returning null.
Depending on what the programmer wants, the RequiredSolrParams could be chained
with just the request params:
(invariants - ((requires - request) - defaults) + appends)
or could be chained with the entire chain as it exists:
requires -- (invariants -- ((request -- defaults) + appends)))
I've attached an illustrative implementation. I must apologize, while it
compiles I have not yet tested it, I am under deadline and have spent too much
time on this today already; I'll try to do so over the weekend, along with the
RequestHandlerBase/SolrPluginUtils implementation. It accepts a requires
SolrParams as described above, with the values interpreted as a Formatter
string. It also has an always required mode with a method signature which
accepts a fixed message format string. It also has a convenience method
(temporarily commented out because of method signature clash) which shows how
you can provide custom messages for some parameters but have a stock default
message for others.
I believe this object should be compatible with what Ryan posted, e.g. you
could add implementations for getXXX(param, default) which override the throw
the exception behavior it now has.
Anyway, I am open to feedback. Useful? Excessive? Broken? Stupid?
add getRequiredParameter() to SolrParams
Key: SOLR-183
URL: https://issues.apache.org/jira/browse/SOLR-183
Project: Solr
Issue Type: Wish
Reporter: Ryan McKinley
Priority: Trivial
Attachments: SOLR-183-required-param.patch,
SOLR-183-required-param.patch
I find myself including this with every patch, so i'll just separate it out.
This simply adds a utilty function to SolrParams that throws a 400 if the
parameter is missing:
/** returns the value of the param, or throws a 400 exception if missing */
public String getRequiredParameter(String param) throws SolrException {
String val = get(param);
if( val == null ) {
throw new SolrException( 400, Missing parameter: +param );
}
return val;
}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-183) add getRequiredParameter() to SolrParams
[
https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12478934
]
Ryan McKinley commented on SOLR-183:
It seems bad to have the requited params be user configurable. The real use
case is that the RequestHandler developer wants to ask for a parameter and know
that the error checking is taken care of.
If the required params are configured externally, you run the risk of them
getting out of sync with the handler code - not to mention that it really isn't
something that should be configured. If misconfigured you get a null pointer
exception rather then 400... defeating the purpose altogether.
add getRequiredParameter() to SolrParams
Key: SOLR-183
URL: https://issues.apache.org/jira/browse/SOLR-183
Project: Solr
Issue Type: Wish
Reporter: Ryan McKinley
Priority: Trivial
Attachments: RequiredSolrParams.java, SOLR-183-required-param.patch,
SOLR-183-required-param.patch
I find myself including this with every patch, so i'll just separate it out.
This simply adds a utilty function to SolrParams that throws a 400 if the
parameter is missing:
/** returns the value of the param, or throws a 400 exception if missing */
public String getRequiredParameter(String param) throws SolrException {
String val = get(param);
if( val == null ) {
throw new SolrException( 400, Missing parameter: +param );
}
return val;
}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-183) add getRequiredParameter() to SolrParams
[
https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12478980
]
J.J. Larrea commented on SOLR-183:
--
By the way, I think your logic to catch type conversion errors and return 400
with a specific error rather than let the request dispatcher return a generic
500, is very useful, but should be implemented directly in SolrParams and then
get inherited by RequiredSolrParams, ServletSolrParams, etc.
The concern of supplied or not is different from the concern of well-formed
or not, and params.getInt( param-returning-notint ) is an error, and should
ALWAYS return a specific and informative exception (code and message) as you
have done, regardless of the underlying SolrParams implementation. Ditto for
params.getInt( param-returning-notint, 999 ).
add getRequiredParameter() to SolrParams
Key: SOLR-183
URL: https://issues.apache.org/jira/browse/SOLR-183
Project: Solr
Issue Type: Wish
Reporter: Ryan McKinley
Priority: Trivial
Attachments: RequiredSolrParams.java, SOLR-183-required-param.patch,
SOLR-183-required-param.patch
I find myself including this with every patch, so i'll just separate it out.
This simply adds a utilty function to SolrParams that throws a 400 if the
parameter is missing:
/** returns the value of the param, or throws a 400 exception if missing */
public String getRequiredParameter(String param) throws SolrException {
String val = get(param);
if( val == null ) {
throw new SolrException( 400, Missing parameter: +param );
}
return val;
}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-183) add getRequiredParameter() to SolrParams
[
https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12479175
]
J.J. Larrea commented on SOLR-183:
--
I was unfortunately not very clear, and confounded 2 things, an enhanced
programmer-facing API, based on yours, for request-handler developers, and
secondly an API supported by RequestHandlerBase for request handler
configurators.
From the programmer perspective, my contribution is simply to allow
specification of either a global error format, and/or a parameter-specific
definition of which parameters are required and how missing required
parameters should be reported. It has no negative impact on the use case you
desire, and the modified code should pass all the exists/doesn't exist tests
in your RequiredSolrParamTest.java; if you slapped in your method signatures
that return 400 SolrExceptions on bad type conversion, either into my
RequiredSolrParams or SolrParams as I suggested above, it should pass all the
tests, and if not, I will make it so.
For example,
MapString,String rmap = new HashMapString, String();
rmap.put( q , A query must be specified using the q parameter );
rmap.put( version , This handler depends on version being explicitly
set );
SolrParams required = new RequiredSolrParams( params, new MapSolrParams(
rmap ) );
This is similar to the suggestion in Hoss' first comment on this issue.
The other use-case is for the RequestHandler configurator. There are a lot
more of those than RequestHandler programmers. My model is that they are
defining request handling service APIs by defining requestHandlers in
solrconfig. Those APIs can be used by other web programmers in the
organization, who will make mistakes in calling the API, as we all do.
RequestHandlerBase gives RequestHandler configurators three options for
controlling the API, the invariants defaults and appends. I am simply
proposing a 4th option to define which parameters are required, and the error
message that should be returned in the case it is missing. It's not a
comprehensive parameter validation mechanism, but such would be beyond the
scope of SOLR. However as someone who is actively creating RequestHandler APIs
for other programmers in my organization, using custom code when necessary but
avoiding it whenever possible, I think it might be useful.
And in no way does this second use-case by itself allow RH configurators to
override the first use-case requirements set up by RH programmers, unless the
RH programmers make explicit provision to do so. For example, by chaining a
DefaultSolrParams with params derived from a requestHandler requires list in
front of a default MapSolrParams like the above, the RH programmer allows the
RH configurator to add new requirements, and externally change the error
strings for programmer-supplied requirements, but not to remove
programmer-supplied requirements.
Anyway, hopefully I've better communicated the idea this time.
add getRequiredParameter() to SolrParams
Key: SOLR-183
URL: https://issues.apache.org/jira/browse/SOLR-183
Project: Solr
Issue Type: Wish
Reporter: Ryan McKinley
Priority: Trivial
Attachments: RequiredSolrParams.java, SOLR-183-required-param.patch,
SOLR-183-required-param.patch
I find myself including this with every patch, so i'll just separate it out.
This simply adds a utilty function to SolrParams that throws a 400 if the
parameter is missing:
/** returns the value of the param, or throws a 400 exception if missing */
public String getRequiredParameter(String param) throws SolrException {
String val = get(param);
if( val == null ) {
throw new SolrException( 400, Missing parameter: +param );
}
return val;
}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-183) add getRequiredParameter() to SolrParams
[
https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12478508
]
Hoss Man commented on SOLR-183:
---
Ryan: this patch is nice and simple ... but it has me wondering if it might be
more generally usefull to have this in a SolrParams decorator that applied it
at the outermost level to all of the methods which don't take in a default? ...
SolrParams myParams = ...
myParams = new RequiredSolrParams(myParams, sort, q, qf,
f.foo.facet.limit);
...
public class RequiredSolrParams extends SolrParams {
...
SolrParams nested;
SetString required;
...
public String get(String param) {
String val = nester.get(param);
if (null == val) throw new SolrException( 400, Missing parameter:
+param );
return val;
}
...
public String get(String param, String def) {
return nested.get(param, def); // bypass exception throwing when default
}
...
}
?
add getRequiredParameter() to SolrParams
Key: SOLR-183
URL: https://issues.apache.org/jira/browse/SOLR-183
Project: Solr
Issue Type: Wish
Reporter: Ryan McKinley
Priority: Trivial
Attachments: SOLR-183-required-param.patch
I find myself including this with every patch, so i'll just separate it out.
This simply adds a utilty function to SolrParams that throws a 400 if the
parameter is missing:
/** returns the value of the param, or throws a 400 exception if missing */
public String getRequiredParameter(String param) throws SolrException {
String val = get(param);
if( val == null ) {
throw new SolrException( 400, Missing parameter: +param );
}
return val;
}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-183) add getRequiredParameter() to SolrParams
[
https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12478523
]
Hoss Man commented on SOLR-183:
---
yeha ... the one thing about an approach like this that i'm not sure how i feel
about yet is that it pushes the list of things that should be required away
from where they are actually used (at the moment of construction)
another approach that might cleaner would be to eliminate the explicit list of
required fields, and say that if you use the decorator *every* param is
required unless a default is specified, and then each time you ask for a
param's value, you can use the orriginal params instance if it's not required,
or the decorated params if it is...
SolrParams myParams = ...;
SolrParams required = new RequiredSolrParams(myParams);
...
Integer foo = myParams.getInt(yak); ... not required, may be null
...
Integer bar = required.getInt(yak); ... required in this use, exception if
missing
...
add getRequiredParameter() to SolrParams
Key: SOLR-183
URL: https://issues.apache.org/jira/browse/SOLR-183
Project: Solr
Issue Type: Wish
Reporter: Ryan McKinley
Priority: Trivial
Attachments: SOLR-183-required-param.patch
I find myself including this with every patch, so i'll just separate it out.
This simply adds a utilty function to SolrParams that throws a 400 if the
parameter is missing:
/** returns the value of the param, or throws a 400 exception if missing */
public String getRequiredParameter(String param) throws SolrException {
String val = get(param);
if( val == null ) {
throw new SolrException( 400, Missing parameter: +param );
}
return val;
}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-183) add getRequiredParameter() to SolrParams
[
https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12478525
]
Ryan McKinley commented on SOLR-183:
yes, this is better.
but what should happen with
Integer bar = required.getInt( yak, 100 );
* treat it as required.getInt() that will throw 400 if missing?
* equivolent to myParams.getInt( yak, 100 );?
* unsuported operation? no.
add getRequiredParameter() to SolrParams
Key: SOLR-183
URL: https://issues.apache.org/jira/browse/SOLR-183
Project: Solr
Issue Type: Wish
Reporter: Ryan McKinley
Priority: Trivial
Attachments: SOLR-183-required-param.patch
I find myself including this with every patch, so i'll just separate it out.
This simply adds a utilty function to SolrParams that throws a 400 if the
parameter is missing:
/** returns the value of the param, or throws a 400 exception if missing */
public String getRequiredParameter(String param) throws SolrException {
String val = get(param);
if( val == null ) {
throw new SolrException( 400, Missing parameter: +param );
}
return val;
}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-183) add getRequiredParameter() to SolrParams
[
https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12478552
]
Ryan McKinley commented on SOLR-183:
I'm getting into it now... the easiest is to throw a 400 exception for
everyting. the SolrParams abstract class calls get( '' ) for each of the getX(
name, devault ) - so, we would have to overwrite all the getX functions rather
then just the one. If we do that, we may as well catch the 'parse exception'
from Integer.parseInt() and send a 400 rather then a 500 w/ stack trace.
That is cleaner from user standpoint, so it must be the better option.
add getRequiredParameter() to SolrParams
Key: SOLR-183
URL: https://issues.apache.org/jira/browse/SOLR-183
Project: Solr
Issue Type: Wish
Reporter: Ryan McKinley
Priority: Trivial
Attachments: SOLR-183-required-param.patch
I find myself including this with every patch, so i'll just separate it out.
This simply adds a utilty function to SolrParams that throws a 400 if the
parameter is missing:
/** returns the value of the param, or throws a 400 exception if missing */
public String getRequiredParameter(String param) throws SolrException {
String val = get(param);
if( val == null ) {
throw new SolrException( 400, Missing parameter: +param );
}
return val;
}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
[jira] Commented: (SOLR-183) add getRequiredParameter() to SolrParams
[
https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12478556
]
Yonik Seeley commented on SOLR-183:
---
I like anything that can avoid yet another hash lookup in the common cases.
I think either the original getRequired() or the separate SolrParams
required could fit the bill.
The latter is more powerful since it applies to all get methods, but it's also
more awkward as you need to construct it wherever you need to get a required
param.
add getRequiredParameter() to SolrParams
Key: SOLR-183
URL: https://issues.apache.org/jira/browse/SOLR-183
Project: Solr
Issue Type: Wish
Reporter: Ryan McKinley
Priority: Trivial
Attachments: SOLR-183-required-param.patch
I find myself including this with every patch, so i'll just separate it out.
This simply adds a utilty function to SolrParams that throws a 400 if the
parameter is missing:
/** returns the value of the param, or throws a 400 exception if missing */
public String getRequiredParameter(String param) throws SolrException {
String val = get(param);
if( val == null ) {
throw new SolrException( 400, Missing parameter: +param );
}
return val;
}
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
