[ 
https://issues.apache.org/jira/browse/SOLR-5163?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16621372#comment-16621372
 ] 

Edward Ribeiro commented on SOLR-5163:
--------------------------------------

Hey, [~Charles Sanders], you could simplify things a bit more. Like [~dsmiley] 
pointed out 
https://jira.apache.org/jira/browse/SOLR-5163?focusedCommentId=16595133&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-16595133
 it is more interesting to validate the fields in 
*_DismaxQueryParser#parseQueryFields_* like below:
{code:java}
try {
   for (String e : queryFields.keySet()){
      indexSchema.getField(e);
   }
} catch (SolrException ex) {
  throw new SyntaxError(ex.getMessage());
}

return queryFields;{code}
Or, even use Java8 style like below:
{code:java}
queryFields.keySet().forEach(indexSchema::getField);

return queryFields;{code}
The advantage is double fold: it gets checked for both DismaxParser and 
ExtendedDismaxParser and the logic is in a well defined place (besides being a 
tidy bit more simple).

As for the tests, relying too heavily on String comparisons makes the tests 
fragile (a unrelated change on the return string and the test can fail). IIRC, 
it is even an anti-pattern, but I know it can be difficult not to resort to 
this kind of check then why not make it more economic like below?
{code:java}
/** SOLR-5163 */ 
@Test
public void testValidateQueryFields() throws Exception {

  // test existent qf
  assertJQ(req("defType", "edismax", "df", "text", "q", "olive AND other", 
"qf", "subject^3 title")
      , "/response/numFound==0"
  );

  // test nonexistent qf
  try {
      assertJQ(req("defType", "edismax", "df", "text", "q", "olive AND other", 
"qf", "subject^3 nosuchfield")
          , "/response/numFound==0"
      );
      fail("nosuchfield qf doesn't exists");
  } catch (Exception e) {
     assertEquals("undefined field: \"nosuchfield\"", e.getMessage());
  }{code}
 

Cheers!

> edismax should throw exception when qf refers to nonexistent field
> ------------------------------------------------------------------
>
>                 Key: SOLR-5163
>                 URL: https://issues.apache.org/jira/browse/SOLR-5163
>             Project: Solr
>          Issue Type: Bug
>          Components: query parsers, search
>    Affects Versions: 4.10.4
>            Reporter: Steven Bower
>            Assignee: David Smiley
>            Priority: Major
>              Labels: newdev
>         Attachments: SOLR-5163.patch, SOLR-5163.patch
>
>
> query:
> q=foo AND bar
> qf=field1
> qf=field2
> defType=edismax
> Where field1 exists and field2 doesn't..
> will treat the AND as a term vs and operator



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@lucene.apache.org
For additional commands, e-mail: dev-h...@lucene.apache.org

Reply via email to