Skip to content

Conversation

eeaters
Copy link
Contributor

@eeaters eeaters commented Sep 2, 2025

The inner method JdbcChatMemoryRepositoryDialect#from already contains exception handling logic. The outer try-catch block is unnecessary and can be safely removed to avoid redundant exception handling and improve code clarity.

The inner method JdbcChatMemoryRepositoryDialect#from already contains
exception handling logic. The outer try-catch block is unnecessary and
can be safely removed to avoid redundant exception handling and improve
code clarity.

Signed-off-by: eeaters <870905780@qq.com>
@sunyuhan1998
Copy link
Contributor

sunyuhan1998 commented Sep 2, 2025

Thank you for your contribution! However, I think the current approach of silently ignoring exceptions in the JdbcChatMemoryRepositoryDialect#from method is not ideal. Since the detection of the JDBC database vendor currently relies on establishing a connection and reading metadata, suppressing exceptions and falling back to a default dialect may mislead users and make issues harder to diagnose. A better approach would be to properly report the underlying problem to the user. In that case, the outer try-catch blocks mentioned in this issue would serve a meaningful purpose. To address this, I've opened a PR (#4290 ) with the proposed changes, it also includes some other optimizations.—what do you think?

@eeaters
Copy link
Contributor Author

eeaters commented Sep 2, 2025

Yes; I just had doubts about the default return of PostgresChatMemoryRepositoryDialect while reading the code here. In comparison, I think directly throwing an exception internally would be more acceptable to me; or maybe the previous person was using a PostgreSQL database. Haha.

@eeaters
Copy link
Contributor Author

eeaters commented Sep 2, 2025

While reading the code, I noticed the try-catch block, but I found that the method does not throw any exceptions internally. This made me reread the code repeatedly. Therefore, I think it might be better to either remove the exception handling (with adjustments made later if necessary) or simply avoid using try-catch here for now.

I saw your commit; you changed the code style, but the default behavior still returns the PostgreSQL database. Shouldn’t we just keep it consistent?

@sunyuhan1998
Copy link
Contributor

While reading the code, I noticed the try-catch block, but I found that the method does not throw any exceptions internally. This made me reread the code repeatedly. Therefore, I think it might be better to either remove the exception handling (with adjustments made later if necessary) or simply avoid using try-catch here for now.

I saw your commit; you changed the code style, but the default behavior still returns the PostgreSQL database. Shouldn’t we just keep it consistent?

Sorry for the confusion. Actually, in my initial commits, I did follow the approach you suggested—propagating the connection exception upward. However, this approach failed the JdbcChatMemoryRepositoryBuilderTests unit tests. That made me realize that some existing tests (and possibly some users) are already relying on the default fallback to PostgresChatMemoryRepositoryDialect. To avoid introducing a potentially breaking change, I later adjusted the implementation to simply log a warning instead of throwing an exception—hopefully making it more acceptable while preserving backward compatibility. So, given the current state, you're absolutely right: there is no conflict between these two PRs.

@sobychacko sobychacko added this to the 1.1.0.M1 milestone Sep 3, 2025
@sobychacko sobychacko merged commit eda3c74 into spring-projects:main Sep 3, 2025
2 checks passed
spring-builds pushed a commit that referenced this pull request Sep 3, 2025
The inner method JdbcChatMemoryRepositoryDialect#from already contains
exception handling logic. The outer try-catch block is unnecessary and
can be safely removed to avoid redundant exception handling and improve
code clarity.

Fixes: #4288

Signed-off-by: eeaters <870905780@qq.com>
(cherry picked from commit eda3c74)
scionaltera pushed a commit to scionaltera/spring-ai that referenced this pull request Sep 3, 2025
…ts#4288)

The inner method JdbcChatMemoryRepositoryDialect#from already contains
exception handling logic. The outer try-catch block is unnecessary and
can be safely removed to avoid redundant exception handling and improve
code clarity.

Fixes: spring-projects#4288
Auto-cherry-pick to 1.0.x

Signed-off-by: eeaters <870905780@qq.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants