You are viewing a plain text version of this content. The canonical link for it is here.
Posted to solr-dev@lucene.apache.org by "Ryan McKinley (JIRA)" <ji...@apache.org> on 2007/03/06 04:23:24 UTC
[jira] Created: (SOLR-183) add getRequiredParameter() to SolrParams
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
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] Updated: (SOLR-183) add getRequiredParameter() to SolrParams
Posted by "Ryan McKinley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ryan McKinley updated SOLR-183:
-------------------------------
Attachment: SOLR-183-required-param.patch
This adds a RequiredSolrParams class that wraps most of the getXXX() functions and makes sure the value exists and is valid.
the case Hoss mentioned:
Integer bar = required.getInt( "yak", null );
isn't possible since getInt() takes an 'int' not an Integer as the default
I put the class in "org.apache.solr.util" rather then "org.apache.solr.request" - I'm really hoping with SOLR-135 most of the general non-lucene based helper classes can be in "util"
You'll notice some of the code style is a little non-standard - that helps my dyslexic head keep stuff straight (at least sometimes).
Yonik - there are no extra hash lookups with this.
> 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
Posted by "J.J. Larrea (JIRA)" <ji...@apache.org>.
[ 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
Posted by "J.J. Larrea (JIRA)" <ji...@apache.org>.
[ 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="version">2.1</str>
<int name="rows">0</int>
...
</lst>
<lst name="requires">
<str name="q">A query must be specified using the q parameter</str>
<str name="version">This 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
Posted by "J.J. Larrea (JIRA)" <ji...@apache.org>.
[ 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,
Map<String,String> rmap = new HashMap<String, 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 <requestHandler>s 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
Posted by "Ryan McKinley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12478517 ]
Ryan McKinley commented on SOLR-183:
------------------------------------
I've been using it as a check just before you use the variable:
String key = params.getRequiredParam( 'key' );
This is nice and simple, the advantage to your suggestion is that you could use it to check non-string values:
SolrParams required = new RequiredSolrParams( params, "size", "debug" );
int size = required.getInt( "size" );
boolean debug = required.getBool( "debug" );
String other = required.get( "somethingelse", "defaultValue" );
I guess simple things might not be as simple as they seem!
> 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] Updated: (SOLR-183) add getRequiredParameter() to SolrParams
Posted by "J.J. Larrea (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
J.J. Larrea updated SOLR-183:
-----------------------------
Attachment: RequiredSolrParams.java
> 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
Posted by "Hoss Man (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12478548 ]
Hoss Man commented on SOLR-183:
-------------------------------
i see no reason why it shouldn't be "equivolent to myParams.getInt( "yak", 100 );" ... here's the interesting case...
Integer bar = required.getInt( "yak", null );
...in that case, i think there should be an exception unless "yak" exists.
the contract would be sumarized as "no method will ever return null, under any circumstances"
> 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
Posted by "J.J. Larrea (JIRA)" <ji...@apache.org>.
[ 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] Updated: (SOLR-183) add getRequiredParameter() to SolrParams
Posted by "Ryan McKinley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ryan McKinley updated SOLR-183:
-------------------------------
Attachment: SOLR-183-required-param.patch
> 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] Updated: (SOLR-183) add getRequiredParameter() to SolrParams
Posted by "J.J. Larrea (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
J.J. Larrea updated SOLR-183:
-----------------------------
Attachment: SOLR-183-required-param.patch
> 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
>
>
> 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
Posted by "Ryan McKinley (JIRA)" <ji...@apache.org>.
[ 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
Posted by "Ryan McKinley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#action_12478905 ]
Ryan McKinley commented on SOLR-183:
------------------------------------
I agree it is a bit excessive... the thing that convinced me the hoops are ok is getting a 400 exception rather then a 500 exception for:
int val = required.getInt( "hello" );
The hoops are ugly, but the result is that anything from the RequiredParams will be valid - and throw a 400 exception if not. In my view, that is a different enough "contract" to warrant a special class rather then adding more functions to SolrParams.
All that said, simply adding getRequiredParam() to SolrParams is simple, clean and solves most cases I'm worried about.
> 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
Posted by "J.J. Larrea (JIRA)" <ji...@apache.org>.
[ 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
Posted by "Hoss Man (JIRA)" <ji...@apache.org>.
[ 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
Posted by "Ryan McKinley (JIRA)" <ji...@apache.org>.
[ 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
Posted by "Ryan McKinley (JIRA)" <ji...@apache.org>.
[ 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
Posted by "J.J. Larrea (JIRA)" <ji...@apache.org>.
[ 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
Posted by "Hoss Man (JIRA)" <ji...@apache.org>.
[ 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;
Set<String> 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] Updated: (SOLR-183) add getRequiredParameter() to SolrParams
Posted by "Ryan McKinley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ryan McKinley updated SOLR-183:
-------------------------------
Attachment: SOLR-183-required-param.patch
This update changes some things in response to JJ's comments.
I agree the "well-formed or not" check should be directly in SolrParams - there is no reason to throw a 500 exception for rather then a 400 for bad input.
That leaves the one open question:
Should getRequiredXXX() go directly in SolrParams or be implemented as a decorator?
This patch puts it directly in SolrParams (I don't care either way, I just want something so that I don't rewrite it for every custom handler). It also adds a test case for SolrParams.
JJ, can we move the RequiredSolrParams.java to a different issue? It seems like a reasonable proposal but it does help the reason i opened this issue: a standard/quick way for the RequestHandler author to make sure parameters are specified.
> 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
Posted by "Yonik Seeley (JIRA)" <ji...@apache.org>.
[ 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.
[jira] Updated: (SOLR-183) add getRequiredParameter() to SolrParams
Posted by "Ryan McKinley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Ryan McKinley updated SOLR-183:
-------------------------------
Attachment: SOLR-183-required-param.patch
Looks good. thanks. I agree it is cleaner as a decorator. As a decorator, I think getInt( 'xxx', defaultVal ) shoould work, not throw an exception.
I don't follow the strict/not strict logic to getFieldParam... If you don't want strict checking, use the normal SolrParams, if you do, use RequiredSolrParams
This update changes things so the basic contract with RequiredSolrParams is that you get back a valid non-null value (unless you pass it in as a default)
* functions with default values call the wrapped params directly
* replaced tabs with 2 spaces
* removed the 'strict' field logic
> 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] Resolved: (SOLR-183) add getRequiredParameter() to
SolrParams
Posted by "Yonik Seeley (JIRA)" <ji...@apache.org>.
[ https://issues.apache.org/jira/browse/SOLR-183?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel ]
Yonik Seeley resolved SOLR-183.
-------------------------------
Resolution: Fixed
Thanks, I just committed this.
> 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.