SOLR-5163: edismax now throws an exception when qf refers to a nonexistent field

solr/CHANGES.txt

@@ -71,6 +71,8 @@ Other Changes
   java.time.DateTimeFormatter instead of Joda time (see upgrade notes).  "Lenient" is enabled.  Removed Joda Time dependency.
   (David Smiley, Bar Rotstein)
   
+* SOLR-5163: edismax now throws an exception when qf refers to a nonexistent field (Charles Sanders, David Smiley)
+
 * SOLR-12805: Store previous term (generation) of replica when start recovery process (Cao Manh Dat)
 
 * SOLR-12652: Remove SolrMetricManager.overridableRegistryName method (Peter Somogyi via David Smiley)

solr/core/src/java/org/apache/solr/search/ExtendedDismaxQParser.java

@@ -48,6 +48,7 @@ import org.apache.lucene.search.PhraseQuery;
 import org.apache.lucene.search.Query;
 import org.apache.lucene.util.Version;
 import org.apache.solr.analysis.TokenizerChain;
+import org.apache.solr.common.SolrException;
 import org.apache.solr.common.params.DisMaxParams;
 import org.apache.solr.common.params.SolrParams;
 import org.apache.solr.common.util.NamedList;
@@ -55,6 +56,8 @@ import org.apache.solr.parser.QueryParser;
 import org.apache.solr.parser.SolrQueryParserBase.MagicFieldName;
 import org.apache.solr.request.SolrQueryRequest;
 import org.apache.solr.schema.FieldType;
+import org.apache.solr.schema.IndexSchema;
+import org.apache.solr.search.ExtendedDismaxQParser.ExtendedSolrQueryParser.Alias;
 import org.apache.solr.util.SolrPluginUtils;
 
 import com.google.common.collect.Multimap;
@@ -144,6 +147,7 @@ public class ExtendedDismaxQParser extends QParser {
       ExtendedSolrQueryParser up = createEdismaxQueryParser(this, IMPOSSIBLE_FIELD_NAME);
       up.addAlias(IMPOSSIBLE_FIELD_NAME, config.tiebreaker, config.queryFields);
       addAliasesFromRequest(up, config.tiebreaker);
+      validateQueryFields(up);
       up.setPhraseSlop(config.qslop);     // slop for explicit user phrase queries
       up.setAllowLeadingWildcard(true);
       up.setAllowSubQueryParsing(config.userFields.isAllowed(MagicFieldName.QUERY.field));
@@ -205,6 +209,84 @@ public class ExtendedDismaxQParser extends QParser {
     return topQuery;
   }
   
+  /**
+   * Validate query field names. Must be explicitly defined in the schema or match a dynamic field pattern.
+   * Checks source field(s) represented by a field alias
+   * 
+   * @param up parser used
+   * @throws SyntaxError for invalid field name
+   */
+  protected void validateQueryFields(ExtendedSolrQueryParser up) throws SyntaxError {
+    List<String> flds = new ArrayList<>(config.queryFields.keySet().size());
+    for (String fieldName : config.queryFields.keySet()) {
+      buildQueryFieldList(fieldName, up.getAlias(fieldName), flds, up);
+    }
+    
+    checkFieldsInSchema(flds);
+  }
+  
+  /**
+   * Build list of source (non-alias) query field names. Recursive through aliases.
+   * 
+   * @param fieldName query field name
+   * @param alias field alias
+   * @param flds list of query field names
+   * @param up parser used
+   * @throws SyntaxError for invalid field name
+   */
+  private void buildQueryFieldList(String fieldName, Alias alias, List<String> flds, ExtendedSolrQueryParser up) throws SyntaxError {
+    if (null == alias) {
+        flds.add(fieldName);
+        return;
+    }
+
+    up.validateCyclicAliasing(fieldName);
+    flds.addAll(getFieldsFromAlias(up, alias));
+  }
+  
+  /**
+   * Return list of source (non-alias) field names from an alias
+   * 
+   * @param up parser used
+   * @param a field alias
+   * @return list of source fields
+   * @throws SyntaxError for invalid field name
+   */
+  private List<String> getFieldsFromAlias(ExtendedSolrQueryParser up, Alias a) throws SyntaxError {
+    List<String> lst = new ArrayList<>();
+    for (String s : a.fields.keySet()) {
+      buildQueryFieldList(s, up.getAlias(s), lst, up);
+    }
+
+    return lst;
+  }
+  
+  /**
+   * Verify field name exists in schema, explicit or dynamic field pattern
+   * 
+   * @param fieldName source field name to verify
+   * @throws SyntaxError for invalid field name
+   */
+  private void checkFieldInSchema(String fieldName) throws SyntaxError {
+    try {
+        config.schema.getField(fieldName);
+    } catch (SolrException se) {
+        throw new SyntaxError("Query Field '" + fieldName + "' is not a valid field name", se);
+    }
+  }
+
+  /**
+   * Verify list of source field names
+   * 
+   * @param flds list of source field names to verify
+   * @throws SyntaxError for invalid field name
+   */
+  private void checkFieldsInSchema(List<String> flds) throws SyntaxError {
+    for (String fieldName : flds) {
+        checkFieldInSchema(fieldName);
+    }
+  }
+  
   /**
    * Adds shingled phrase queries to all the fields specified in the pf, pf2 anf pf3 parameters
    * 
@@ -1598,14 +1680,17 @@ public class ExtendedDismaxQParser extends QParser {
 
     protected boolean splitOnWhitespace;
     
+    protected IndexSchema schema;
+
     public ExtendedDismaxConfiguration(SolrParams localParams,
         SolrParams params, SolrQueryRequest req) {
       solrParams = SolrParams.wrapDefaults(localParams, params);
-      minShouldMatch = DisMaxQParser.parseMinShouldMatch(req.getSchema(), solrParams); // req.getSearcher() here causes searcher refcount imbalance
+      schema = req.getSchema();
+      minShouldMatch = DisMaxQParser.parseMinShouldMatch(schema, solrParams); // req.getSearcher() here causes searcher refcount imbalance
       final boolean forbidSubQueryByDefault = req.getCore().getSolrConfig().luceneMatchVersion.onOrAfter(Version.LUCENE_7_2_0);
       userFields = new UserFields(U.parseFieldBoosts(solrParams.getParams(DMP.UF)), forbidSubQueryByDefault);
       try {
-        queryFields = DisMaxQParser.parseQueryFields(req.getSchema(), solrParams);  // req.getSearcher() here causes searcher refcount imbalance
+        queryFields = DisMaxQParser.parseQueryFields(schema, solrParams);  // req.getSearcher() here causes searcher refcount imbalance
       } catch (SyntaxError e) {
         throw new RuntimeException(e);
       }

solr/core/src/test/org/apache/solr/search/TestExtendedDismaxParser.java

@@ -38,6 +38,7 @@ import org.apache.solr.common.params.ModifiableSolrParams;
 import org.apache.solr.common.params.SolrParams;
 import org.apache.solr.request.SolrQueryRequest;
 import org.apache.solr.util.SolrPluginUtils;
+import org.junit.Assert;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.noggit.ObjectBuilder;
@@ -672,7 +673,8 @@ public class TestExtendedDismaxParser extends SolrTestCaseJ4 {
       try {
         h.query(req("defType","edismax", "q","blarg", "qf","field1", "f.field1.qf","field2 field3","f.field2.qf","field4 field5", "f.field4.qf","field5", "f.field5.qf","field6", "f.field3.qf","field6"));
       } catch (SolrException e) {
-        fail("This is not cyclic alising");
+        assertFalse("This is not cyclic alising", e.getCause().getMessage().contains("Field aliases lead to a cycle"));
+        assertTrue(e.getCause().getMessage().contains("not a valid field name"));
       }
       
       try {
@@ -683,7 +685,7 @@ public class TestExtendedDismaxParser extends SolrTestCaseJ4 {
       }
       
       try {
-        h.query(req("defType","edismax", "q","who:(Zapp Pig)", "qf","field1", "f.who.qf","name","f.name.qf","myalias", "f.myalias.qf","who"));
+        h.query(req("defType","edismax", "q","who:(Zapp Pig)", "qf","text", "f.who.qf","name","f.name.qf","myalias", "f.myalias.qf","who"));
         fail("Cyclic alising not detected");
       } catch (SolrException e) {
         assertTrue(e.getCause().getMessage().contains("Field aliases lead to a cycle"));
@@ -2091,4 +2093,31 @@ public class TestExtendedDismaxParser extends SolrTestCaseJ4 {
     assertJQ(req("defType", "edismax", "q", "*", "qq", "{!edismax v=something}", "bq", "{!edismax v=$qq}"));
   }
   
+  /** SOLR-5163 */ 
+  @Test
+  public void testValidateQueryFields() throws Exception {
+    // field aliasing covered by test - testAliasing
+    ModifiableSolrParams params = new ModifiableSolrParams();
+    params.add("defType", "edismax");
+    params.add("df", "text");
+    params.add("q", "olive AND other");
+    params.add("qf", "subject^3 title");
+    params.add("debugQuery", "true");
+    
+    // test valid field names
+    try (SolrQueryRequest req = req(params)) {
+      String response = h.query(req);
+      response.contains("+DisjunctionMaxQuery((title:olive | (subject:oliv)^3.0)) +DisjunctionMaxQuery((title:other | (subject:other)^3.0))");
+    }
+    
+    // test invalid field name
+    params.set("qf", "subject^3 nosuchfield");
+    try (SolrQueryRequest req = req(params)) {
+      h.query(req);
+    } catch (Exception e) {
+      Assert.assertEquals("org.apache.solr.search.SyntaxError: Query Field 'nosuchfield' is not a valid field name", e.getMessage());
+    }
+    
+  }
+
 }