From e584c9d1405576fceceea3c10ec69582a7982b19 Mon Sep 17 00:00:00 2001 From: Munendra S N Date: Tue, 20 Aug 2019 09:44:05 +0530 Subject: [PATCH] SOLR-13704: correct error codes for client errors in expand component --- solr/CHANGES.txt | 2 + .../handler/component/ExpandComponent.java | 48 +++++++++-------- .../DistributedExpandComponentTest.java | 13 +++-- .../component/TestExpandComponent.java | 54 +++++++++++++++++-- 4 files changed, 88 insertions(+), 29 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index bf202a12f2d..15a1777f8dd 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -110,6 +110,8 @@ Bug Fixes * SOLR-6328: facet.missing=true should return missing count even when facet.limit=0 (hossman, Munendra S N) +* SOLR-13704: Improve error message and change error code to 400 for client errors in ExpandComponent (Munendra S N) + Other Changes ---------------------- diff --git a/solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java b/solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java index bda643bcf98..85f0ae57269 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java +++ b/solr/core/src/java/org/apache/solr/handler/component/ExpandComponent.java @@ -61,6 +61,7 @@ import org.apache.lucene.util.CharsRefBuilder; import org.apache.lucene.util.FixedBitSet; import org.apache.lucene.util.LongValues; import org.apache.solr.common.SolrDocumentList; +import org.apache.solr.common.SolrException; import org.apache.solr.common.params.ExpandParams; import org.apache.solr.common.params.SolrParams; import org.apache.solr.common.util.NamedList; @@ -79,6 +80,7 @@ import org.apache.solr.search.DocSlice; import org.apache.solr.search.QParser; import org.apache.solr.search.SolrIndexSearcher; import org.apache.solr.search.SortSpecParsing; +import org.apache.solr.search.SyntaxError; import org.apache.solr.util.plugin.PluginInfoInitialized; import org.apache.solr.util.plugin.SolrCoreAware; @@ -146,7 +148,7 @@ public class ExpandComponent extends SearchComponent implements PluginInfoInitia } if (field == null) { - throw new IOException("Expand field is null."); + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, "missing expand field"); } String sortParam = params.get(ExpandParams.EXPAND_SORT); @@ -161,39 +163,34 @@ public class ExpandComponent extends SearchComponent implements PluginInfoInitia } Query query; - if (qs == null) { - query = rb.getQuery(); - } else { - try { + List newFilters = new ArrayList<>(); + try { + if (qs == null) { + query = rb.getQuery(); + } else { QParser parser = QParser.getParser(qs, req); query = parser.getQuery(); - } catch (Exception e) { - throw new IOException(e); } - } - List newFilters = new ArrayList<>(); - - if (fqs == null) { - List filters = rb.getFilters(); - if (filters != null) { - for (Query q : filters) { - if (!(q instanceof CollapsingQParserPlugin.CollapsingPostFilter)) { - newFilters.add(q); + if (fqs == null) { + List filters = rb.getFilters(); + if (filters != null) { + for (Query q : filters) { + if (!(q instanceof CollapsingQParserPlugin.CollapsingPostFilter)) { + newFilters.add(q); + } } } - } - } else { - try { + } else { for (String fq : fqs) { if (fq != null && fq.trim().length() != 0 && !fq.equals("*:*")) { QParser fqp = QParser.getParser(fq, req); newFilters.add(fqp.getQuery()); } } - } catch (Exception e) { - throw new IOException(e); } + } catch (SyntaxError e) { + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, e); } SolrIndexSearcher searcher = req.getSearcher(); @@ -214,7 +211,7 @@ public class ExpandComponent extends SearchComponent implements PluginInfoInitia } else { values = DocValues.getSorted(reader, field); } - } else { + } else if (fieldType.getNumberType() != null) { //Get the nullValue for the numeric collapse field String defaultValue = searcher.getSchema().getField(field).getDefaultValue(); @@ -223,6 +220,9 @@ public class ExpandComponent extends SearchComponent implements PluginInfoInitia // Since the expand component depends on the operation of the collapse component, // which validates that numeric field types are 32-bit, // we don't need to handle invalid 64-bit field types here. + // FIXME: what happens when expand.field specified? + // how would this work for date field? + // SOLR-10400: before this, long and double were explicitly handled if (defaultValue != null) { if (numType == NumberType.INTEGER) { nullValue = Long.parseLong(defaultValue); @@ -232,6 +232,10 @@ public class ExpandComponent extends SearchComponent implements PluginInfoInitia } else if (NumberType.FLOAT.equals(numType)) { // Integer case already handled by nullValue defaulting to 0 nullValue = Float.floatToIntBits(0.0f); } + } else { + // possible if directly expand.field is specified + throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, + "Expand not supported for fieldType:'" + fieldType.getTypeName() +"'"); } FixedBitSet groupBits = null; diff --git a/solr/core/src/test/org/apache/solr/handler/component/DistributedExpandComponentTest.java b/solr/core/src/test/org/apache/solr/handler/component/DistributedExpandComponentTest.java index 5633c46f551..7a4d7ce5532 100644 --- a/solr/core/src/test/org/apache/solr/handler/component/DistributedExpandComponentTest.java +++ b/solr/core/src/test/org/apache/solr/handler/component/DistributedExpandComponentTest.java @@ -16,17 +16,18 @@ */ package org.apache.solr.handler.component; +import java.util.Iterator; +import java.util.Map; + import org.apache.solr.BaseDistributedSearchTestCase; import org.apache.solr.client.solrj.response.QueryResponse; import org.apache.solr.common.SolrDocument; import org.apache.solr.common.SolrDocumentList; +import org.apache.solr.common.SolrException; import org.apache.solr.common.params.ModifiableSolrParams; import org.junit.BeforeClass; import org.junit.Test; -import java.util.Map; -import java.util.Iterator; - /** * Test for QueryComponent's distributed querying * @@ -90,6 +91,12 @@ public class DistributedExpandComponentTest extends BaseDistributedSearchTestCas query("q", "*:*", "start","1", "rows", "1", "fq", "{!collapse field="+group+"}", "defType", "edismax", "bf", "field(test_i)", "expand", "true", "fl","*,score"); + ignoreException("missing expand field"); + SolrException e = expectThrows(SolrException.class, () -> query("q", "*:*", "expand", "true")); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code()); + assertTrue(e.getMessage().contains("missing expand field")); + resetExceptionIgnores(); + //First basic test case. ModifiableSolrParams params = new ModifiableSolrParams(); params.add("q", "*:*"); diff --git a/solr/core/src/test/org/apache/solr/handler/component/TestExpandComponent.java b/solr/core/src/test/org/apache/solr/handler/component/TestExpandComponent.java index 9c976d819b4..f5d26ef162a 100644 --- a/solr/core/src/test/org/apache/solr/handler/component/TestExpandComponent.java +++ b/solr/core/src/test/org/apache/solr/handler/component/TestExpandComponent.java @@ -21,6 +21,7 @@ import java.util.Collections; import java.util.List; import org.apache.solr.SolrTestCaseJ4; +import org.apache.solr.common.SolrException; import org.apache.solr.common.params.ModifiableSolrParams; import org.apache.solr.search.CollapsingQParserPlugin; import org.junit.Before; @@ -44,8 +45,6 @@ public class TestExpandComponent extends SolrTestCaseJ4 { assertU(commit()); } - - @Test public void testExpand() throws Exception { List groups = new ArrayList<>(); @@ -57,7 +56,7 @@ public class TestExpandComponent extends SolrTestCaseJ4 { String hint = (random().nextBoolean() ? " hint="+ CollapsingQParserPlugin.HINT_TOP_FC : ""); - _testExpand(groups.get(0), floatAppend, hint); + _testExpand(groups.get(0), floatAppend, hint); } @Test @@ -77,7 +76,7 @@ public class TestExpandComponent extends SolrTestCaseJ4 { String hint = ""; - _testExpand(groups.get(0), floatAppend, hint); + _testExpand(groups.get(0), floatAppend, hint); } private void _testExpand(String group, String floatAppend, String hint) throws Exception { @@ -331,4 +330,51 @@ public class TestExpandComponent extends SolrTestCaseJ4 { assertQ(req(params), "*[count(//doc)=0]"); } + + @Test + public void testErrorCases() { + String[] doc = {"id","1", "term_s", "YYYY", "text_t", "bleh bleh", "test_i", "5000", "test_l", "100", "test_f", "200"}; + assertU(adoc(doc)); + assertU(commit()); + String[] doc1 = {"id","2", "term_s", "YYYY", "text_t", "bleh bleh", "test_i", "500", "test_l", "1000", "test_f", "2000"}; + assertU(adoc(doc1)); + + ignoreException("missing expand field"); + ignoreException("Expected identifier at pos 2"); + ignoreException("Can't determine a Sort Order"); + ignoreException("Expand not supported for fieldType:'text'"); + + // no expand field + SolrException e = expectThrows(SolrException.class, () -> h.query(req("q", "*:*", "expand", "true"))); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code()); + assertEquals("missing expand field", e.getMessage()); + + // query and filter syntax errors + e = expectThrows(SolrException.class, () -> h.query(req("q", "*:*", "expand", "true", + "expand.field", "term_s", "expand.q", "{!"))); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code()); + assertTrue(e.getMessage().contains("Expected identifier at pos 2 str='{!'")); + + e = expectThrows(SolrException.class, () -> h.query(req("q", "*:*", "expand", "true", + "expand.field", "term_s", "expand.q", "*:*", "expand.fq", "{!"))); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code()); + assertTrue(e.getMessage().contains("Expected identifier at pos 2 str='{!'")); + + e = expectThrows(SolrException.class, () -> h.query(req("q", "*:*", "expand", "true", + "expand.field", "term_s", "expand.q", "*:*", "expand.fq", "{!"))); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code()); + assertTrue(e.getMessage().contains("Expected identifier at pos 2 str='{!'")); + + e = expectThrows(SolrException.class, () -> h.query(req("q", "*:*", "expand", "true", + "expand.field", "term_s", "expand.q", "*:*", "expand.sort", "bleh"))); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code()); + assertTrue(e.getMessage().contains("Can't determine a Sort Order (asc or desc) in sort spec 'bleh'")); + + e = expectThrows(SolrException.class, () -> h.query(req("q", "*:*", "expand", "true", + "expand.field", "text_t", "expand.q", "*:*"))); + assertEquals(SolrException.ErrorCode.BAD_REQUEST.code, e.code()); + assertEquals("Expand not supported for fieldType:'text'", e.getMessage()); + + resetExceptionIgnores(); + } }