lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Edward Ribeiro (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (SOLR-5163) edismax should throw exception when qf refers to nonexistent field
Date Thu, 20 Sep 2018 01:01:00 GMT

    [ 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-unsubscribe@lucene.apache.org
For additional commands, e-mail: dev-help@lucene.apache.org


Mime
View raw message