[Bug fixes] Fix ListIndexTool and SearchIndexTool #3720
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,12 +8,13 @@ | |
| import static org.opensearch.action.support.clustermanager.ClusterManagerNodeRequest.DEFAULT_CLUSTER_MANAGER_NODE_TIMEOUT; | ||
| import static org.opensearch.ml.common.utils.StringUtils.gson; | ||
|
|
||
| import java.util.ArrayList; | ||
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.Map.Entry; | ||
|
|
||
| import org.apache.commons.lang3.StringUtils; | ||
| import org.apache.logging.log4j.util.Strings; | ||
| import org.opensearch.action.admin.indices.get.GetIndexRequest; | ||
| import org.opensearch.action.admin.indices.get.GetIndexResponse; | ||
|
|
@@ -34,6 +35,8 @@ | |
| @ToolAnnotation(IndexMappingTool.TYPE) | ||
| public class IndexMappingTool implements Tool { | ||
| public static final String TYPE = "IndexMappingTool"; | ||
| public static final String INPUT_SCHEMA_FIELD = "input_schema"; | ||
| public static final String STRICT_FIELD = "strict"; | ||
| private static final String DEFAULT_DESCRIPTION = String | ||
| .join( | ||
| " ", | ||
|
|
@@ -44,6 +47,12 @@ public class IndexMappingTool implements Tool { | |
| "The mappings are in JSON format under the key 'properties' which includes the field name as a key and a JSON object with field type under the key 'type'.", | ||
| "The settings are in flattened map with 'index' as the top element and key-value pairs for each setting." | ||
| ); | ||
| public static final String DEFAULT_INPUT_SCHEMA = "{\"type\":\"object\",\"" | ||
| + "properties\":{\"index\":{\"type\":\"array\",\"description\":\"OpenSearch index name list, separated by comma. " | ||
| + "for example: [\\\"index1\\\", \\\"index2\\\"]\"," | ||
| + "\"items\":{\"type\":\"string\"}}}," | ||
| + "\"required\":[\"index\"]," | ||
| + "\"additionalProperties\":false}"; | ||
|
|
||
| @Setter | ||
| @Getter | ||
|
|
@@ -67,12 +76,8 @@ public IndexMappingTool(Client client) { | |
| this.client = client; | ||
|
|
||
| this.attributes = new HashMap<>(); | ||
| attributes.put(INPUT_SCHEMA_FIELD, DEFAULT_INPUT_SCHEMA); | ||
| attributes.put(STRICT_FIELD, true); | ||
|
|
||
| outputParser = new Parser<>() { | ||
| @Override | ||
|
|
@@ -86,75 +91,80 @@ public Object parse(Object o) { | |
|
|
||
| @Override | ||
| public <T> void run(Map<String, String> parameters, ActionListener<T> listener) { | ||
| try { | ||
| List<String> indexList = new ArrayList<>(); | ||
| if (StringUtils.isNotBlank(parameters.get("index"))) { | ||
pyek-bot marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| indexList = gson.fromJson(parameters.get("index"), List.class); | ||
| } | ||
|
|
||
| if (indexList.isEmpty()) { | ||
| @SuppressWarnings("unchecked") | ||
|
There was a problem hiding this comment. are just trying to returnning a string on listener response? it's not recommended to supresswarning, try compose the string in a better way. There was a problem hiding this comment. but I don't have the context on this listener, when it's empty index list, should it return empty list or this string? try this? There was a problem hiding this comment. most of it is existing code that has been moved (see above), i lack the complete context on why it was built this way tbh so the tools are executed using input provided by a model - these are always going to be strings and the response is also passed as a string to the model, therefore it is safe to work with the assumption its a string however, there is concern if at any point this object changes, i will try to see if it can be handled in a better way There was a problem hiding this comment. @mingshl , This is to return the correct type The actionListener expects type There was a problem hiding this comment. @pyek-bot , let's create issue tracking the issue and fix in new PR ? There was a problem hiding this comment. Sure, created issue: #3722 |
||
| T empty = (T) ("There were no results searching the index parameter [" + parameters.get("index") + "]."); | ||
|
There was a problem hiding this comment. how do you know this guarantee that T is actually String. Otherwise this will cause a ClassCastException at runtime. There was a problem hiding this comment. This is existing code. But I think we can enhance the logic as we already know the |
||
| listener.onResponse(empty); | ||
| return; | ||
| } | ||
|
|
||
| final String[] indices = indexList.toArray(Strings.EMPTY_ARRAY); | ||
|
|
||
| final IndicesOptions indicesOptions = IndicesOptions.strictExpand(); | ||
| final boolean local = Boolean.parseBoolean(parameters.getOrDefault("local", "false")); | ||
| final TimeValue clusterManagerNodeTimeout = DEFAULT_CLUSTER_MANAGER_NODE_TIMEOUT; | ||
|
|
||
| ActionListener<GetIndexResponse> internalListener = new ActionListener<GetIndexResponse>() { | ||
|
|
||
| @Override | ||
| public void onResponse(GetIndexResponse getIndexResponse) { | ||
| try { | ||
| // Handle empty response | ||
| if (getIndexResponse.indices().length == 0) { | ||
| @SuppressWarnings("unchecked") | ||
| T empty = (T) ("There were no results searching the index parameter [" + parameters.get("index") + "]."); | ||
|
There was a problem hiding this comment. same here, can you avoid this manual suppresswarning |
||
| listener.onResponse(empty); | ||
| return; | ||
| } | ||
| StringBuilder sb = new StringBuilder(); | ||
| for (String index : getIndexResponse.indices()) { | ||
| sb.append("index: ").append(index).append("\n\n"); | ||
|
|
||
| MappingMetadata mapping = getIndexResponse.mappings().get(index); | ||
| if (mapping != null) { | ||
| sb.append("mappings:\n"); | ||
| for (Entry<String, Object> entry : mapping.sourceAsMap().entrySet()) { | ||
| sb.append(entry.getKey()).append("=").append(entry.getValue()).append('\n'); | ||
| } | ||
| sb.append("\n\n"); | ||
| } | ||
|
|
||
| Settings settings = getIndexResponse.settings().get(index); | ||
| if (settings != null) { | ||
| sb.append("settings:\n").append(settings.toDelimitedString('\n')).append("\n\n"); | ||
| } | ||
| } | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| T response = (T) sb.toString(); | ||
|
There was a problem hiding this comment. maybe using the StringUtils.toString(), which takes in an object will help in this case? |
||
| listener.onResponse(response); | ||
| } catch (Exception e) { | ||
| onFailure(e); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
| public void onFailure(final Exception e) { | ||
| listener.onFailure(e); | ||
| } | ||
|
|
||
| }; | ||
| final GetIndexRequest getIndexRequest = new GetIndexRequest() | ||
| .indices(indices) | ||
| .indicesOptions(indicesOptions) | ||
| .local(local) | ||
| .clusterManagerNodeTimeout(clusterManagerNodeTimeout); | ||
|
|
||
| client.admin().indices().getIndex(getIndexRequest, internalListener); | ||
| } catch (Exception e) { | ||
| listener.onFailure(e); | ||
| } | ||
| } | ||
|
|
||
| @Override | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it the attributes and parameters are reading the same map? what is the intention?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
earlier, parameters was being overwritten by attributes map as we were reassigning parameters (bug)
now, we are assigning it correctly to attributes
attributes map is the last item in the bytestream, readMap() is a method that allows populating a map from the bytestream
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am noticing some weird behavior in a multi-node cluster, will validate and get back
***resolved, the issue was with reading attributes twice, this code is still valid