Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correct search_after handling #50629

Merged
merged 3 commits into from
Jan 6, 2020
Merged

Correct search_after handling #50629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0cbf00a
Correct search_after handling
aleksmaus Jan 4, 2020
32eeae1
Address Igor's code review suggestions
aleksmaus Jan 6, 2020
1bd4327
Update EqlSearchRequest unit test for null searchAfterBuilder cases
aleksmaus Jan 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 23 additions & 38 deletions x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/action/EqlSearchRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,10 @@
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.index.query.AbstractQueryBuilder;
import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.search.searchafter.SearchAfterBuilder;

import java.io.IOException;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.Objects;
import java.util.function.Supplier;

Expand All @@ -40,7 +39,7 @@ public class EqlSearchRequest extends ActionRequest implements IndicesRequest.Re
private String eventTypeField = "event.category";
private String implicitJoinKeyField = "agent.id";
private int fetchSize = 50;
private List<String> searchAfter = Collections.emptyList();
private SearchAfterBuilder searchAfterBuilder;
private String rule;

static final String KEY_QUERY = "query";
Expand Down Expand Up @@ -74,23 +73,10 @@ public EqlSearchRequest(StreamInput in) throws IOException {
eventTypeField = in.readString();
implicitJoinKeyField = in.readString();
fetchSize = in.readVInt();
searchAfter = in.readList(StreamInput::readString);
searchAfterBuilder = in.readOptionalWriteable(SearchAfterBuilder::new);
rule = in.readString();
}

public EqlSearchRequest(String[] indices, QueryBuilder query,
String timestampField, String eventTypeField, String implicitJoinKeyField,
int fetchSize, List<String> searchAfter, String rule) {
this.indices = indices;
this.query = query;
this.timestampField = timestampField;
this.eventTypeField = eventTypeField;
this.implicitJoinKeyField = implicitJoinKeyField;
this.fetchSize = fetchSize;
this.searchAfter = searchAfter;
this.rule = rule;
}

@Override
public ActionRequestValidationException validate() {
ActionRequestValidationException validationException = null;
Expand Down Expand Up @@ -122,10 +108,6 @@ public ActionRequestValidationException validate() {
validationException = addValidationError("size must be more than 0.", validationException);
}

if (searchAfter == null) {
validationException = addValidationError("search after is null", validationException);
}

return validationException;
}

Expand All @@ -141,13 +123,10 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
}
builder.field(KEY_SIZE, fetchSize());

if (this.searchAfter != null && !this.searchAfter.isEmpty()) {
builder.startArray(KEY_SEARCH_AFTER);
for (String val : this.searchAfter) {
builder.value(val);
}
builder.endArray();
if (searchAfterBuilder != null) {
builder.array(SEARCH_AFTER.getPreferredName(), searchAfterBuilder.getSortValues());
}

builder.field(KEY_RULE, rule);

return builder;
Expand All @@ -165,11 +144,13 @@ protected static <R extends EqlSearchRequest> ObjectParser<R, Void> objectParser
parser.declareString(EqlSearchRequest::eventTypeField, EVENT_TYPE_FIELD);
parser.declareString(EqlSearchRequest::implicitJoinKeyField, IMPLICIT_JOIN_KEY_FIELD);
parser.declareInt(EqlSearchRequest::fetchSize, SIZE);
parser.declareStringArray(EqlSearchRequest::searchAfter, SEARCH_AFTER);
parser.declareField(EqlSearchRequest::setSearchAfter, SearchAfterBuilder::fromXContent, SEARCH_AFTER,
ObjectParser.ValueType.OBJECT_ARRAY);
parser.declareString(EqlSearchRequest::rule, RULE);
return parser;
}

@Override
public EqlSearchRequest indices(String... indices) {
this.indices = indices;
return this;
Expand Down Expand Up @@ -219,22 +200,26 @@ public EqlSearchRequest fetchSize(int size) {
return this;
}

public List<String> searchAfter() {
return searchAfter;
public Object[] searchAfter() {
if (searchAfterBuilder == null) {
return null;
}
return searchAfterBuilder.getSortValues();
}

public EqlSearchRequest searchAfter(List<String> searchAfter) {
if (searchAfter != null && !searchAfter.isEmpty()) {
this.searchAfter = searchAfter;
}
public EqlSearchRequest searchAfter(Object[] values) {
this.searchAfterBuilder = new SearchAfterBuilder().setSortValues(values);
return this;
}

private EqlSearchRequest setSearchAfter(SearchAfterBuilder builder) {
this.searchAfterBuilder = builder;
return this;
}

public String rule() { return this.rule; }

public EqlSearchRequest rule(String rule) {
// TODO: possibly attempt to parse the rule here
this.rule = rule;
return this;
}
Expand All @@ -249,7 +234,7 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeString(eventTypeField);
out.writeString(implicitJoinKeyField);
out.writeVInt(fetchSize);
out.writeStringCollection(searchAfter);
out.writeOptionalWriteable(searchAfterBuilder);
out.writeString(rule);
}

Expand All @@ -270,7 +255,7 @@ public boolean equals(Object o) {
Objects.equals(timestampField, that.timestampField) &&
Objects.equals(eventTypeField, that.eventTypeField) &&
Objects.equals(implicitJoinKeyField, that.implicitJoinKeyField) &&
Objects.equals(searchAfter, that.searchAfter) &&
Objects.equals(searchAfterBuilder, that.searchAfterBuilder) &&
Objects.equals(rule, that.rule);
}

Expand All @@ -284,7 +269,7 @@ public int hashCode() {
timestampField,
eventTypeField,
implicitJoinKeyField,
searchAfter,
searchAfterBuilder,
rule);
}

Expand Down
9 changes: 4 additions & 5 deletions x-pack/plugin/eql/src/main/java/org/elasticsearch/xpack/eql/plugin/RestEqlSearchAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,11 @@
import static org.elasticsearch.rest.RestRequest.Method.POST;

public class RestEqlSearchAction extends BaseRestHandler {
public RestEqlSearchAction(RestController controller) {
// Not sure yet if we will always have index in the path or not
// TODO: finalize the endpoints
controller.registerHandler(GET, "/{index}/_eql/search", this);
controller.registerHandler(POST, "/{index}/_eql/search", this);
private static final String SEARCH_PATH = "/{index}/_eql/search";

public RestEqlSearchAction(RestController controller) {
controller.registerHandler(GET, SEARCH_PATH, this);
controller.registerHandler(POST, SEARCH_PATH, this);
}

@Override
Expand Down
6 changes: 2 additions & 4 deletions ...ck/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/action/EqlRequestParserTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
import org.elasticsearch.test.ESTestCase;

import java.io.IOException;
import java.util.Arrays;
import java.util.List;
import java.util.function.Consumer;
import java.util.function.Function;
Expand Down Expand Up @@ -54,7 +53,7 @@ public void testSearchRequestParser() throws IOException {
+ "\"timestamp_field\" : \"tsf\", "
+ "\"event_type_field\" : \"etf\","
+ "\"implicit_join_key_field\" : \"imjf\","
+ "\"search_after\" : [ \"device-20184\", \"/user/local/foo.exe\", \"2019-11-26T00:45:43.542\" ],"
+ "\"search_after\" : [ 12345678, \"device-20184\", \"/user/local/foo.exe\", \"2019-11-26T00:45:43.542\" ],"
+ "\"size\" : \"101\","
+ "\"rule\" : \"file where user != 'SYSTEM' by file_path\""
+ "}", EqlSearchRequest::fromXContent);
Expand All @@ -67,8 +66,7 @@ public void testSearchRequestParser() throws IOException {
assertEquals("tsf", request.timestampField());
assertEquals("etf", request.eventTypeField());
assertEquals("imjf", request.implicitJoinKeyField());
assertArrayEquals(Arrays.asList("device-20184", "/user/local/foo.exe", "2019-11-26T00:45:43.542").toArray(),
request.searchAfter().toArray());
assertArrayEquals(new Object[]{12345678, "device-20184", "/user/local/foo.exe", "2019-11-26T00:45:43.542"}, request.searchAfter());
assertEquals(101, request.fetchSize());
assertEquals("file where user != 'SYSTEM' by file_path", request.rule());
}
Expand Down
70 changes: 53 additions & 17 deletions ...ck/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/action/EqlSearchRequestTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,21 +5,27 @@
*/
package org.elasticsearch.xpack.eql.action;

import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.io.stream.NamedWriteableRegistry;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.text.Text;
import org.elasticsearch.common.xcontent.NamedXContentRegistry;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.json.JsonXContent;
import org.elasticsearch.index.query.QueryBuilder;
import org.elasticsearch.search.SearchModule;
import org.elasticsearch.search.searchafter.SearchAfterBuilder;
import org.elasticsearch.test.AbstractSerializingTestCase;
import org.elasticsearch.test.ESTestCase;
import org.junit.Before;

import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import java.util.function.Supplier;

import static org.elasticsearch.index.query.AbstractQueryBuilder.parseInnerQueryBuilder;

Expand Down Expand Up @@ -52,15 +58,25 @@ protected NamedXContentRegistry xContentRegistry() {

@Override
protected EqlSearchRequest createTestInstance() {
QueryBuilder query = null;
try {
query = parseQuery(defaultTestQuery);
QueryBuilder query = parseQuery(defaultTestQuery);
EqlSearchRequest request = new EqlSearchRequest()
.indices(new String[]{defaultTestIndex})
.query(query)
.timestampField(randomAlphaOfLength(10))
.eventTypeField(randomAlphaOfLength(10))
.implicitJoinKeyField(randomAlphaOfLength(10))
.fetchSize(randomIntBetween(1, 50))
.rule(randomAlphaOfLength(10));

if (randomBoolean()) {
request.searchAfter(randomJsonSearchFromBuilder());
}
return request;
} catch (IOException ex) {
assertNotNull("unexpected IOException " + ex.getCause().getMessage(), ex);
}
return new EqlSearchRequest(new String[]{defaultTestIndex}, query,
randomAlphaOfLength(10), randomAlphaOfLength(10), randomAlphaOfLength(10),
randomIntBetween(1, 50), randomSearchAfter(), randomAlphaOfLength(10));
return null;
}

protected QueryBuilder parseQuery(String queryAsString) throws IOException {
Expand All @@ -74,16 +90,36 @@ protected QueryBuilder parseQuery(XContentParser parser) throws IOException {
return parseInnerQueryBuilder;
}

private List<String> randomSearchAfter() {
if (randomBoolean()) {
return Collections.emptyList();
} else {
int size = randomIntBetween(1, 50);
List<String> arr = new ArrayList<>(size);
for (int i = 0; i < size; i++) {
arr.add(randomAlphaOfLength(randomIntBetween(1, 15)));
}
return Collections.unmodifiableList(arr);
private Object randomValue() {
Supplier<Object> value = randomFrom(Arrays.asList(
ESTestCase::randomInt,
ESTestCase::randomFloat,
ESTestCase::randomLong,
ESTestCase::randomDouble,
() -> randomAlphaOfLengthBetween(5, 20),
ESTestCase::randomBoolean,
ESTestCase::randomByte,
ESTestCase::randomShort,
() -> new Text(randomAlphaOfLengthBetween(5, 20)),
() -> null));
return value.get();
}

private Object[] randomJsonSearchFromBuilder() throws IOException {
int numSearchAfter = randomIntBetween(1, 10);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about an empty list?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

normally SearchAfter doesn't accept the empty list, throws error:

    java.lang.IllegalArgumentException: Values must contains at least one value.

The original searchafter test doesn't use empty list as well

elasticsearch/server/src/test/java/org/elasticsearch/search/searchafter/SearchAfterBuilderTests.java

Line 101 in df83eb9

int numSearchAfter = randomIntBetween(1, 10);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about returning setting search after values to null sometimes then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Already handled there as well

    java.lang.NullPointerException: Values cannot be null.
        at __randomizedtesting.SeedInfo.seed([1C63F70E75AEE77B:48BB964652FCDAE7]:0)
        at org.elasticsearch.search.searchafter.SearchAfterBuilder.setSortValues(SearchAfterBuilder.java:78)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

discussed over slack, updated implementation

XContentBuilder jsonBuilder = XContentFactory.jsonBuilder();
jsonBuilder.startObject();
jsonBuilder.startArray("search_after");
for (int i = 0; i < numSearchAfter; i++) {
jsonBuilder.value(randomValue());
}
jsonBuilder.endArray();
jsonBuilder.endObject();
try (XContentParser parser = createParser(JsonXContent.jsonXContent, BytesReference.bytes(jsonBuilder))) {
parser.nextToken();
parser.nextToken();
parser.nextToken();
return SearchAfterBuilder.fromXContent(parser).getSortValues();
}
}

Expand Down