Skip to content

Commit

Permalink
iter
Browse files Browse the repository at this point in the history
  • Loading branch information
@javanna
javanna committed Feb 7, 2024
1 parent f380d97 commit 1b037ae
Show file tree
Hide file tree
Showing 4 changed files with 184 additions and 88 deletions.
19 changes: 7 additions & 12 deletions server/src/internalClusterTest/java/org/elasticsearch/search/searchafter/SearchAfterIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

package org.elasticsearch.search.searchafter;

import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.action.admin.indices.create.CreateIndexRequestBuilder;
import org.elasticsearch.action.bulk.BulkRequestBuilder;
import org.elasticsearch.action.index.IndexRequest;
Expand Down Expand Up @@ -67,28 +68,22 @@ public void testsShouldFail() throws Exception {
ensureGreen();
indexRandom(true, prepareIndex("test").setId("0").setSource("field1", 0, "field2", "toto"));
{
SearchPhaseExecutionException e = expectThrows(
SearchPhaseExecutionException.class,
ActionRequestValidationException e = expectThrows(
ActionRequestValidationException.class,
prepareSearch("test").addSort("field1", SortOrder.ASC)
.setQuery(matchAllQuery())
.searchAfter(new Object[] { 0 })
.setScroll("1m")
);
assertTrue(e.shardFailures().length > 0);
for (ShardSearchFailure failure : e.shardFailures()) {
assertThat(failure.toString(), containsString("`search_after` cannot be used in a scroll context."));
}
assertThat(e.getMessage(), containsString("[search_after] cannot be used in a scroll context"));
}

{
SearchPhaseExecutionException e = expectThrows(
SearchPhaseExecutionException.class,
ActionRequestValidationException e = expectThrows(
ActionRequestValidationException.class,
prepareSearch("test").addSort("field1", SortOrder.ASC).setQuery(matchAllQuery()).searchAfter(new Object[] { 0 }).setFrom(10)
);
assertTrue(e.shardFailures().length > 0);
for (ShardSearchFailure failure : e.shardFailures()) {
assertThat(failure.toString(), containsString("`from` parameter must be set to 0 when `search_after` is used."));
}
assertThat(e.getMessage(), containsString("[from] parameter must be set to 0 when [search_after] is used"));
}

{
Expand Down
11 changes: 5 additions & 6 deletions server/src/internalClusterTest/java/org/elasticsearch/search/slice/SearchSliceIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

package org.elasticsearch.search.slice;

import org.elasticsearch.action.ActionRequestValidationException;
import org.elasticsearch.action.admin.indices.alias.IndicesAliasesRequest;
import org.elasticsearch.action.index.IndexRequestBuilder;
import org.elasticsearch.action.search.ClosePointInTimeRequest;
Expand All @@ -22,7 +23,6 @@
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.search.Scroll;
import org.elasticsearch.search.SearchException;
import org.elasticsearch.search.SearchHit;
import org.elasticsearch.search.builder.PointInTimeBuilder;
import org.elasticsearch.search.sort.ShardDocSortField;
Expand All @@ -41,6 +41,7 @@
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertAcked;
import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertResponse;
import static org.elasticsearch.xcontent.XContentFactory.jsonBuilder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.startsWith;

Expand Down Expand Up @@ -283,13 +284,11 @@ public void testInvalidFields() throws Exception {

public void testInvalidQuery() throws Exception {
setupIndex(0, 1);
SearchPhaseExecutionException exc = expectThrows(
SearchPhaseExecutionException.class,
ActionRequestValidationException exc = expectThrows(
ActionRequestValidationException.class,
prepareSearch().setQuery(matchAllQuery()).slice(new SliceBuilder("invalid_random_int", 0, 10))
);
Throwable rootCause = findRootCause(exc);
assertThat(rootCause.getClass(), equalTo(SearchException.class));
assertThat(rootCause.getMessage(), equalTo("[slice] can only be used with [scroll] or [point-in-time] requests"));
assertThat(exc.getMessage(), containsString("[slice] can only be used with [scroll] or [point-in-time] requests"));
}

private Throwable findRootCause(Exception e) {
Expand Down
119 changes: 49 additions & 70 deletions server/src/main/java/org/elasticsearch/search/SearchService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import org.elasticsearch.action.ActionRunnable;
import org.elasticsearch.action.search.CanMatchNodeRequest;
import org.elasticsearch.action.search.CanMatchNodeResponse;
import org.elasticsearch.action.search.SearchRequest;
import org.elasticsearch.action.search.SearchShardTask;
import org.elasticsearch.action.search.SearchType;
import org.elasticsearch.action.support.TransportActions;
Expand All @@ -46,6 +47,7 @@
import org.elasticsearch.core.Releasable;
import org.elasticsearch.core.Releasables;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.core.UpdateForV9;
import org.elasticsearch.index.Index;
import org.elasticsearch.index.IndexNotFoundException;
import org.elasticsearch.index.IndexService;
Expand Down Expand Up @@ -1229,6 +1231,7 @@ private void parseSource(DefaultSearchContext context, SearchSourceBuilder sourc
if (source == null) {
return;
}
validateSearchSource(source, context.scrollContext() != null);
SearchShardTarget shardTarget = context.shardTarget();
SearchExecutionContext searchExecutionContext = context.getSearchExecutionContext();
context.from(source.from());
Expand Down Expand Up @@ -1270,17 +1273,6 @@ private void parseSource(DefaultSearchContext context, SearchSourceBuilder sourc
}
}
context.trackScores(source.trackScores());
if (source.trackTotalHitsUpTo() != null
&& source.trackTotalHitsUpTo() != SearchContext.TRACK_TOTAL_HITS_ACCURATE
&& context.scrollContext() != null) {
// this check is now performed in the coord node, data nodes still need to check it for bw comp.
// You could have a coord node in the previous version that does not perform the check, in which case the shard must check.
throw new SearchException(
shardTarget,
"disabling [track_total_hits] is not allowed in a scroll context",
new IllegalArgumentException("disabling [track_total_hits] is not allowed in a scroll context")
);
}
if (source.trackTotalHitsUpTo() != null) {
context.trackTotalHitsUpTo(source.trackTotalHitsUpTo());
}
Expand Down Expand Up @@ -1408,79 +1400,20 @@ private void parseSource(DefaultSearchContext context, SearchSourceBuilder sourc
context.groupStats(source.stats());
}
if (CollectionUtils.isEmpty(source.searchAfter()) == false) {
// these checks are now performed in the coord node, data nodes still need to check it for bw comp.
// You could have a coord node in the previous version that does not perform the check, in which case the shard must check.
if (context.scrollContext() != null) {
throw new SearchException(
shardTarget,
"`search_after` cannot be used in a scroll context.",
new IllegalArgumentException("`search_after` cannot be used in a scroll context.")
);
}
if (context.from() > 0) {
throw new SearchException(
shardTarget,
"`from` parameter must be set to 0 when `search_after` is used.",
new IllegalArgumentException("`from` parameter must be set to 0 when `search_after` is used.")
);
}
String collapseField = source.collapse() != null ? source.collapse().getField() : null;
FieldDoc fieldDoc = SearchAfterBuilder.buildFieldDoc(context.sort(), source.searchAfter(), collapseField);
context.searchAfter(fieldDoc);
}

if (source.slice() != null) {
if (source.pointInTimeBuilder() == null && context.scrollContext() == null) {
// this check is now performed in the coord node, data nodes still need to check it for bw comp.
// You could have a coord node in the previous version that does not perform the check, then the shard must check.
throw new SearchException(
shardTarget,
"[slice] can only be used with [scroll] or [point-in-time] requests",
new IllegalArgumentException("[slice] can only be used with [scroll] or [point-in-time] requests")
);
}
context.sliceBuilder(source.slice());
}

if (source.storedFields() != null) {
if (source.storedFields().fetchFields() == false) {
// these checks are now performed in the coord node, data nodes still need to check it for bw comp.
// You could have a coord node in the previous version that does not perform the check, in which case the shard must check.
if (context.sourceRequested()) {
throw new SearchException(
shardTarget,
"[stored_fields] cannot be disabled if [_source] is requested",
new IllegalArgumentException("[stored_fields] cannot be disabled if [_source] is requested")
);
}
if (context.fetchFieldsContext() != null) {
throw new SearchException(
shardTarget,
"[stored_fields] cannot be disabled when using the [fields] option",
new IllegalArgumentException("[stored_fields] cannot be disabled if [_source] is requested")
);
}
}
context.storedFieldsContext(source.storedFields());
}

if (source.collapse() != null) {
// these checks are now performed in the coord node, data nodes still need to check it for bw comp.
// You could have a coord node in the previous version that does not perform the check, in which case the shard must check.
if (context.scrollContext() != null) {
throw new SearchException(
shardTarget,
"cannot use `collapse` in a scroll context",
new IllegalArgumentException("cannot use `collapse` in a scroll context")
);
}
if (context.rescore() != null && context.rescore().isEmpty() == false) {
throw new SearchException(
shardTarget,
"cannot use `collapse` in conjunction with `rescore`",
new IllegalArgumentException("cannot use `collapse` in a scroll context")
);
}
final CollapseContext collapseContext = source.collapse().build(searchExecutionContext);
context.collapse(collapseContext);
}
Expand All @@ -1494,6 +1427,52 @@ private void parseSource(DefaultSearchContext context, SearchSourceBuilder sourc
}
}

/**
* Validates the incoming search request on the data node. These checks have been moved to the coordinating node.
* Validation is still performed on the data nodes to ensure that in a mixed cluster scenario, when the coordinating node is on an older
* version that does not yet perform validation, the shards make up for that.
* This method can be entirely removed in the next major version, when all nodes perform the validation when coordinating a search.
*
* @see SearchRequest#validate()
*/
@UpdateForV9
private static void validateSearchSource(SearchSourceBuilder source, boolean hasScroll) {
if (source.trackTotalHitsUpTo() != null && source.trackTotalHitsUpTo() != SearchContext.TRACK_TOTAL_HITS_ACCURATE && hasScroll) {
throw new IllegalArgumentException("disabling [track_total_hits] is not allowed in a scroll context");
}
if (CollectionUtils.isEmpty(source.searchAfter()) == false) {
if (hasScroll) {
throw new IllegalArgumentException("`search_after` cannot be used in a scroll context.");
}
if (source.from() > 0) {
throw new IllegalArgumentException("`from` parameter must be set to 0 when `search_after` is used.");
}
}
if (source.collapse() != null) {
if (hasScroll) {
throw new IllegalArgumentException("cannot use `collapse` in a scroll context");
}
if (source.rescores() != null && source.rescores().isEmpty() == false) {
throw new IllegalArgumentException("cannot use `collapse` in conjunction with `rescore`");
}
}
if (source.slice() != null) {
if (source.pointInTimeBuilder() == null && (hasScroll == false)) {
throw new IllegalArgumentException("[slice] can only be used with [scroll] or [point-in-time] requests");
}
}
if (source.storedFields() != null) {
if (source.storedFields().fetchFields() == false) {
if (source.fetchSource() != null && source.fetchSource().fetchSource()) {
throw new IllegalArgumentException("[stored_fields] cannot be disabled if [_source] is requested");
}
if (source.fetchFields() != null) {
throw new IllegalArgumentException("[stored_fields] cannot be disabled when using the [fields] option");
}
}
}
}

/**
* Shortcut ids to load, we load only "from" and up to "size". The phase controller
* handles this as well since the result is always size * shards for Q_T_F
Expand Down
Loading

0 comments on commit 1b037ae

Please sign in to comment.