fix(JdbcChatMemory): get query for MSSQL Server #2806

xchopin · 2025-04-19T03:07:56Z

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

xchopin · 2025-04-19T03:12:18Z

@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

Signed-off-by: Xavier Chopin <[email protected]>

leijendary

At this point, I think using spring-data-jdbc would be better for a cleaner solution since more drivers with different query syntax needs to be supported.

leijendary · 2025-04-19T17:35:13Z

...ork/ai/model/chat/memory/jdbc/autoconfigure/JdbcChatMemoryAutoConfigurationPostgreSQLIT.java


 import static org.assertj.core.api.Assertions.assertThat;

 /**
 * @author Jonathan Leijendekker
 */
 @Testcontainers
-class JdbcChatMemoryAutoConfigurationIT {
+class JdbcChatMemoryAutoConfigurationPostgreSQLIT {


It would be preferable to name it JdbcChatMemoryAutoConfigurationPostgresIT

leijendary · 2025-04-19T17:36:23Z

...model/chat/memory/jdbc/autoconfigure/JdbcChatMemoryDataSourceScriptDatabasePostgreSQLIT.java


 import static org.assertj.core.api.Assertions.assertThat;

 /**
 * @author Jonathan Leijendekker
 */
 @Testcontainers
-class JdbcChatMemoryDataSourceScriptDatabaseInitializerTests {
+class JdbcChatMemoryDataSourceScriptDatabasePostgreSQLIT {


It would be preferable to name this JdbcChatMemoryDataSourceScriptDatabasePostgresIT

leijendary · 2025-04-19T17:41:48Z

...l-chat-memory-jdbc/src/main/java/org/springframework/ai/chat/memory/jdbc/JdbcChatMemory.java

@@ -45,14 +45,29 @@ public class JdbcChatMemory implements ChatMemory {
 			INSERT INTO ai_chat_memory (conversation_id, content, type) VALUES (?, ?, ?)""";

 	private static final String QUERY_GET = """
-			SELECT content, type FROM ai_chat_memory WHERE conversation_id = ? ORDER BY "timestamp" DESC LIMIT ?""";
+		SELECT content, type \


Why is this changed to multiline with a backslash? Also, utilise the proper alignment for closing the text blocks. Reference: Text Blocks

I switched it to multiline to keep it consistent with the new query.

My bad I thought without the backslash it would add extra spaces

leijendary · 2025-04-19T17:45:54Z

...l-chat-memory-jdbc/src/main/java/org/springframework/ai/chat/memory/jdbc/JdbcChatMemory.java

+		LIMIT ?
+	""";
+
+	private static final String MSSQL_QUERY_GET = """


Same as previous comment regarding utilising text block alignments; you can move the string content 1 tab to the right.

ORDER BY should be DESC since we're querying lastN not the first n. Refer to Fixed message order for JDBC Chat Memory #2781 for the additional fix.

ok, I will fix it

putting DESC actually breaks the test and returns the list in a backward side, so either the one in Postgres is broken or there is something weird between the two behaviors

I am going to check the link thank you

leijendary · 2025-04-19T17:49:43Z

...-jdbc/src/test/java/org/springframework/ai/chat/memory/jdbc/JdbcChatMemoryMSSQLServerIT.java

+			chatMemory.add(conversationId, message);
+
+			var jdbcTemplate = context.getBean(JdbcTemplate.class);
+			var query = "SELECT conversation_id, content, type, \"timestamp\" FROM ai_chat_memory  WHERE conversation_id = ?";


Isn't the timestamp column going to fail here?

both "timestamp" [timestamp] works, the test succeeded when calling the method

I could add an assert to verify it contains a value

leijendary · 2025-04-19T17:49:53Z

...-jdbc/src/test/java/org/springframework/ai/chat/memory/jdbc/JdbcChatMemoryMSSQLServerIT.java

+			chatMemory.add(conversationId, messages);
+
+			var jdbcTemplate = context.getBean(JdbcTemplate.class);
+			var query = "SELECT conversation_id, content, type, \"timestamp\" FROM ai_chat_memory WHERE conversation_id = ?";


Isn't the timestamp column going to fail here?

leijendary · 2025-04-19T17:50:26Z

...y-jdbc/src/test/java/org/springframework/ai/chat/memory/jdbc/JdbcChatMemoryPostgreSQLIT.java

 */
 @Testcontainers
-class JdbcChatMemoryIT {
+class JdbcChatMemoryPostgreSQLIT {


Preferable to change name this to JdbcChatMemoryPostgresIT

leijendary · 2025-04-19T19:00:51Z

memory/spring-ai-model-chat-memory-jdbc/pom.xml

-			<groupId>org.springframework</groupId>
-			<artifactId>spring-jdbc</artifactId>
+			<groupId>org.springframework.boot</groupId>
+			<artifactId>spring-boot-starter-data-jdbc</artifactId>


Use spring-data-jdbc instead of the starter.

I will have to change org.springframework.boot.jdbc.DatabaseDriver then

@xchopin btw, it is worth considering spring-data-jdbc repositories directly instead of manual query creations at this point. I added a comment to the PR.

@leijendary is it still worth it to move to spring-data-jdbc with this incoming PR?

#2803

@xchopin Yes it is still worth it. The implementation in that PR is the same as the current implementation wherein it uses driver specific syntax. We should probably wait for that PR to be merged though as it is a conflict.

Signed-off-by: Xavier Chopin <[email protected]>

ThomasVitale · 2025-04-25T20:13:52Z

@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: #2890

xchopin · 2025-04-29T23:31:04Z

@ThomasVitale I have noticed you merged this PR #2781 today even tho the JdbcChatMemory is deprecated. Should I still continue my fix for MSSQL for this class or no?

cc @linarkou

markpollack · 2025-05-06T05:21:43Z

Thoughts @ThomasVitale ?

ThomasVitale · 2025-05-06T05:44:21Z

@xchopin thanks for your contribution! The deprecated JdbcChatMemory API has been removed in #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 #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?

markpollack · 2025-05-08T18:26:55Z

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.

markpollack · 2025-05-08T21:46:33Z

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.

#3055

closing this one now so we can concentrate on that PR. Your PR was very helpful! thanks @xchopin

xchopin changed the title ~~fix: get query for MSSQL Server~~ fix(JdbcChatMemory): get query for MSSQL Server Apr 19, 2025

xchopin force-pushed the hotfix/jdbcchatmemory-mssql branch from ec8c282 to 6ddaa46 Compare April 19, 2025 03:10

xchopin force-pushed the hotfix/jdbcchatmemory-mssql branch 6 times, most recently from 6b3840b to 2a23eca Compare April 19, 2025 16:07

fix: get query for MSSQL Server

b4f95f2

Signed-off-by: Xavier Chopin <[email protected]>

xchopin force-pushed the hotfix/jdbcchatmemory-mssql branch from 2a23eca to b4f95f2 Compare April 19, 2025 16:10

feat: simplify sql condition and add tests

44de2ba

Signed-off-by: Xavier Chopin <[email protected]>

leijendary reviewed Apr 19, 2025

View reviewed changes

refactor: rename postgres classes and remove breaklines

7125a22

Signed-off-by: Xavier Chopin <[email protected]>

xchopin closed this Apr 26, 2025

xchopin reopened this Apr 29, 2025

markpollack added the Chat Memory label May 6, 2025

ThomasVitale mentioned this pull request May 6, 2025

feat: Support custom table and column names for JdbcChatMemoryRepository #3006

Closed

markpollack added this to the 1.0.0-RC1 milestone May 8, 2025

markpollack closed this May 8, 2025

fix(JdbcChatMemory): get query for MSSQL Server #2806

fix(JdbcChatMemory): get query for MSSQL Server #2806

Uh oh!

Conversation

xchopin commented Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xchopin commented Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leijendary left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xchopin Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xchopin Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xchopin Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

leijendary Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xchopin Apr 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ThomasVitale commented Apr 25, 2025

Uh oh!

xchopin commented Apr 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markpollack commented May 6, 2025

Uh oh!

ThomasVitale commented May 6, 2025

Uh oh!

markpollack commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

markpollack commented May 8, 2025

Uh oh!

Uh oh!

xchopin commented Apr 19, 2025 •

edited

Loading

xchopin commented Apr 19, 2025 •

edited

Loading

xchopin Apr 19, 2025 •

edited

Loading

xchopin Apr 19, 2025 •

edited

Loading

xchopin Apr 19, 2025 •

edited

Loading

leijendary Apr 19, 2025 •

edited

Loading

xchopin Apr 19, 2025 •

edited

Loading

xchopin commented Apr 29, 2025 •

edited

Loading

markpollack commented May 8, 2025 •

edited

Loading