Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
* Added APIs and components to implement running scheduled experiments ([#220](https://github.com/opensearch-project/search-relevance/pull/220))

### Enhancements
* Support for adding description in Search Configuration ([#293](https://github.com/opensearch-project/search-relevance/pull/293))

### Bug Fixes

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,8 @@ private SearchConfiguration convertToSearchConfiguration(SearchResponse response
(String) source.get(SearchConfiguration.TIME_STAMP),
(String) source.get(SearchConfiguration.INDEX),
(String) source.get(SearchConfiguration.QUERY),
(String) source.get(SearchConfiguration.SEARCH_PIPELINE)
(String) source.get(SearchConfiguration.SEARCH_PIPELINE),
(String) source.get(SearchConfiguration.DESCRIPTION)
);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ public class ExperimentRunningManager {
/**
* Starts the experiment by setting up cancellation callback for the cancellation token and also retrieves the queryset.
*
* One thing to be careful about is that only at most scheduled run for a given scheduled experiment run id
* One thing to be careful about is that only at most scheduled run for a given scheduled experiment run id
* should be in the system at all times.
* @param experimentId - the id of the experiment to be run
* @param request - required parameters for placing a request to start an experiment
Expand Down Expand Up @@ -329,7 +329,8 @@ private SearchConfiguration convertToSearchConfiguration(SearchResponse response
(String) source.get("timestamp"),
(String) source.get("index"),
(String) source.get("query"),
(String) source.get("searchPipeline")
(String) source.get("searchPipeline"),
(String) source.get("description")
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

we have a weird mix of using constants and strings through this code base. Not asking you to change this here, since it follows the pattern, more noting it as a potential refactoring opportunity in the future!

);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
public class SearchConfiguration implements ToXContentObject {
public static final String ID = "id";
public static final String NAME = "name";
public static final String DESCRIPTION = "description";
public static final String TIME_STAMP = "timestamp";
public static final String INDEX = "index";

Expand All @@ -29,18 +30,28 @@ public class SearchConfiguration implements ToXContentObject {
*/
private final String id;
private final String name;
private final String description;
private final String timestamp;
private final String index;
private final String query;
private final String searchPipeline;

public SearchConfiguration(String id, String name, String timestamp, String index, String query, String searchPipeline) {
public SearchConfiguration(
String id,
String name,
String timestamp,
String index,
String query,
String searchPipeline,
String description
) {
this.id = id;
this.name = name;
this.timestamp = timestamp;
this.index = index;
this.query = query;
this.searchPipeline = searchPipeline;
this.description = description;
}

@Override
Expand All @@ -52,6 +63,7 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
xContentBuilder.field(INDEX, this.index.trim());
xContentBuilder.field(QUERY, this.query.trim());
xContentBuilder.field(SEARCH_PIPELINE, this.searchPipeline == null ? "" : this.searchPipeline.trim());
xContentBuilder.field(DESCRIPTION, this.description == null ? "" : this.description.trim());
return xContentBuilder.endObject();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@

import static java.util.Collections.singletonList;
import static org.opensearch.rest.RestRequest.Method.PUT;
import static org.opensearch.searchrelevance.common.PluginConstants.DESCRIPTION;
import static org.opensearch.searchrelevance.common.PluginConstants.INDEX;
import static org.opensearch.searchrelevance.common.PluginConstants.NAME;
import static org.opensearch.searchrelevance.common.PluginConstants.QUERY;
Expand Down Expand Up @@ -72,11 +73,28 @@ protected RestChannelConsumer prepareRequest(RestRequest request, NodeClient cli
new BytesRestResponse(RestStatus.BAD_REQUEST, "Invalid name: " + nameValidation.getErrorMessage())
);
}

String description = (String) source.get(DESCRIPTION);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Here is an example where we swapped back to a constant ;-(.

if (description != null) {
TextValidationUtil.ValidationResult descriptionValidation = TextValidationUtil.validateDescription(description);
if (!descriptionValidation.isValid()) {
return channel -> channel.sendResponse(
new BytesRestResponse(RestStatus.BAD_REQUEST, "Invalid description: " + descriptionValidation.getErrorMessage())
);
}
}

String index = (String) source.get(INDEX);
String queryBody = (String) source.get(QUERY);
String searchPipeline = (String) source.getOrDefault(SEARCH_PIPELINE, "");

PutSearchConfigurationRequest createRequest = new PutSearchConfigurationRequest(name, index, queryBody, searchPipeline);
PutSearchConfigurationRequest createRequest = new PutSearchConfigurationRequest(
name,
index,
queryBody,
searchPipeline,
description
);

return channel -> client.execute(PutSearchConfigurationAction.INSTANCE, createRequest, new ActionListener<IndexResponse>() {
@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,14 @@ public class PutSearchConfigurationRequest extends ActionRequest {
private final String index;
private final String queryBody;
private final String searchPipeline;
private final String description;

public PutSearchConfigurationRequest(String name, String index, String queryBody, String searchPipeline) {
public PutSearchConfigurationRequest(String name, String index, String queryBody, String searchPipeline, String description) {
this.name = name;
this.index = index;
this.queryBody = queryBody;
this.searchPipeline = searchPipeline;
this.description = description;
}

public PutSearchConfigurationRequest(StreamInput in) throws IOException {
Expand All @@ -33,6 +35,7 @@ public PutSearchConfigurationRequest(StreamInput in) throws IOException {
this.index = in.readString();
this.queryBody = in.readString();
this.searchPipeline = in.readOptionalString();
this.description = in.readOptionalString();
}

@Override
Expand All @@ -42,6 +45,7 @@ public void writeTo(StreamOutput out) throws IOException {
out.writeString(index);
out.writeString(queryBody);
out.writeOptionalString(searchPipeline);
out.writeOptionalString(description);
}

public String getName() {
Expand All @@ -60,6 +64,10 @@ public String getSearchPipeline() {
return searchPipeline;
}

public String getDescription() {
return description;
}

@Override
public ActionRequestValidationException validate() {
return null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -55,14 +55,24 @@ protected void doExecute(Task task, PutSearchConfigurationRequest request, Actio
listener.onFailure(new SearchRelevanceException("Name cannot be null or empty. Request: " + request, RestStatus.BAD_REQUEST));
return;
}
String description = request.getDescription();

String index = request.getIndex();
String queryBody = request.getQueryBody();
String searchPipeline = request.getSearchPipeline();

StepListener<Void> createIndexStep = new StepListener<>();
searchConfigurationDao.createIndexIfAbsent(createIndexStep);
createIndexStep.whenComplete(v -> {
SearchConfiguration searchConfiguration = new SearchConfiguration(id, name, timestamp, index, queryBody, searchPipeline);
SearchConfiguration searchConfiguration = new SearchConfiguration(
id,
name,
timestamp,
index,
queryBody,
searchPipeline,
description
);
searchConfigurationDao.putSearchConfiguration(searchConfiguration, listener);
}, listener::onFailure);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,13 @@
public class PutSearchConfigurationActionTests extends OpenSearchTestCase {

public void testStreams() throws IOException {
PutSearchConfigurationRequest request = new PutSearchConfigurationRequest("base_line", "sample_id", "{\"match_all\":{}}", "n/a");
PutSearchConfigurationRequest request = new PutSearchConfigurationRequest(
"base_line",
"sample_id",
"{\"match_all\":{}}",
"n/a",
"sample description"
);
BytesStreamOutput output = new BytesStreamOutput();
request.writeTo(output);
StreamInput in = StreamInput.wrap(output.bytes().toBytesRef().bytes);
Expand All @@ -26,10 +32,17 @@ public void testStreams() throws IOException {
assertEquals("sample_id", serialized.getIndex());
assertEquals("{\"match_all\":{}}", serialized.getQueryBody());
assertEquals("n/a", serialized.getSearchPipeline());
assertEquals("sample description", serialized.getDescription());
}

public void testRequestValidation() {
PutSearchConfigurationRequest request = new PutSearchConfigurationRequest("base_line", "sample_id", "{\"match_all\":{}}", "n/a");
PutSearchConfigurationRequest request = new PutSearchConfigurationRequest(
"base_line",
"sample_id",
"{\"match_all\":{}}",
"n/a",
"sample description"
);
assertNull(request.validate());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,7 @@ public void testMainActions_whenCreateReadDeleteSearchConfig_thenSuccessful() {
assertEquals("test_index", source.get("index"));
assertEquals("{\"query\": {\"match_all\": {}}}", source.get("query"));
assertEquals("test_pipeline", source.get("searchPipeline"));
assertEquals("sample description", source.get("description"));

Response deleteSearchConfigResponse = makeRequest(
client(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ public void testConcurrentFutureMapUpdates() {
}

public void testStartExperimentRunReject() {
// Make sure that only at most one experiment run for a given scheduled experiment run id
// Make sure that only at most one experiment run for a given scheduled experiment run id
// can be scheduled at a given time
// Also makes sure the entry in the mapping futures is removed when the cancellation token is cancelled.
PutExperimentRequest request = createExperimentRequest();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,6 @@
"name": "test_search_config",
"index": "test_index",
"query": "{\"query\": {\"match_all\": {}}}",
"searchPipeline": "test_pipeline"
"searchPipeline": "test_pipeline",
"description": "sample description"
}