From 9aa639d45e31059bb2910dade6d7728ea075cd57 Mon Sep 17 00:00:00 2001 From: Chris Hostetter Date: Tue, 19 Jul 2016 11:11:49 -0700 Subject: [PATCH] SOLR-9309: Fix SolrCloud RTG response structure when multi ids requested but only 1 found --- solr/CHANGES.txt | 2 + .../component/RealTimeGetComponent.java | 144 +++++++++++------- .../solr/cloud/TestRandomFlRTGCloud.java | 7 +- 3 files changed, 93 insertions(+), 60 deletions(-) diff --git a/solr/CHANGES.txt b/solr/CHANGES.txt index 0ccccee9227..55fae4737f2 100644 --- a/solr/CHANGES.txt +++ b/solr/CHANGES.txt @@ -159,6 +159,8 @@ Bug Fixes * SOLR-9288: Fix [docid] transformer to return -1 when used in RTG with uncommitted doc (hossman) +* SOLR-9309: Fix SolrCloud RTG response structure when multi ids requested but only 1 found (hossman) + Optimizations ---------------------- diff --git a/solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java b/solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java index 9865a1129da..9018a861eff 100644 --- a/solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java +++ b/solr/core/src/java/org/apache/solr/handler/component/RealTimeGetComponent.java @@ -143,10 +143,9 @@ public class RealTimeGetComponent extends SearchComponent return; } - String id[] = params.getParams("id"); - String ids[] = params.getParams("ids"); - - if (id == null && ids == null) { + final IdsRequsted reqIds = IdsRequsted.parseParams(req); + + if (reqIds.allIds.isEmpty()) { return; } @@ -171,20 +170,6 @@ public class RealTimeGetComponent extends SearchComponent throw new SolrException(SolrException.ErrorCode.BAD_REQUEST, e); } - - String[] allIds = id==null ? new String[0] : id; - - if (ids != null) { - List lst = new ArrayList<>(); - for (String s : allIds) { - lst.add(s); - } - for (String idList : ids) { - lst.addAll( StrUtils.splitSmart(idList, ",", true) ); - } - allIds = lst.toArray(new String[lst.size()]); - } - SolrCore core = req.getCore(); SchemaField idField = core.getLatestSchema().getUniqueKeyField(); FieldType fieldType = idField.getType(); @@ -209,7 +194,7 @@ public class RealTimeGetComponent extends SearchComponent SolrIndexSearcher searcher = null; BytesRefBuilder idBytes = new BytesRefBuilder(); - for (String idStr : allIds) { + for (String idStr : reqIds.allIds) { fieldType.readableToIndexed(idStr, idBytes); if (ulog != null) { Object o = ulog.lookup(idBytes.get()); @@ -297,18 +282,7 @@ public class RealTimeGetComponent extends SearchComponent } } - - // if the client specified a single id=foo, then use "doc":{ - // otherwise use a standard doclist - - if (ids == null && allIds.length <= 1) { - // if the doc was not found, then use a value of null. - rsp.add("doc", docList.size() > 0 ? docList.get(0) : null); - } else { - docList.setNumFound(docList.size()); - rsp.addResponse(docList); - } - + addDocListToResponse(rb, docList); } @@ -461,25 +435,13 @@ public class RealTimeGetComponent extends SearchComponent } public int createSubRequests(ResponseBuilder rb) throws IOException { - SolrParams params = rb.req.getParams(); - String id1[] = params.getParams("id"); - String ids[] = params.getParams("ids"); - - if (id1 == null && ids == null) { + + final IdsRequsted reqIds = IdsRequsted.parseParams(rb.req); + if (reqIds.allIds.isEmpty()) { return ResponseBuilder.STAGE_DONE; } - - List allIds = new ArrayList<>(); - if (id1 != null) { - for (String s : id1) { - allIds.add(s); - } - } - if (ids != null) { - for (String s : ids) { - allIds.addAll( StrUtils.splitSmart(s, ",", true) ); - } - } + + SolrParams params = rb.req.getParams(); // TODO: handle collection=...? @@ -495,7 +457,7 @@ public class RealTimeGetComponent extends SearchComponent Map> sliceToId = new HashMap<>(); - for (String id : allIds) { + for (String id : reqIds.allIds) { Slice slice = coll.getRouter().getTargetSlice(id, null, null, params, coll); List idsForShard = sliceToId.get(slice.getName()); @@ -524,7 +486,7 @@ public class RealTimeGetComponent extends SearchComponent rb.addRequest(this, sreq); } } else { - String shardIdList = StrUtils.join(allIds, ','); + String shardIdList = StrUtils.join(reqIds.allIds, ','); ShardRequest sreq = new ShardRequest(); sreq.purpose = 1; @@ -586,17 +548,31 @@ public class RealTimeGetComponent extends SearchComponent docList.addAll(subList); } } + + addDocListToResponse(rb, docList); + } - if (docList.size() <= 1 && rb.req.getParams().getParams("ids")==null) { + /** + * Encapsulates logic for how a {@link SolrDocumentList} should be added to the response + * based on the request params used + */ + private void addDocListToResponse(final ResponseBuilder rb, final SolrDocumentList docList) { + assert null != docList; + + final SolrQueryResponse rsp = rb.rsp; + final IdsRequsted reqIds = IdsRequsted.parseParams(rb.req); + + if (reqIds.useSingleDocResponse) { + assert docList.size() <= 1; // if the doc was not found, then use a value of null. - rb.rsp.add("doc", docList.size() > 0 ? docList.get(0) : null); + rsp.add("doc", docList.size() > 0 ? docList.get(0) : null); } else { docList.setNumFound(docList.size()); - rb.rsp.addResponse(docList); + rsp.addResponse(docList); } } - + //////////////////////////////////////////// /// SolrInfoMBean @@ -768,6 +744,66 @@ public class RealTimeGetComponent extends SearchComponent return new ArrayList<>(versionsToRet); } + /** + * Simple struct for tracking what ids were requested and what response format is expected + * acording to the request params + */ + private final static class IdsRequsted { + /** An List (which may be empty but will never be null) of the uniqueKeys requested. */ + public final List allIds; + /** + * true if the params provided by the user indicate that a single doc response structure + * should be used. + * Value is meaninless if ids is empty. + */ + public final boolean useSingleDocResponse; + private IdsRequsted(List allIds, boolean useSingleDocResponse) { + assert null != allIds; + this.allIds = allIds; + this.useSingleDocResponse = useSingleDocResponse; + } + + /** + * Parsers the id and ids params attached to the specified request object, + * and returns an IdsRequsted struct to use for this request. + * The IdsRequsted is cached in the {@link SolrQueryRequest#getContext} so subsequent + * method calls on the same request will not re-parse the params. + */ + public static IdsRequsted parseParams(SolrQueryRequest req) { + final String contextKey = IdsRequsted.class.toString() + "_PARSED_ID_PARAMS"; + if (req.getContext().containsKey(contextKey)) { + return (IdsRequsted)req.getContext().get(contextKey); + } + final SolrParams params = req.getParams(); + final String id[] = params.getParams("id"); + final String ids[] = params.getParams("ids"); + + if (id == null && ids == null) { + IdsRequsted result = new IdsRequsted(Collections.emptyList(), true); + req.getContext().put(contextKey, result); + return result; + } + final List allIds = new ArrayList<>((null == id ? 0 : id.length) + + (null == ids ? 0 : (2 * ids.length))); + if (null != id) { + for (String singleId : id) { + allIds.add(singleId); + } + } + if (null != ids) { + for (String idList : ids) { + allIds.addAll( StrUtils.splitSmart(idList, ",", true) ); + } + } + // if the client specified a single id=foo, then use "doc":{ + // otherwise use a standard doclist + IdsRequsted result = new IdsRequsted(allIds, (ids == null && allIds.size() <= 1)); + req.getContext().put(contextKey, result); + return result; + } + } + + /** * A lite weight ResultContext for use with RTG requests that can point at Realtime Searchers */ diff --git a/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java b/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java index 682d6a03aac..8fc61c7be0e 100644 --- a/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java +++ b/solr/core/src/test/org/apache/solr/cloud/TestRandomFlRTGCloud.java @@ -387,16 +387,11 @@ public class TestRandomFlRTGCloud extends SolrCloudTestCase { * trivial helper method to deal with diff response structure between using a single 'id' param vs * 2 or more 'id' params (or 1 or more 'ids' params). * - * NOTE: expectList is currently ignored due to SOLR-9309 -- instead best efforst are made to - * return a synthetic list based on whatever can be found in the response. - * * @return List from response, or a synthetic one created from single response doc if * expectList was false; May be empty; May be null if response included null list. */ private static SolrDocumentList getDocsFromRTGResponse(final boolean expectList, final QueryResponse rsp) { - // TODO: blocked by SOLR-9309 (once this can be fixed, update jdocs) - if (null != rsp.getResults()) { // TODO: replace this.. - // if (expectList) { // TODO: ...with this tighter check. + if (expectList) { return rsp.getResults(); }