Skip to content

Commit da485fb

Browse files
b4sjoob4sjoo
authored
[Bug] fix: Suppres NONE event in add memories response and add UT coverage (opensearch-project#4071)
* Add UT coverage for memories API Signed-off-by: Sicheng Song <[email protected]> * suppres NONE event in add memories response Signed-off-by: Sicheng Song <[email protected]> --------- Signed-off-by: Sicheng Song <[email protected]>
1 parent 2b54733 commit da485fb

13 files changed

+3127
-61
lines changed

‎plugin/build.gradle‎

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -383,20 +383,8 @@ List<String> jacocoExclusions = [
383383
'org.opensearch.ml.utils.ParseUtils',
384384
'org.opensearch.ml.jobs.processors.MLStatsJobProcessor',
385385
'org.opensearch.ml.jobs.processors.MLJobProcessor',
386-
// Memory container related exclusions
387386
'org.opensearch.ml.action.memorycontainer.memory.TransportAddMemoriesAction',
388-
'org.opensearch.ml.action.memorycontainer.memory.TransportSearchMemoriesAction',
389-
'org.opensearch.ml.action.memorycontainer.memory.TransportDeleteMemoryAction',
390-
'org.opensearch.ml.action.memorycontainer.memory.TransportUpdateMemoryAction',
391-
'org.opensearch.ml.action.memorycontainer.memory.TransportAddMemoriesAction.*',
392-
'org.opensearch.ml.action.memorycontainer.memory.TransportSearchMemoriesAction.*',
393-
'org.opensearch.ml.action.memorycontainer.memory.TransportDeleteMemoryAction.*',
394-
'org.opensearch.ml.action.memorycontainer.memory.TransportUpdateMemoryAction.*',
395-
'org.opensearch.ml.rest.RestMLAddMemoriesAction',
396-
'org.opensearch.ml.rest.RestMLSearchMemoriesAction',
397-
'org.opensearch.ml.rest.RestMLDeleteMemoryAction',
398-
'org.opensearch.ml.rest.RestMLUpdateMemoryAction',
399-
'org.opensearch.ml.utils.MemorySearchQueryBuilder'
387+
'org.opensearch.ml.action.memorycontainer.memory.TransportAddMemoriesAction.*'
400388
]
401389

402390
jacocoTestCoverageVerification {

‎plugin/src/main/java/org/opensearch/ml/action/memorycontainer/memory/MemoryOperationsService.java‎

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -125,16 +125,8 @@ public void executeMemoryOperations(
125125
break;
126126

127127
case NONE:
128-
results
129-
.add(
130-
MemoryResult
131-
.builder()
132-
.memoryId(decision.getId())
133-
.memory(decision.getText())
134-
.event(MemoryEvent.NONE)
135-
.oldMemory(null)
136-
.build()
137-
);
128+
// NONE events are not included in the response
129+
// They represent facts that didn't change and don't need user visibility
138130
break;
139131
}
140132
}

‎plugin/src/main/java/org/opensearch/ml/rest/RestMLAddMemoriesAction.java‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55

66
package org.opensearch.ml.rest;
77

8+
import static org.opensearch.core.xcontent.XContentParserUtils.ensureExpectedToken;
89
import static org.opensearch.ml.common.memorycontainer.MemoryContainerConstants.MEMORIES_PATH;
910
import static org.opensearch.ml.common.memorycontainer.MemoryContainerConstants.PARAMETER_MEMORY_CONTAINER_ID;
1011
import static org.opensearch.ml.common.settings.MLCommonsSettings.ML_COMMONS_AGENTIC_MEMORY_DISABLED_MESSAGE;
@@ -78,10 +79,4 @@ private MLAddMemoriesRequest getRequest(RestRequest request) throws IOException
7879

7980
return new MLAddMemoriesRequest(mlAddMemoryInput);
8081
}
81-
82-
private static void ensureExpectedToken(XContentParser.Token expected, XContentParser.Token actual, XContentParser parser) {
83-
if (actual != expected) {
84-
throw new IllegalArgumentException("Expected token [" + expected + "] but found [" + actual + "]");
85-
}
86-
}
8782
}

‎plugin/src/test/java/org/opensearch/ml/action/memorycontainer/memory/MemoryOperationsServiceTests.java‎

Lines changed: 70 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@
55

66
package org.opensearch.ml.action.memorycontainer.memory;
77

8+
import static org.junit.Assert.assertEquals;
9+
import static org.junit.Assert.assertTrue;
810
import static org.mockito.ArgumentMatchers.any;
911
import static org.mockito.Mockito.doAnswer;
1012
import static org.mockito.Mockito.mock;
@@ -20,6 +22,7 @@
2022

2123
import org.junit.Before;
2224
import org.junit.Test;
25+
import org.mockito.ArgumentCaptor;
2326
import org.mockito.Mock;
2427
import org.mockito.MockitoAnnotations;
2528
import org.opensearch.action.bulk.BulkItemResponse;
@@ -253,7 +256,73 @@ public void testExecuteMemoryOperations_NoneDecision() {
253256

254257
memoryOperationsService.executeMemoryOperations(decisions, indexName, sessionId, user, input, storageConfig, operationsListener);
255258

256-
verify(operationsListener).onResponse(any(List.class));
259+
// Verify that NONE events result in an empty response list (no operations to execute)
260+
ArgumentCaptor<List<MemoryResult>> resultsCaptor = ArgumentCaptor.forClass(List.class);
261+
verify(operationsListener).onResponse(resultsCaptor.capture());
262+
List<MemoryResult> results = resultsCaptor.getValue();
263+
assertTrue(results.isEmpty());
264+
}
265+
266+
@Test
267+
public void testExecuteMemoryOperations_MixedDecisionsExcludesNone() {
268+
// Create mixed decisions: ADD, UPDATE, DELETE, and NONE
269+
MemoryDecision addDecision = mock(MemoryDecision.class);
270+
when(addDecision.getEvent()).thenReturn(MemoryEvent.ADD);
271+
when(addDecision.getText()).thenReturn("New fact");
272+
273+
MemoryDecision updateDecision = mock(MemoryDecision.class);
274+
when(updateDecision.getEvent()).thenReturn(MemoryEvent.UPDATE);
275+
when(updateDecision.getId()).thenReturn("memory-2");
276+
when(updateDecision.getText()).thenReturn("Updated fact");
277+
when(updateDecision.getOldMemory()).thenReturn("Old fact");
278+
279+
MemoryDecision deleteDecision = mock(MemoryDecision.class);
280+
when(deleteDecision.getEvent()).thenReturn(MemoryEvent.DELETE);
281+
when(deleteDecision.getId()).thenReturn("memory-3");
282+
when(deleteDecision.getText()).thenReturn("Deleted fact");
283+
284+
MemoryDecision noneDecision = mock(MemoryDecision.class);
285+
when(noneDecision.getEvent()).thenReturn(MemoryEvent.NONE);
286+
when(noneDecision.getId()).thenReturn("memory-4");
287+
when(noneDecision.getText()).thenReturn("Unchanged fact");
288+
289+
List<MemoryDecision> decisions = Arrays.asList(addDecision, updateDecision, deleteDecision, noneDecision);
290+
String indexName = "memory-index";
291+
String sessionId = "session-123";
292+
User user = null;
293+
MLAddMemoriesInput input = mock(MLAddMemoriesInput.class);
294+
when(input.getAgentId()).thenReturn("agent-123");
295+
when(input.getTags()).thenReturn(new HashMap<>());
296+
MemoryStorageConfig storageConfig = mock(MemoryStorageConfig.class);
297+
298+
BulkResponse bulkResponse = mock(BulkResponse.class);
299+
when(bulkResponse.hasFailures()).thenReturn(false);
300+
BulkItemResponse addItem = mock(BulkItemResponse.class);
301+
when(addItem.getOpType()).thenReturn(org.opensearch.action.DocWriteRequest.OpType.INDEX);
302+
when(addItem.isFailed()).thenReturn(false);
303+
when(addItem.getId()).thenReturn("new-memory-id");
304+
when(bulkResponse.getItems()).thenReturn(new BulkItemResponse[] { addItem });
305+
306+
doAnswer(invocation -> {
307+
ActionListener<BulkResponse> listener = invocation.getArgument(1);
308+
listener.onResponse(bulkResponse);
309+
return null;
310+
}).when(client).bulk(any(), any());
311+
312+
memoryOperationsService.executeMemoryOperations(decisions, indexName, sessionId, user, input, storageConfig, operationsListener);
313+
314+
// Verify that only ADD, UPDATE, DELETE are included in results (not NONE)
315+
ArgumentCaptor<List<MemoryResult>> resultsCaptor = ArgumentCaptor.forClass(List.class);
316+
verify(operationsListener).onResponse(resultsCaptor.capture());
317+
List<MemoryResult> results = resultsCaptor.getValue();
318+
319+
// Should have 3 results (ADD, UPDATE, DELETE) but not NONE
320+
assertEquals(3, results.size());
321+
322+
// Verify the events in the results
323+
assertEquals(MemoryEvent.ADD, results.get(0).getEvent());
324+
assertEquals(MemoryEvent.UPDATE, results.get(1).getEvent());
325+
assertEquals(MemoryEvent.DELETE, results.get(2).getEvent());
257326
}
258327

259328
@Test

0 commit comments

Comments
 (0)