diff --git a/ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/ReadFromScratchPadTool.java b/ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/ReadFromScratchPadTool.java index 402342ca7c..70ca8a1072 100644 --- a/ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/ReadFromScratchPadTool.java +++ b/ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/ReadFromScratchPadTool.java @@ -100,18 +100,12 @@ public boolean validate(Map parameters) { */ @Override public void run(Map parameters, ActionListener listener) { - // Handle both List and String (JSON) formats for existing notes + List notes; - Map rawParameters = parameters; - Object existingNotes = rawParameters.get(SCRATCHPAD_NOTES_KEY); - if (existingNotes instanceof List) { - notes = new ArrayList<>((List) existingNotes); - } else if (existingNotes instanceof String) { - List parsedNotes = StringUtils.parseStringArrayToList((String) existingNotes); - notes = parsedNotes != null ? new ArrayList<>(parsedNotes) : new ArrayList<>(); - } else { - notes = new ArrayList<>(); - } + String existingNotes = parameters.getOrDefault(SCRATCHPAD_NOTES_KEY, "[]"); + + List parsedNotes = StringUtils.parseStringArrayToList(existingNotes); + notes = parsedNotes != null ? new ArrayList<>(parsedNotes) : new ArrayList<>(); String persistentNotes = parameters.getOrDefault(PERSISTENT_NOTES_KEY, ""); @@ -119,7 +113,7 @@ public void run(Map parameters, ActionListener listener) notes.add(persistentNotes); } - rawParameters.put(SCRATCHPAD_NOTES_KEY, notes); + parameters.put(SCRATCHPAD_NOTES_KEY, StringUtils.toJson(notes)); if (notes.isEmpty()) { listener.onResponse((T) "Scratchpad is empty."); diff --git a/ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/WriteToScratchPadTool.java b/ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/WriteToScratchPadTool.java index 82d3e2148b..ba15fbb8b3 100644 --- a/ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/WriteToScratchPadTool.java +++ b/ml-algorithms/src/main/java/org/opensearch/ml/engine/tools/WriteToScratchPadTool.java @@ -111,21 +111,15 @@ public void run(Map parameters, ActionListener listener) return; } - // Handle both List and String (JSON) formats for existing notes List notes; - Map rawParameters = parameters; - Object existingNotes = rawParameters.get(SCRATCHPAD_NOTES_KEY); - if (existingNotes instanceof List) { - notes = new ArrayList<>((List) existingNotes); - } else if (existingNotes instanceof String) { - List parsedNotes = StringUtils.parseStringArrayToList((String) existingNotes); - notes = parsedNotes != null ? new ArrayList<>(parsedNotes) : new ArrayList<>(); - } else { - notes = new ArrayList<>(); - } + ; + String existingNotes = parameters.getOrDefault(SCRATCHPAD_NOTES_KEY, "[]"); + + List parsedNotes = StringUtils.parseStringArrayToList(existingNotes); + notes = parsedNotes != null ? new ArrayList<>(parsedNotes) : new ArrayList<>(); notes.add(currentNote); - rawParameters.put(SCRATCHPAD_NOTES_KEY, notes); + parameters.put(SCRATCHPAD_NOTES_KEY, StringUtils.toJson(notes)); if (returnHistory) { String fullNotesFormatted = "- " + String.join("\n- ", notes); diff --git a/ml-algorithms/src/test/java/org/opensearch/ml/engine/tools/ReadFromScratchPadToolTests.java b/ml-algorithms/src/test/java/org/opensearch/ml/engine/tools/ReadFromScratchPadToolTests.java index 6324e38830..b94960e107 100644 --- a/ml-algorithms/src/test/java/org/opensearch/ml/engine/tools/ReadFromScratchPadToolTests.java +++ b/ml-algorithms/src/test/java/org/opensearch/ml/engine/tools/ReadFromScratchPadToolTests.java @@ -10,8 +10,6 @@ import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.verify; -import java.util.ArrayList; -import java.util.Arrays; import java.util.HashMap; import java.util.Map; @@ -83,89 +81,89 @@ public void testValidate() { @Test public void testRun_NoNotes() { - Map parameters = new HashMap<>(); - tool.run((Map) parameters, listener); + Map parameters = new HashMap<>(); + tool.run(parameters, listener); ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); verify(listener).onResponse(captor.capture()); assertEquals("Scratchpad is empty.", captor.getValue()); - assertEquals(new ArrayList<>(), parameters.get(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY)); + assertEquals("[]", parameters.get(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY)); } @Test public void testRun_WithScratchpadNotes() { - Map parameters = new HashMap<>(); - parameters.put(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY, new ArrayList<>(Arrays.asList("existing note"))); - tool.run((Map) parameters, listener); + Map parameters = new HashMap<>(); + parameters.put(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY, "[\"existing note\"]"); + tool.run(parameters, listener); ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); verify(listener).onResponse(captor.capture()); assertEquals("Notes from scratchpad:\n- existing note", captor.getValue()); - assertEquals(Arrays.asList("existing note"), parameters.get(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY)); + assertEquals("[\"existing note\"]", parameters.get(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY)); } @Test public void testRun_WithPersistentNotes() { - Map parameters = new HashMap<>(); - parameters.put(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY, new ArrayList<>()); // Initialize with empty list + Map parameters = new HashMap<>(); + parameters.put(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY, "[]"); parameters.put(ReadFromScratchPadTool.PERSISTENT_NOTES_KEY, "persistent note"); - tool.run((Map) parameters, listener); + tool.run(parameters, listener); ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); verify(listener).onResponse(captor.capture()); assertEquals("Notes from scratchpad:\n- persistent note", captor.getValue()); - assertEquals(Arrays.asList("persistent note"), parameters.get(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY)); + assertEquals("[\"persistent note\"]", parameters.get(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY)); } @Test public void testRun_WithBothNotes() { - Map parameters = new HashMap<>(); - parameters.put(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY, new ArrayList<>(Arrays.asList("existing note"))); + Map parameters = new HashMap<>(); + parameters.put(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY, "[\"existing note\"]"); parameters.put(ReadFromScratchPadTool.PERSISTENT_NOTES_KEY, "persistent note"); - tool.run((Map) parameters, listener); + tool.run(parameters, listener); ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); verify(listener).onResponse(captor.capture()); assertEquals("Notes from scratchpad:\n- existing note\n- persistent note", captor.getValue()); - assertEquals(Arrays.asList("existing note", "persistent note"), parameters.get(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY)); + assertEquals("[\"existing note\",\"persistent note\"]", parameters.get(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY)); } @Test public void testRun_WithDuplicatePersistentNotes() { - Map parameters = new HashMap<>(); - parameters.put(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY, new ArrayList<>(Arrays.asList("existing note", "persistent note"))); + Map parameters = new HashMap<>(); + parameters.put(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY, "[\"existing note\",\"persistent note\"]"); parameters.put(ReadFromScratchPadTool.PERSISTENT_NOTES_KEY, "persistent note"); - tool.run((Map) parameters, listener); + tool.run(parameters, listener); ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); verify(listener).onResponse(captor.capture()); assertEquals("Notes from scratchpad:\n- existing note\n- persistent note", captor.getValue()); - assertEquals(Arrays.asList("existing note", "persistent note"), parameters.get(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY)); + assertEquals("[\"existing note\",\"persistent note\"]", parameters.get(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY)); } @Test public void testRun_WithNonListScratchpadNotes() { - Map parameters = new HashMap<>(); + Map parameters = new HashMap<>(); parameters.put(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY, "not a list"); - tool.run((Map) parameters, listener); + tool.run(parameters, listener); ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); verify(listener).onResponse(captor.capture()); assertEquals("Scratchpad is empty.", captor.getValue()); - assertEquals(new ArrayList<>(), parameters.get(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY)); + assertEquals("[]", parameters.get(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY)); } @Test public void testRun_WithJsonStringNotes() { - Map parameters = new HashMap<>(); + Map parameters = new HashMap<>(); parameters.put(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY, "[\"json note\"]"); - tool.run((Map) parameters, listener); + tool.run(parameters, listener); ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); verify(listener).onResponse(captor.capture()); assertEquals("Notes from scratchpad:\n- json note", captor.getValue()); - assertEquals(Arrays.asList("json note"), parameters.get(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY)); + assertEquals("[\"json note\"]", parameters.get(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY)); } @Test public void testRun_WithEmptyPersistentNotes() { - Map parameters = new HashMap<>(); - parameters.put(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY, new ArrayList<>(Arrays.asList("existing note"))); + Map parameters = new HashMap<>(); + parameters.put(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY, "[\"existing note\"]"); parameters.put(ReadFromScratchPadTool.PERSISTENT_NOTES_KEY, ""); - tool.run((Map) parameters, listener); + tool.run(parameters, listener); ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); verify(listener).onResponse(captor.capture()); assertEquals("Notes from scratchpad:\n- existing note", captor.getValue()); @@ -173,15 +171,52 @@ public void testRun_WithEmptyPersistentNotes() { @Test public void testRun_WithNullPersistentNotes() { - Map parameters = new HashMap<>(); - parameters.put(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY, new ArrayList<>(Arrays.asList("existing note"))); + Map parameters = new HashMap<>(); + parameters.put(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY, "[\"existing note\"]"); parameters.put(ReadFromScratchPadTool.PERSISTENT_NOTES_KEY, null); - tool.run((Map) parameters, listener); + tool.run(parameters, listener); ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); verify(listener).onResponse(captor.capture()); assertEquals("Notes from scratchpad:\n- existing note", captor.getValue()); } + @Test + public void testRun_StringConversion_EmptyArray() { + Map parameters = new HashMap<>(); + parameters.put(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY, "[]"); + tool.run(parameters, listener); + + ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); + verify(listener).onResponse(captor.capture()); + assertEquals("Scratchpad is empty.", captor.getValue()); + assertEquals("[]", parameters.get(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY)); + } + + @Test + public void testRun_StringConversion_WithJsonArray() { + Map parameters = new HashMap<>(); + parameters.put(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY, "[\"note1\",\"note2\"]"); + tool.run(parameters, listener); + + ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); + verify(listener).onResponse(captor.capture()); + assertEquals("Notes from scratchpad:\n- note1\n- note2", captor.getValue()); + assertEquals("[\"note1\",\"note2\"]", parameters.get(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY)); + } + + @Test + public void testRun_StringConversion_AddPersistentNote() { + Map parameters = new HashMap<>(); + parameters.put(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY, "[\"existing\"]"); + parameters.put(ReadFromScratchPadTool.PERSISTENT_NOTES_KEY, "new note"); + tool.run(parameters, listener); + + ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); + verify(listener).onResponse(captor.capture()); + assertEquals("Notes from scratchpad:\n- existing\n- new note", captor.getValue()); + assertEquals("[\"existing\",\"new note\"]", parameters.get(ReadFromScratchPadTool.SCRATCHPAD_NOTES_KEY)); + } + @Test public void testFactory() { ReadFromScratchPadTool.Factory factory = ReadFromScratchPadTool.Factory.getInstance(); diff --git a/ml-algorithms/src/test/java/org/opensearch/ml/engine/tools/WriteToScratchPadToolTests.java b/ml-algorithms/src/test/java/org/opensearch/ml/engine/tools/WriteToScratchPadToolTests.java index 5ac432e265..35419d1872 100644 --- a/ml-algorithms/src/test/java/org/opensearch/ml/engine/tools/WriteToScratchPadToolTests.java +++ b/ml-algorithms/src/test/java/org/opensearch/ml/engine/tools/WriteToScratchPadToolTests.java @@ -11,8 +11,6 @@ import static org.junit.Assert.assertTrue; import static org.mockito.Mockito.verify; -import java.util.ArrayList; -import java.util.Arrays; import java.util.HashMap; import java.util.Map; @@ -90,33 +88,33 @@ public void testValidate() { @Test public void testRun_Success_NoExistingNotes() { - Map parameters = new HashMap<>(); + Map parameters = new HashMap<>(); parameters.put(WriteToScratchPadTool.NOTES_KEY, "new note"); - tool.run((Map) parameters, listener); + tool.run(parameters, listener); ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); verify(listener).onResponse(captor.capture()); assertEquals("Wrote to scratchpad: new note", captor.getValue()); - assertEquals(Arrays.asList("new note"), parameters.get(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY)); + assertEquals("[\"new note\"]", parameters.get(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY)); } @Test public void testRun_Success_WithExistingNotes() { - Map parameters = new HashMap<>(); + Map parameters = new HashMap<>(); parameters.put(WriteToScratchPadTool.NOTES_KEY, "new note"); - parameters.put(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY, new ArrayList<>(Arrays.asList("existing note"))); - tool.run((Map) parameters, listener); + parameters.put(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY, "[\"existing note\"]"); + tool.run(parameters, listener); ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); verify(listener).onResponse(captor.capture()); assertEquals("Wrote to scratchpad: new note", captor.getValue()); - assertEquals(Arrays.asList("existing note", "new note"), parameters.get(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY)); + assertEquals("[\"existing note\",\"new note\"]", parameters.get(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY)); } @Test public void testRun_Failure_NoNotes() { - Map parameters = new HashMap<>(); - tool.run((Map) parameters, listener); + Map parameters = new HashMap<>(); + tool.run(parameters, listener); ArgumentCaptor captor = ArgumentCaptor.forClass(Exception.class); verify(listener).onFailure(captor.capture()); @@ -126,9 +124,9 @@ public void testRun_Failure_NoNotes() { @Test public void testRun_Failure_EmptyNotes() { - Map parameters = new HashMap<>(); + Map parameters = new HashMap<>(); parameters.put(WriteToScratchPadTool.NOTES_KEY, ""); - tool.run((Map) parameters, listener); + tool.run(parameters, listener); ArgumentCaptor captor = ArgumentCaptor.forClass(Exception.class); verify(listener).onFailure(captor.capture()); @@ -138,69 +136,109 @@ public void testRun_Failure_EmptyNotes() { @Test public void testRun_Success_ReturnHistory_WithExistingNotes() { - Map parameters = new HashMap<>(); + Map parameters = new HashMap<>(); parameters.put(WriteToScratchPadTool.NOTES_KEY, "new note"); - parameters.put(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY, new ArrayList<>(Arrays.asList("existing note"))); + parameters.put(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY, "[\"existing note\"]"); parameters.put(WriteToScratchPadTool.RETURN_HISTORY_KEY, "true"); - tool.run((Map) parameters, listener); + tool.run(parameters, listener); ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); verify(listener).onResponse(captor.capture()); assertEquals("Scratchpad updated. Full content:\n- existing note\n- new note", captor.getValue()); - assertEquals(Arrays.asList("existing note", "new note"), parameters.get(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY)); + assertEquals("[\"existing note\",\"new note\"]", parameters.get(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY)); } @Test public void testRun_Success_ReturnHistory_NoExistingNotes() { - Map parameters = new HashMap<>(); + Map parameters = new HashMap<>(); parameters.put(WriteToScratchPadTool.NOTES_KEY, "new note"); parameters.put(WriteToScratchPadTool.RETURN_HISTORY_KEY, "true"); - tool.run((Map) parameters, listener); + tool.run(parameters, listener); ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); verify(listener).onResponse(captor.capture()); assertEquals("Scratchpad updated. Full content:\n- new note", captor.getValue()); - assertEquals(Arrays.asList("new note"), parameters.get(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY)); + assertEquals("[\"new note\"]", parameters.get(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY)); } @Test public void testRun_Success_NonListScratchpadNotes() { - Map parameters = new HashMap<>(); + Map parameters = new HashMap<>(); parameters.put(WriteToScratchPadTool.NOTES_KEY, "new note"); parameters.put(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY, "not a list"); - tool.run((Map) parameters, listener); + tool.run(parameters, listener); ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); verify(listener).onResponse(captor.capture()); assertEquals("Wrote to scratchpad: new note", captor.getValue()); - assertEquals(Arrays.asList("new note"), parameters.get(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY)); + assertEquals("[\"new note\"]", parameters.get(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY)); } @Test public void testRun_Success_WithJsonStringNotes() { - Map parameters = new HashMap<>(); + Map parameters = new HashMap<>(); parameters.put(WriteToScratchPadTool.NOTES_KEY, "new note"); parameters.put(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY, "[\"existing note\"]"); - tool.run((Map) parameters, listener); + tool.run(parameters, listener); ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); verify(listener).onResponse(captor.capture()); assertEquals("Wrote to scratchpad: new note", captor.getValue()); - assertEquals(Arrays.asList("existing note", "new note"), parameters.get(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY)); + assertEquals("[\"existing note\",\"new note\"]", parameters.get(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY)); } @Test public void testRun_Success_ReturnHistory_False() { - Map parameters = new HashMap<>(); + Map parameters = new HashMap<>(); parameters.put(WriteToScratchPadTool.NOTES_KEY, "new note"); parameters.put(WriteToScratchPadTool.RETURN_HISTORY_KEY, "false"); - tool.run((Map) parameters, listener); + tool.run(parameters, listener); ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); verify(listener).onResponse(captor.capture()); assertEquals("Wrote to scratchpad: new note", captor.getValue()); } + @Test + public void testRun_StringConversion_EmptyArray() { + Map parameters = new HashMap<>(); + parameters.put(WriteToScratchPadTool.NOTES_KEY, "test note"); + parameters.put(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY, "[]"); + tool.run(parameters, listener); + + ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); + verify(listener).onResponse(captor.capture()); + assertEquals("Wrote to scratchpad: test note", captor.getValue()); + assertEquals("[\"test note\"]", parameters.get(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY)); + } + + @Test + public void testRun_StringConversion_WithJsonArray() { + Map parameters = new HashMap<>(); + parameters.put(WriteToScratchPadTool.NOTES_KEY, "new note"); + parameters.put(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY, "[\"existing note\"]"); + tool.run(parameters, listener); + + ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); + verify(listener).onResponse(captor.capture()); + assertEquals("Wrote to scratchpad: new note", captor.getValue()); + assertEquals("[\"existing note\",\"new note\"]", parameters.get(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY)); + } + + @Test + public void testRun_StringConversion_ReturnHistory() { + Map parameters = new HashMap<>(); + parameters.put(WriteToScratchPadTool.NOTES_KEY, "new note"); + parameters.put(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY, "[\"existing\"]"); + parameters.put(WriteToScratchPadTool.RETURN_HISTORY_KEY, "true"); + tool.run(parameters, listener); + + ArgumentCaptor captor = ArgumentCaptor.forClass(String.class); + verify(listener).onResponse(captor.capture()); + assertEquals("Scratchpad updated. Full content:\n- existing\n- new note", captor.getValue()); + assertEquals("[\"existing\",\"new note\"]", parameters.get(WriteToScratchPadTool.SCRATCHPAD_NOTES_KEY)); + } + @Test public void testFactory() { WriteToScratchPadTool.Factory factory = WriteToScratchPadTool.Factory.getInstance();