Removed deprecated method in ChatMemory and Refactor Memory Advisors

[markpollack]

Removed Deprecated chatmemory method.

Updated to reduce duplication via template methods and add extensive tests

Core Architecture Changes:

1. New Abstract Class: AbstractConversationHistoryAdvisor

   • Created a new abstract class that extends AbstractChatMemoryAdvisor
   • Provides common functionality for managing conversation history
   • Implements methods for retrieving messages and applying them to requests
   • Simplifies the implementation of concrete advisors

2. Refactored AbstractChatMemoryAdvisor

   • Removed the defaultChatMemoryRetrieveSize parameter from constructors
   • Enhanced the builder pattern with generic type parameters for better type safety
   • Added abstract before(ChatClientRequest, String) method for subclasses to implement
   • Improved logging for conversation ID handling

3. Refactored MessageChatMemoryAdvisor

   • Now extends AbstractConversationHistoryAdvisor instead of directly extending AbstractChatMemoryAdvisor
   • Simplified implementation by leveraging parent class methods
   • Fixed handling of multiple user messages in a single prompt

4. Refactored PromptChatMemoryAdvisor

   • Now extends AbstractConversationHistoryAdvisor instead of directly extending AbstractChatMemoryAdvisor
   • Fixed handling of multiple user messages by using getUserMessages() instead of getUserMessage()
   • Enhanced logging for better debugging

Test Changes:

1. New Test Classes

   • Added MessageChatMemoryAdvisorIT for integration testing of MessageChatMemoryAdvisor
   • Added PromptChatMemoryAdvisorIT for integration testing of PromptChatMemoryAdvisor
   • Both extend from AbstractChatMemoryAdvisorIT to share common test logic

2. Test Coverage

   • Added tests for handling multiple user messages in a single prompt
   • Added tests for custom conversation IDs
   • Added tests for maintaining separate conversations
   • Added tests for reactive mode operation

Key Improvements:

1. Code Duplication Reduction: Moved common functionality to the parent class
2. Bug Fix: Fixed a bug in PromptChatMemoryAdvisor where it was only storing the last user message
3. Better Logging: Added detailed logging for better debugging and traceability
4. Simplified API: Removed unnecessary parameters from constructors
5. Improved Test Coverage: Added comprehensive tests for various scenarios

[markpollack]

I think the builder maybe doesn't need to be implemented in that manner. I had some design with additional type parameters in the class for arbitrary parameters one could pass into the advisors

[ThomasVitale]

I didn't fully understand the goal of this new class. In any case, I would use a different name because we're dealing with chat memory and not chat history, which is something different (for convenience, I described the difference in the docs https://docs.spring.io/spring-ai/reference/api/chat-memory.html)

[ThomasVitale]

I'm also wondering about whether we need all these abstract classes. We have a BaseAdvisor interface takes care of the blocking/non-blocking bit, so at least we can inherit from there (similar to what we did in QuestionAnswerAdvisor) and only have to implement before() and after().

Having the same abstraction for vector store (dealing with text) and memory store (dealing with messages) might also be unnecessary. Since this is a breaking change, we might take a chance to review the design of this sets of advisors.

[ThomasVitale]

Could we do the following to simplify them all?

• MessageChatMemoryAdvisor implements BaseAdvisor
• PromptChatMemoryAdvisor implements BaseAdvisor
• VectorStoreChatMemoryAdvisor implements BaseAdvisor

If needed, a BaseChatMemoryAdvisor interface (not an abstract class), implementing BaseAdvisor, could be introduced for sharing some logic between MessageChatMemoryAdvisor and PromptChatMemoryAdvisor since they both deal with messages. That would give the same flexibility for customisation, but it would make it easier for us to evolve those classes rather than having abstract/protected methods.

I would leave VectorStoreChatMemoryAdvisor on its own since it works very differently (embeddings instead of messages).

[markpollack]

Ok, thanks, let me look into that. I was focused on removing duplication and having something more formal in the design to separate the message based retrieval MemoryAdvisors as compared to the VectorStore one that doesn't convert from Document to Messages.

[markpollack]

I've opened up #3121 There is still the need for an abstract base class, AbstractChatMemoryAdvisor but now that implements BaseAdvisor. All three implementations inherit from that. The Builder class has been improved and tests added.

[ThomasVitale]

The previous logic here was a bit strange: storing only the latest user message in the input list.

Having all user messages from the input messages as done here is an improvement, but as a developer it would still surprise me. I'd expect having all input messages stored in the ChatMemory and not just some of them.

The ChatMemory implementation contains logic to deal with SystemMessages as well. So it would support correctly storing the entire list of input messages.

[markpollack]

The test AbstractChatMemoryAdvisorIT.testMultipleUserMessagesInPrompt is what uncovered this issue.

in short

		ChatClient chatClient = ChatClient.builder(chatModel).defaultAdvisors(advisor).build();

		// Create a prompt with multiple user messages
		List<Message> messages = new ArrayList<>();
		messages.add(new UserMessage("My name is David."));
		messages.add(new UserMessage("I work as a software engineer."));
		messages.add(new UserMessage("What is my profession?"));

		Prompt prompt = new Prompt(messages);

		String answer = chatClient.prompt(prompt)
			.advisors(a -> a.param(AbstractChatMemoryAdvisor.CHAT_MEMORY_CONVERSATION_ID_KEY, conversationId))
			.call()
			.content();

		logger.info("Answer: {}", answer);
		assertThat(answer).containsIgnoringCase("software engineer");

		List<Message> memoryMessages = chatMemory.get(conversationId);
		assertThat(memoryMessages).hasSize(4); // 3 user messages + 1 assistant response

I would expect all 3 user messages to be stored. What do you think?

[markpollack]

Closing in favor of PR #3121