spring-projects
fix(JdbcChatMemory): get query for MSSQL Server
Fix the GET query for Microsoft SQL Server on JdbcChatMemory
I have tried to make the less changes, I was thinking at first to change with Impl classes and make a @OnConditionalClass("sql driver") but that would break too many things as it introduces the class a Component.
I hope the way I have fixed the issue fits the Spring standards, if not I will do the necessary.
Happy to contribute!
Related issue: #2807
@leijendary @sobychacko the query was creating an error when using MSSQL Server. It is now fixed.
Can you get a look? I have tried making the least breaking changes so it doesn't break for existing projects using the class
@xchopin 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.
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
@ThomasVitale I have noticed you merged this PR https://github.com/spring-projects/spring-ai/pull/2781 today even tho the JdbcChatMemory is deprecated. Should I still continue my fix for MSSQL for this class or no?
cc @linarkou
@xchopin thanks for your contribution! The deprecated JdbcChatMemory API has been removed in https://github.com/spring-projects/spring-ai/pull/3002, but we can add support for MSSQL to JdbcChatMemoryRepository.
Since MSSQL requires a different query (see https://github.com/spring-projects/spring-ai/pull/2806/files#diff-af27045980fb458706f33f2a0e23f6688b1f6c9ed46abb3d8cdb5f5c1cc6d234R76), I wonder if we should address the current problem of the JdbcChatMemoryRepository which doesn't allow customisations of the queries, as also pointed out in https://github.com/spring-projects/spring-ai/issues/2974 We might introduce some sort of QueryProvider interface to pass the SQL queries to JdbcChatMemoryRepository. And then we could have a MsSqlQueryProvider implementation for when we want to use MSSQL.
What do you think @markpollack?
The current state is certainly going to violate the principle of least surpise given the name JdbcChatMemoryRepository, so we should try to think of something to support other DB providers other than the postgres syntax.
I do like that things are simple now in terms of a few SQL statements to customize, but not sure how that works out in practice. Also, everyone wants control to name their own tables and rows, so there is a long tail to customization as well. The default table name also doesn't align with the default table name in vector stores which has spring_ai prefix.
not sure of what is a pragmatic way forward with a small time frame. will research a bit and post back.
I've made this PR that I hope will take into account all the various issues that have been raised around more flexible support for multiple databases.
https://github.com/spring-projects/spring-ai/pull/3055
closing this one now so we can concentrate on that PR. Your PR was very helpful! thanks @xchopin