diff --git a/src/main/java/org/opensearch/search/relevance/actionfilter/SearchActionFilter.java b/src/main/java/org/opensearch/search/relevance/actionfilter/SearchActionFilter.java index 85bfb06..d4635ca 100644 --- a/src/main/java/org/opensearch/search/relevance/actionfilter/SearchActionFilter.java +++ b/src/main/java/org/opensearch/search/relevance/actionfilter/SearchActionFilter.java @@ -24,6 +24,7 @@ import org.opensearch.common.io.stream.NamedWriteableRegistry; import org.opensearch.common.io.stream.StreamInput; import org.opensearch.common.settings.Setting; +import org.opensearch.common.settings.Settings; import org.opensearch.search.SearchHit; import org.opensearch.search.SearchHits; import org.opensearch.search.aggregations.InternalAggregations; @@ -89,7 +90,7 @@ public void app // TODO: Remove originalSearchSource and replace with a deep copy of the SearchRequest object // once https://github.com/opensearch-project/OpenSearch/issues/869 is implemented - SearchSourceBuilder originalSearchSource = null; + SearchSourceBuilder originalSearchSource; if (searchRequest.source() != null) { originalSearchSource = searchRequest.source().shallowCopy(); if (searchRequest.source().fetchSource() != null) { @@ -98,6 +99,8 @@ public void app originalSearchSource.fetchSource(new FetchSourceContext(fetchSourceContext.fetchSource(), fetchSourceContext.includes(), fetchSourceContext.excludes())); } + } else { + originalSearchSource = null; } final String[] indices = searchRequest.indices(); @@ -107,27 +110,29 @@ public void app return; } - List resultTransformerConfigurations = - getResultTransformerConfigurations(indices[0], searchRequest); - LinkedHashMap orderedTransformersAndConfigs = new LinkedHashMap<>(); - for (ResultTransformerConfiguration config : resultTransformerConfigurations) { - ResultTransformer resultTransformer = resultTransformerMap.get(config.getTransformerName()); - // TODO: Should transformers make a decision based on the original request or the request they receive in the chain - if (resultTransformer.shouldTransform(searchRequest, config)) { - searchRequest = resultTransformer.preprocessRequest(searchRequest, config); - orderedTransformersAndConfigs.put(resultTransformer, config); + ActionListener> resultTransformerConfigsListener = ActionListener.wrap(rtc -> { + LinkedHashMap orderedTransformersAndConfigs = new LinkedHashMap<>(); + SearchRequest transformedRequest = searchRequest; + for (ResultTransformerConfiguration config : rtc) { + ResultTransformer resultTransformer = resultTransformerMap.get(config.getTransformerName()); + // TODO: Should transformers make a decision based on the original request or the request they receive in the chain + if (resultTransformer.shouldTransform(searchRequest, config)) { + transformedRequest = resultTransformer.preprocessRequest(transformedRequest, config); + orderedTransformersAndConfigs.put(resultTransformer, config); + } } - } - if (!orderedTransformersAndConfigs.isEmpty()) { - final ActionListener searchResponseListener = createSearchResponseListener( - listener, startTime, orderedTransformersAndConfigs, searchRequest, originalSearchSource); - chain.proceed(task, action, request, searchResponseListener); - return; - } + if (!orderedTransformersAndConfigs.isEmpty()) { + final ActionListener searchResponseListener = createSearchResponseListener( + listener, startTime, orderedTransformersAndConfigs, transformedRequest, originalSearchSource); + chain.proceed(task, action, request, searchResponseListener); + return; + } + chain.proceed(task, action, request, listener); + }, listener::onFailure); - chain.proceed(task, action, request, listener); + getResultTransformerConfigurations(indices[0], searchRequest, resultTransformerConfigsListener); } /** @@ -139,16 +144,18 @@ public void app * @return ordered and validated list of result transformers, empty list if not specified at * either request or index level */ - private List getResultTransformerConfigurations( + private void getResultTransformerConfigurations( final String indexName, - final SearchRequest searchRequest) { + final SearchRequest searchRequest, + ActionListener> resultTransformerConfigListener) { List configs = new ArrayList<>(); // Request level configuration takes precedence over index level configs = ConfigurationUtils.getResultTransformersFromRequestConfiguration(searchRequest); if (!configs.isEmpty()) { - return configs; + resultTransformerConfigListener.onResponse(configs); + return; } // Fetch all index settings for this plugin @@ -159,10 +166,10 @@ private List getResultTransformerConfigurations( .map(Setting::getKey)) .toArray(String[]::new); - configs = ConfigurationUtils.getResultTransformersFromIndexConfiguration( - openSearchClient.getIndexSettings(indexName, settingNames), resultTransformerMap); - return configs; + ActionListener settingsListener = ActionListener.map(resultTransformerConfigListener, + s -> ConfigurationUtils.getResultTransformersFromIndexConfiguration(s, resultTransformerMap)); + openSearchClient.getIndexSettings(indexName, settingNames, settingsListener); } /** diff --git a/src/main/java/org/opensearch/search/relevance/client/OpenSearchClient.java b/src/main/java/org/opensearch/search/relevance/client/OpenSearchClient.java index 1b306dc..438685d 100644 --- a/src/main/java/org/opensearch/search/relevance/client/OpenSearchClient.java +++ b/src/main/java/org/opensearch/search/relevance/client/OpenSearchClient.java @@ -7,6 +7,7 @@ */ package org.opensearch.search.relevance.client; +import org.opensearch.action.ActionListener; import org.opensearch.action.admin.indices.settings.get.GetSettingsAction; import org.opensearch.action.admin.indices.settings.get.GetSettingsRequest; import org.opensearch.action.admin.indices.settings.get.GetSettingsResponse; @@ -20,13 +21,13 @@ public OpenSearchClient(Client client) { this.client = client; } - public Settings getIndexSettings(String indexName, String[] settingNames) { + public void getIndexSettings(String indexName, String[] settingNames, ActionListener settingsListener) { GetSettingsRequest getSettingsRequest = new GetSettingsRequest() .indices(indexName); if (settingNames != null && settingNames.length > 0) { getSettingsRequest.names(settingNames); } - GetSettingsResponse getSettingsResponse = client.execute(GetSettingsAction.INSTANCE, getSettingsRequest).actionGet(); - return getSettingsResponse.getIndexToSettings().get(indexName); + ActionListener responseListener = ActionListener.map(settingsListener, r -> r.getIndexToSettings().get(indexName)); + client.execute(GetSettingsAction.INSTANCE, getSettingsRequest, responseListener); } } diff --git a/src/main/java/org/opensearch/search/relevance/configuration/ConfigurationUtils.java b/src/main/java/org/opensearch/search/relevance/configuration/ConfigurationUtils.java index f2c9a71..eb1c498 100644 --- a/src/main/java/org/opensearch/search/relevance/configuration/ConfigurationUtils.java +++ b/src/main/java/org/opensearch/search/relevance/configuration/ConfigurationUtils.java @@ -7,6 +7,7 @@ */ package org.opensearch.search.relevance.configuration; +import org.opensearch.action.ActionListener; import org.opensearch.action.search.SearchRequest; import org.opensearch.common.settings.Settings; import org.opensearch.search.SearchExtBuilder; @@ -33,65 +34,67 @@ public static List getResultTransformersFromInde Map resultTransformerMap) { List indexLevelConfigs = new ArrayList<>(); - if (settings != null) { - if (settings.getGroups(RESULT_TRANSFORMER_SETTING_PREFIX) != null) { - for (Map.Entry transformerSettings : settings.getGroups(RESULT_TRANSFORMER_SETTING_PREFIX).entrySet()) { - if (resultTransformerMap.containsKey(transformerSettings.getKey())) { - ResultTransformer transformer = resultTransformerMap.get(transformerSettings.getKey()); - indexLevelConfigs.add(transformer.getConfigurationFactory().configure(transformerSettings.getValue())); - } + if (settings != null) { + if (settings.getGroups(RESULT_TRANSFORMER_SETTING_PREFIX) != null) { + for (Map.Entry transformerSettings : settings.getGroups(RESULT_TRANSFORMER_SETTING_PREFIX).entrySet()) { + if (resultTransformerMap.containsKey(transformerSettings.getKey())) { + ResultTransformer transformer = resultTransformerMap.get(transformerSettings.getKey()); + indexLevelConfigs.add(transformer.getConfigurationFactory().configure(transformerSettings.getValue())); } } } + } - return reorderAndValidateConfigs(indexLevelConfigs); - } + return reorderAndValidateConfigs(indexLevelConfigs); + } - /** - * Get result transformer configurations from Search Request - * @param searchRequest input request - * @return ordered and validated list of result transformers, empty list if not specified - */ - public static List getResultTransformersFromRequestConfiguration( - final SearchRequest searchRequest) { + /** + * Get result transformer configurations from Search Request + * + * @param searchRequest input request + * @return ordered and validated list of result transformers, empty list if not specified + */ + public static List getResultTransformersFromRequestConfiguration( + final SearchRequest searchRequest) { - // Fetch result transformers specified in request - SearchConfigurationExtBuilder requestLevelSearchConfiguration = null; - if (searchRequest.source() != null && searchRequest.source().ext() != null && !searchRequest.source().ext().isEmpty()) { - // Filter ext builders by name - List extBuilders = searchRequest.source().ext().stream() - .filter(searchExtBuilder -> SearchConfigurationExtBuilder.NAME.equals(searchExtBuilder.getWriteableName())) - .collect(Collectors.toList()); - if (!extBuilders.isEmpty()) { - requestLevelSearchConfiguration = (SearchConfigurationExtBuilder) extBuilders.get(0); - } - } + // Fetch result transformers specified in request + SearchConfigurationExtBuilder requestLevelSearchConfiguration = null; + if (searchRequest.source() != null && searchRequest.source().ext() != null && !searchRequest.source().ext().isEmpty()) { + // Filter ext builders by name + List extBuilders = searchRequest.source().ext().stream() + .filter(searchExtBuilder -> SearchConfigurationExtBuilder.NAME.equals(searchExtBuilder.getWriteableName())) + .collect(Collectors.toList()); + if (!extBuilders.isEmpty()) { + requestLevelSearchConfiguration = (SearchConfigurationExtBuilder) extBuilders.get(0); + } + } - List requestLevelConfigs = new ArrayList<>(); - if (requestLevelSearchConfiguration != null) { - requestLevelConfigs = reorderAndValidateConfigs(requestLevelSearchConfiguration.getResultTransformers()); + List requestLevelConfigs = new ArrayList<>(); + if (requestLevelSearchConfiguration != null) { + requestLevelConfigs = reorderAndValidateConfigs(requestLevelSearchConfiguration.getResultTransformers()); + } + return requestLevelConfigs; } - return requestLevelConfigs; - } - /** - * Sort configurations in ascending order of invocation, and validate - * @param configs list of result transformer configurations - * @return ordered and validated list of result transformers - */ - public static List reorderAndValidateConfigs( - final List configs) throws IllegalArgumentException { + /** + * Sort configurations in ascending order of invocation, and validate + * + * @param configs list of result transformer configurations + * @return ordered and validated list of result transformers + */ + public static List reorderAndValidateConfigs( + final List configs) throws IllegalArgumentException { - // Sort - configs.sort(Comparator.comparingInt(ResultTransformerConfiguration::getOrder)); + // Sort + configs.sort(Comparator.comparingInt(ResultTransformerConfiguration::getOrder)); - for (int i = 0; i < configs.size(); ++i) { - if (configs.get(i).getOrder() != (i + 1)) { - throw new IllegalArgumentException("Expected order [" + (i + 1) + "] for transformer [" + - configs.get(i).getTransformerName() + "], but found [" + configs.get(i).getOrder() + "]"); - } - } + for (int i = 0; i < configs.size(); ++i) { + if (configs.get(i).getOrder() != (i + 1)) { + throw new IllegalArgumentException("Expected order [" + (i + 1) + "] for transformer [" + + configs.get(i).getTransformerName() + "], but found [" + configs.get(i).getOrder() + "]"); + } + } - return configs; - } + return configs; + } } diff --git a/src/test/java/org/opensearch/search/relevance/actionfilter/SearchActionFilterTests.java b/src/test/java/org/opensearch/search/relevance/actionfilter/SearchActionFilterTests.java index 0625792..b09dbc5 100644 --- a/src/test/java/org/opensearch/search/relevance/actionfilter/SearchActionFilterTests.java +++ b/src/test/java/org/opensearch/search/relevance/actionfilter/SearchActionFilterTests.java @@ -9,7 +9,6 @@ import org.apache.lucene.search.TotalHits; import org.mockito.Mockito; -import org.opensearch.action.ActionFuture; import org.opensearch.action.ActionListener; import org.opensearch.action.admin.indices.settings.get.GetSettingsAction; import org.opensearch.action.admin.indices.settings.get.GetSettingsRequest; @@ -56,7 +55,8 @@ import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.mock; public class SearchActionFilterTests extends OpenSearchTestCase { @@ -115,7 +115,6 @@ public void testIgnoresSearchRequestOnMultipleIndices() { private static Client buildMockClient(String indexName, Settings... settings) { Client client = Mockito.mock(Client.class); - ActionFuture mockGetSettingsFuture = Mockito.mock(ActionFuture.class); Settings.Builder settingsBuilder = Settings.builder(); for (Settings settingsEntry : settings) { @@ -124,9 +123,11 @@ private static Client buildMockClient(String indexName, Settings... settings) { Settings settingsObj = settingsBuilder.build(); Map indexSettingsMap = Map.of(indexName, settingsObj); GetSettingsResponse getSettingsResponse = new GetSettingsResponse(indexSettingsMap, Collections.emptyMap()); - when(mockGetSettingsFuture.actionGet()).thenReturn(getSettingsResponse); - when(client.execute(eq(GetSettingsAction.INSTANCE), any(GetSettingsRequest.class))) - .thenReturn(mockGetSettingsFuture); + doAnswer(invocation -> { + ActionListener responseListener = invocation.getArgument(2); + responseListener.onResponse(getSettingsResponse); + return null; + }).when(client).execute(eq(GetSettingsAction.INSTANCE), any(GetSettingsRequest.class), any(ActionListener.class)); return client; } @@ -145,7 +146,8 @@ public void testOperatesOnSingleIndexWithNoTransformers() { AtomicBoolean proceedCalled = new AtomicBoolean(false); ActionFilterChain searchFilterChain = (task1, action, request, listener) -> proceedCalled.set(true); - searchActionFilter.apply(task, SearchAction.NAME, searchRequest, null, searchFilterChain); + ActionListener mockListener = mock(ActionListener.class); + searchActionFilter.apply(task, SearchAction.NAME, searchRequest, mockListener, searchFilterChain); assertTrue(proceedCalled.get()); } @@ -263,7 +265,8 @@ public void testTransformerDoesNotRunWhenNotEnabled() { AtomicBoolean proceedCalled = new AtomicBoolean(false); ActionFilterChain searchFilterChain = (task1, action, request, listener) -> proceedCalled.set(true); - searchActionFilter.apply(task, SearchAction.NAME, searchRequest, null, searchFilterChain); + ActionListener mockListener = mock(ActionListener.class); + searchActionFilter.apply(task, SearchAction.NAME, searchRequest, mockListener, searchFilterChain); assertTrue(proceedCalled.get()); // We should try to check for index-level settings assertTrue(mockTransformer.getTransformerSettingsWasCalled); diff --git a/src/yamlRestTest/resources/rest-api-spec/test/10_basic.yml b/src/yamlRestTest/resources/rest-api-spec/test/10_basic.yml index 9898f9f..0b4f613 100644 --- a/src/yamlRestTest/resources/rest-api-spec/test/10_basic.yml +++ b/src/yamlRestTest/resources/rest-api-spec/test/10_basic.yml @@ -6,3 +6,12 @@ - match: $body: /^opensearch-search-processor-\d+.\d+.\d+.\d+\n$/ + + - do: + indices.create: + index: test + + - do: + search: + index: test + body: { }