Fixed message order for JDBC Chat Memory

[linarkou]

trafficstars

This PR solves some problems with message ordering:

1. JdbcChatMemory fetches rows in DESC order, and MessageChatMemoryAdvisor get and add all messages in that order - from last to first. So messages list needs to be reversed.
2. After batch insert without specifying timestamp manually all rows have the same timestamp (because database sets current_timestamp by default). Sooo after fetching rows from database the message order is unpredictable - they have same timestamp.

[linarkou]

@leijendary @sobychacko Can you please take a look? As far as I see you were main contributor/reviewer of that feature

[leijendary]

This is good! It is probably better to remove the default value for the timestamp column to avoid unintentional current_timestamp since that generates inconsistencies.

[linarkou]

This is good! It is probably better to remove the default value for the timestamp column to avoid unintentional current_timestamp since that generates inconsistencies.

✅ Thanks for the suggestion! I've updated the code.

[linarkou]

I just found out that Cassandra ChatMemory solves same problem in different, probably more efficient way:

final AtomicLong instantSeq = new AtomicLong(Instant.now().toEpochMilli());
messages.forEach(msg -> {
	if (msg.getMetadata().containsKey(CONVERSATION_TS)) {
		msg.getMetadata().put(CONVERSATION_TS, Instant.ofEpochMilli(instantSeq.getAndIncrement()));
	}
	add(conversationId, msg);
});

What do you think, maybe we should implement same logic here?

[ThomasVitale]

@linarkou thanks so much for this improvement! We have just delivered some changes to the ChatMemory API which include the introduction of a ChatMemoryRepository API to separate the two different concerns: memory management strategy and storage mechanism. The JdbcChatMemory has now been superseded by JdbcChatMemoryRepository.

The ChatMemoryRepository.saveAll() method is meant to replace the existing memory for a given conversationId since the management is performed by a given ChatMemory implementation (e.g. removing older messages that exceed the window size). Still, there might be some issues regarding the timestamp for the messages saved in a batch job, so it would be great to get this change out before the next release. Thanks a lot for your contributions!

The "lastN" concept has been deprecated (so the DESC sorting is not needed anymore) in favour of strategy-specific ChatMemory implementation. By default, the MessageWindowChatMemory is used, which handles correctly ensuring the memory doesn't exceed a certain limit. The "lastN" option had the major issue of not considering the type of messages returned, but just returning the last N (e.g. SystemMessages might end up being excluded since they are usually the oldest, and therefore resulting in errors). I hope the new APIs can help bring more flexibility and robustness.

Upgrade Notes: https://docs.spring.io/spring-ai/reference/upgrade-notes.html#_chat_memory New Chat Memory Docs: https://docs.spring.io/spring-ai/reference/api/chat-memory.html PR with the change: https://github.com/spring-projects/spring-ai/pull/2890

[linarkou]

Applied changes for JdbcChatMemoryRepository

[ThomasVitale]

Look great, thank you!

[ilayaperumalg]

@linarkou Could you rebase and squash the commits on this PR please?

[ilayaperumalg]

@linarkou Rebased the PR and running the tests. Will be merging shortly.

[ilayaperumalg]

Squashed and merged the changes as a1db00ab1.

[linarkou]

@ilayaperumalg thank you! And sorry for being late, didn't have ability to update PR.