Skip to content

Add remaining BWC tests for the ent-search module #98016

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

Merged
merged 7 commits into from
Jul 31, 2023
Merged

Add remaining BWC tests for the ent-search module #98016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
93f13bd
Add serialization to ListSearchApplicationAction.Response
kderusso Jul 26, 2023
4d14567
Convert tests to BWC
kderusso Jul 26, 2023
126140e
Add toString to PostAnalyticsEventAction
kderusso Jul 26, 2023
34fbb5f
Fixed post analytics action request test
kderusso Jul 27, 2023
aded2a0
Cleanup
kderusso Jul 27, 2023
850f3a3
Spotless
kderusso Jul 27, 2023
44e1b2d
PR feedback
kderusso Jul 28, 2023
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
36 changes: 33 additions & 3 deletions ...n/java/org/elasticsearch/xpack/application/analytics/action/PostAnalyticsEventAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.xcontent.ConstructingObjectParser;
import org.elasticsearch.xcontent.ParseField;
import org.elasticsearch.xcontent.ToXContentObject;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.XContentParser;
import org.elasticsearch.xcontent.XContentType;
Expand All @@ -47,7 +48,7 @@ private PostAnalyticsEventAction() {
super(NAME, Response::readFromStreamInput);
}

public static class Request extends ActionRequest implements AnalyticsEvent.Context {
public static class Request extends ActionRequest implements AnalyticsEvent.Context, ToXContentObject {

private final String eventCollectionName;

Expand Down Expand Up @@ -199,15 +200,44 @@ public boolean equals(Object o) {
&& debug == that.debug
&& eventTime == that.eventTime
&& Objects.equals(eventType, that.eventType)
&& Objects.equals(xContentType, that.xContentType)
&& Objects.equals(xContentType.canonical(), that.xContentType.canonical())
Copy link
Member Author

@kderusso kderusso Jul 27, 2023

Choose a reason for hiding this comment

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

The canonical() calls were added here to ensure that versioned XContentTypes still act as equivalent in BWC tests.

&& Objects.equals(payload, that.payload)
&& Objects.equals(headers, that.headers)
&& Objects.equals(clientAddress, that.clientAddress);
}

@Override
public int hashCode() {
return Objects.hash(eventCollectionName, eventType, debug, eventTime, xContentType, payload, headers, clientAddress);
return Objects.hash(
eventCollectionName,
eventType,
debug,
eventTime,
xContentType.canonical(),
payload,
headers,
clientAddress
);
}

@Override
public String toString() {
return Strings.toString(this);
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
builder.field("event_collection_name", eventCollectionName);
builder.field("debug", debug);
builder.field("event_time", eventTime);
builder.field("event_type", eventType);
builder.field("x_content_type", xContentType);
builder.field("payload", payload);
builder.field("headers", headers);
builder.field("client_address", clientAddress);
builder.endObject();
return builder;
}
}

Expand Down
30 changes: 30 additions & 0 deletions ...h/src/main/java/org/elasticsearch/xpack/application/search/SearchApplicationListItem.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,20 @@
import org.elasticsearch.common.io.stream.StreamOutput;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.core.Nullable;
import org.elasticsearch.xcontent.ConstructingObjectParser;
import org.elasticsearch.xcontent.ParseField;
import org.elasticsearch.xcontent.ToXContentObject;
import org.elasticsearch.xcontent.XContentBuilder;
import org.elasticsearch.xcontent.XContentParser;

import java.io.IOException;
import java.util.Arrays;
import java.util.List;
import java.util.Objects;

import static org.elasticsearch.xcontent.ConstructingObjectParser.constructorArg;
import static org.elasticsearch.xcontent.ConstructingObjectParser.optionalConstructorArg;

/**
* This class is used for returning information for lists of search applications, to avoid including all
* {@link SearchApplication} information which can be retrieved using subsequent Get Search Application requests.
Expand Down Expand Up @@ -75,6 +81,30 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
return builder;
}

private static final ConstructingObjectParser<SearchApplicationListItem, String> PARSER = new ConstructingObjectParser<>(
"search_application_list_item`",
false,
(params) -> {
final String name = (String) params[0];
@SuppressWarnings("unchecked")
final String[] indices = ((List<String>) params[1]).toArray(String[]::new);
final String analyticsCollectionName = (String) params[2];
final Long updatedAtMillis = (Long) params[3];
return new SearchApplicationListItem(name, indices, analyticsCollectionName, updatedAtMillis);
}
);

static {
PARSER.declareStringOrNull(optionalConstructorArg(), NAME_FIELD);
PARSER.declareStringArray(constructorArg(), INDICES_FIELD);
Copy link
Contributor

@ioanatia ioanatia Jul 28, 2023

Choose a reason for hiding this comment

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

just a note for both of us - this will not be compatible with #98036 since I removed the indices from the list search apps API - so whichever PR gets merged second, we need to make sure we get the latest changes

Copy link
Member Author

@kderusso kderusso Jul 28, 2023

Choose a reason for hiding this comment

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

That's a really good observation. I actually think it might be best to merge this one first, so we can ensure that BWC works after removing the indices. WDYT?

PARSER.declareStringOrNull(optionalConstructorArg(), ANALYTICS_COLLECTION_NAME_FIELD);
PARSER.declareLong(optionalConstructorArg(), UPDATED_AT_MILLIS_FIELD);
}

public SearchApplicationListItem fromXContent(XContentParser parser) {
return PARSER.apply(parser, null);
}

@Override
public void writeTo(StreamOutput out) throws IOException {
out.writeString(name);
Expand Down
14 changes: 12 additions & 2 deletions ...csCollectionResponseSerializingTests.java → ...ollectionResponseBWCSerializingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,16 @@

package org.elasticsearch.xpack.application.analytics.action;

import org.elasticsearch.TransportVersion;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.test.AbstractWireSerializingTestCase;
import org.elasticsearch.xpack.application.analytics.AnalyticsCollection;
import org.elasticsearch.xpack.core.ml.AbstractBWCWireSerializationTestCase;

import java.io.IOException;
import java.util.List;

public class GetAnalyticsCollectionResponseSerializingTests extends AbstractWireSerializingTestCase<GetAnalyticsCollectionAction.Response> {
public class GetAnalyticsCollectionResponseBWCSerializingTests extends AbstractBWCWireSerializationTestCase<
GetAnalyticsCollectionAction.Response> {

@Override
protected Writeable.Reader<GetAnalyticsCollectionAction.Response> instanceReader() {
Expand All @@ -31,4 +33,12 @@ protected GetAnalyticsCollectionAction.Response createTestInstance() {
protected GetAnalyticsCollectionAction.Response mutateInstance(GetAnalyticsCollectionAction.Response instance) throws IOException {
return randomValueOtherThan(instance, this::createTestInstance);
}

@Override
protected GetAnalyticsCollectionAction.Response mutateInstanceForVersion(
GetAnalyticsCollectionAction.Response instance,
TransportVersion version
) {
return instance;
}
}
14 changes: 12 additions & 2 deletions ...csEventDebugResponseSerializingTests.java → ...ventDebugResponseBWCSerializingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,16 @@

package org.elasticsearch.xpack.application.analytics.action;

import org.elasticsearch.TransportVersion;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.test.AbstractWireSerializingTestCase;
import org.elasticsearch.xpack.core.ml.AbstractBWCWireSerializationTestCase;

import java.io.IOException;

import static org.elasticsearch.xpack.application.analytics.event.AnalyticsEventTestUtils.randomAnalyticsEvent;

public class PostAnalyticsEventDebugResponseSerializingTests extends AbstractWireSerializingTestCase<PostAnalyticsEventAction.Response> {
public class PostAnalyticsEventDebugResponseBWCSerializingTests extends AbstractBWCWireSerializationTestCase<
PostAnalyticsEventAction.Response> {

@Override
protected Writeable.Reader<PostAnalyticsEventAction.Response> instanceReader() {
Expand All @@ -30,4 +32,12 @@ protected PostAnalyticsEventAction.Response createTestInstance() {
protected PostAnalyticsEventAction.Response mutateInstance(PostAnalyticsEventAction.Response instance) throws IOException {
return randomValueOtherThan(instance, this::createTestInstance);
}

@Override
protected PostAnalyticsEventAction.Response mutateInstanceForVersion(
PostAnalyticsEventAction.Response instance,
TransportVersion version
) {
return instance;
}
}
13 changes: 11 additions & 2 deletions ...nalyticsEventRequestSerializingTests.java → ...yticsEventRequestBWCSerializingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,15 @@

package org.elasticsearch.xpack.application.analytics.action;

import org.elasticsearch.TransportVersion;
import org.elasticsearch.common.ValidationException;
import org.elasticsearch.common.bytes.BytesArray;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.core.Tuple;
import org.elasticsearch.test.AbstractWireSerializingTestCase;
import org.elasticsearch.xcontent.XContentType;
import org.elasticsearch.xpack.application.analytics.event.AnalyticsEvent;
import org.elasticsearch.xpack.core.ml.AbstractBWCWireSerializationTestCase;

import java.io.IOException;
import java.util.Collections;
Expand All @@ -25,7 +26,7 @@
import static org.elasticsearch.xpack.application.analytics.event.AnalyticsEventTestUtils.randomInetAddress;
import static org.mockito.Mockito.mock;

public class PostAnalyticsEventRequestSerializingTests extends AbstractWireSerializingTestCase<PostAnalyticsEventAction.Request> {
public class PostAnalyticsEventRequestBWCSerializingTests extends AbstractBWCWireSerializationTestCase<PostAnalyticsEventAction.Request> {

public void testValidate() {
assertNull(createTestInstance().validate());
Expand Down Expand Up @@ -83,4 +84,12 @@ protected PostAnalyticsEventAction.Request createTestInstance() {
new BytesArray(randomByteArrayOfLength(20))
).eventTime(randomLong()).debug(randomBoolean()).headers(randomHeaders()).clientAddress(randomInetAddress()).request();
}

@Override
protected PostAnalyticsEventAction.Request mutateInstanceForVersion(
PostAnalyticsEventAction.Request instance,
TransportVersion version
) {
return instance;
}
}
14 changes: 12 additions & 2 deletions ...lesetsActionResponseSerializingTests.java → ...etsActionResponseBWCSerializingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@

package org.elasticsearch.xpack.application.rules.action;

import org.elasticsearch.TransportVersion;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.test.AbstractWireSerializingTestCase;
import org.elasticsearch.xpack.application.rules.QueryRuleset;
import org.elasticsearch.xpack.application.rules.QueryRulesetListItem;
import org.elasticsearch.xpack.application.search.SearchApplicationTestUtils;
import org.elasticsearch.xpack.core.ml.AbstractBWCWireSerializationTestCase;

public class ListQueryRulesetsActionResponseSerializingTests extends AbstractWireSerializingTestCase<ListQueryRulesetsAction.Response> {
public class ListQueryRulesetsActionResponseBWCSerializingTests extends AbstractBWCWireSerializationTestCase<
ListQueryRulesetsAction.Response> {

@Override
protected Writeable.Reader<ListQueryRulesetsAction.Response> instanceReader() {
Expand All @@ -36,4 +38,12 @@ protected ListQueryRulesetsAction.Response mutateInstance(ListQueryRulesetsActio
protected ListQueryRulesetsAction.Response createTestInstance() {
return randomQueryRulesetListItem();
}

@Override
protected ListQueryRulesetsAction.Response mutateInstanceForVersion(
ListQueryRulesetsAction.Response instance,
TransportVersion version
) {
return instance;
}
}
10 changes: 8 additions & 2 deletions ...csearch/xpack/application/rules/action/PutQueryRulesetActionResponseSerializingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,14 @@

package org.elasticsearch.xpack.application.rules.action;

import org.elasticsearch.TransportVersion;
import org.elasticsearch.action.DocWriteResponse;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.test.AbstractWireSerializingTestCase;
import org.elasticsearch.xpack.core.ml.AbstractBWCWireSerializationTestCase;

import java.io.IOException;

public class PutQueryRulesetActionResponseSerializingTests extends AbstractWireSerializingTestCase<PutQueryRulesetAction.Response> {
public class PutQueryRulesetActionResponseSerializingTests extends AbstractBWCWireSerializationTestCase<PutQueryRulesetAction.Response> {

@Override
protected Writeable.Reader<PutQueryRulesetAction.Response> instanceReader() {
Expand All @@ -29,4 +30,9 @@ protected PutQueryRulesetAction.Response createTestInstance() {
protected PutQueryRulesetAction.Response mutateInstance(PutQueryRulesetAction.Response instance) throws IOException {
return randomValueOtherThan(instance, this::createTestInstance);
}

@Override
protected PutQueryRulesetAction.Response mutateInstanceForVersion(PutQueryRulesetAction.Response instance, TransportVersion version) {
return instance;
}
}
2 changes: 1 addition & 1 deletion ...ck/application/search/action/DeleteSearchApplicationActionRequestBWCSerializingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,6 @@ protected DeleteSearchApplicationAction.Request mutateInstanceForVersion(
DeleteSearchApplicationAction.Request instance,
TransportVersion version
) {
return new DeleteSearchApplicationAction.Request(instance.getName());
return instance;
}
}
2 changes: 1 addition & 1 deletion ...xpack/application/search/action/GetSearchApplicationActionRequestBWCSerializingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,6 @@ protected GetSearchApplicationAction.Request mutateInstanceForVersion(
GetSearchApplicationAction.Request instance,
TransportVersion version
) {
return new GetSearchApplicationAction.Request(instance.getName());
return instance;
}
}
2 changes: 1 addition & 1 deletion ...pack/application/search/action/GetSearchApplicationActionResponseBWCSerializingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,6 @@ protected GetSearchApplicationAction.Response mutateInstanceForVersion(
GetSearchApplicationAction.Response instance,
TransportVersion version
) {
return new GetSearchApplicationAction.Response(instance.searchApp());
return instance;
}
}
2 changes: 1 addition & 1 deletion ...pack/application/search/action/ListSearchApplicationActionRequestBWCSerializingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,6 @@ protected ListSearchApplicationAction.Request mutateInstanceForVersion(
ListSearchApplicationAction.Request instance,
TransportVersion version
) {
return new ListSearchApplicationAction.Request(instance.query(), instance.pageParams());
return instance;
}
}
13 changes: 11 additions & 2 deletions ...cationActionResponseSerializingTests.java → ...ionActionResponseBWCSerializingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,14 @@

package org.elasticsearch.xpack.application.search.action;

import org.elasticsearch.TransportVersion;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.test.AbstractWireSerializingTestCase;
import org.elasticsearch.xpack.application.search.SearchApplication;
import org.elasticsearch.xpack.application.search.SearchApplicationListItem;
import org.elasticsearch.xpack.application.search.SearchApplicationTestUtils;
import org.elasticsearch.xpack.core.ml.AbstractBWCWireSerializationTestCase;

public class ListSearchApplicationActionResponseSerializingTests extends AbstractWireSerializingTestCase<
public class ListSearchApplicationActionResponseBWCSerializingTests extends AbstractBWCWireSerializationTestCase<
ListSearchApplicationAction.Response> {

@Override
Expand All @@ -37,4 +38,12 @@ protected ListSearchApplicationAction.Response mutateInstance(ListSearchApplicat
protected ListSearchApplicationAction.Response createTestInstance() {
return randomSearchApplicationListItem();
}

@Override
protected ListSearchApplicationAction.Response mutateInstanceForVersion(
ListSearchApplicationAction.Response instance,
TransportVersion version
) {
return instance;
}
}
2 changes: 1 addition & 1 deletion ...xpack/application/search/action/PutSearchApplicationActionRequestBWCSerializingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,6 @@ protected PutSearchApplicationAction.Request mutateInstanceForVersion(
PutSearchApplicationAction.Request instance,
TransportVersion version
) {
return new PutSearchApplicationAction.Request(instance.getSearchApplication(), instance.create());
return instance;
}
}
13 changes: 11 additions & 2 deletions ...cationActionResponseSerializingTests.java → ...ionActionResponseBWCSerializingTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,14 @@

package org.elasticsearch.xpack.application.search.action;

import org.elasticsearch.TransportVersion;
import org.elasticsearch.action.DocWriteResponse;
import org.elasticsearch.common.io.stream.Writeable;
import org.elasticsearch.test.AbstractWireSerializingTestCase;
import org.elasticsearch.xpack.core.ml.AbstractBWCWireSerializationTestCase;

import java.io.IOException;

public class PutSearchApplicationActionResponseSerializingTests extends AbstractWireSerializingTestCase<
public class PutSearchApplicationActionResponseBWCSerializingTests extends AbstractBWCWireSerializationTestCase<
PutSearchApplicationAction.Response> {

@Override
Expand All @@ -30,4 +31,12 @@ protected PutSearchApplicationAction.Response createTestInstance() {
protected PutSearchApplicationAction.Response mutateInstance(PutSearchApplicationAction.Response instance) throws IOException {
return randomValueOtherThan(instance, this::createTestInstance);
}

@Override
protected PutSearchApplicationAction.Response mutateInstanceForVersion(
Copy link
Contributor

@ioanatia ioanatia Jul 28, 2023

Choose a reason for hiding this comment

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

we have to implement this because we are now inheriting from AbstractBWCWireSerializationTestCase.

this is not related to your PR, but looking at the other (~70) implementation of this method most of them are the same where we just do return instance.

Is there a reason why this is an abstract method?

elasticsearch/x-pack/plugin/eql/src/test/java/org/elasticsearch/xpack/eql/AbstractBWCSerializationTestCase.java

Line 30 in a2d4799

protected abstract T mutateInstanceForVersion(T instance, TransportVersion version);

Copy link
Member Author

@kderusso kderusso Jul 28, 2023

Choose a reason for hiding this comment

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

That's a really good question.

My guess is that the assumption was, these tests wouldn't be written unless we had backwards compatibility changes to address. However, if we're going to be proactively including them (which I think is a good idea) then it would make sense to have a sane default to return the instance.

Copy link
Member

@carlosdelest carlosdelest Jul 31, 2023

Choose a reason for hiding this comment

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

I guess the intention is to make us think about that. If we're doing a BWC test, then something could have changed between version serializations. So probably it's let's not forget?

Copy link
Contributor

@ioanatia ioanatia Jul 31, 2023

Choose a reason for hiding this comment

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

(@kderusso - sorry again, I know this is not related to your PR and it's not a change I propose doing here)
I don't know if there's a clear intention behind making this method abstract or if all implementations just follow a pattern that's already in place.
This method was added as part of #69534 by @astefan.

@astefan - do you think it would make sense to provide a sane default here instead of having this be an abstract method and forcing an implementation in each subclass?

PutSearchApplicationAction.Response instance,
TransportVersion version
) {
return instance;
}
}