Use RDBMS specific queries updating session attributes

[candrews]

Inserting new attributes for the same session can result in a duplicate key exceptions. Using RDMS specific upsert queries solves this problem.

Resolves: #1213

[vpavic]

Thanks for the PR @candrews but I'm not keen to add this kind of complexity to JDBC repo. I've got strong preference for sticking with standard SQL here. For advanced use cases there's an option of customizing SQL, which you already do. Besides complexity, having custom SQL OOTB is just too risky testing-wise as we cannot test all the databases - for instance, while we do have an integration test for Oracle, we cannot run it as a part of our build due to restrictions around Oracle JDBC driver.

Also note that this is only a workaround for #1213 because as explained in that issue the core problem lies in how sessions are consumed. As such, it doesn't really justify the complexity presented here.

[candrews]

I've got strong preference for sticking with standard SQL here.

MERGE is part of the sql standard - we're just using it. If by "standard" you mean "widely supported," then yes - you have a point. But database specific code is nothing new - there are already per-database script distributed as part of Spring Session.

Besides complexity, having custom SQL OOTB is just too risky testing-wise as we cannot test all the databases

Which is why I've written this PR so that it falls back to the current behavior for all other databases. For example, H2 falls back to the basic INSERT query. Therefore, only the databases that have specific SQL need to be tested, which are MySQL, PostgreSQL, DB2, Oracle, and Microsoft SQL Server. Of those, all have automated integration tests already except for DB2 and Oracle. I'm quite willing to drop DB2; I just included it for completeness. Oracle I can assert that I've tested, and that's the best I can do - unfortunately, as you said, licensing for Oracle is problematic. Perhaps you could manually run the tests?

Also note that this is only a workaround for #1213 because as explained in that issue the core problem lies in how sessions are consumed. As such, it doesn't really justify the complexity presented here.

I don't agree that this is a workaround - this is the solution. Applications create attributes and they expect that to work (just see #1213 for users that agree with my perspective); currently, Spring Session doesn't always work. Asking all applications to change their implementation (in a complex way) isn't great - it's much better to have Spring Session JDBC support the approach that applications already take and meet the exceptions they already in alignment with other session implementations (including every application server and other Spring Session implementations).

Could you please reconsider?

[vpavic]

MERGE is part of the sql standard - we're just using it. If by "standard" you mean "widely supported," then yes - you have a point. But database specific code is nothing new - there are already per-database script distributed as part of Spring Session.

Indeed I used term SQL standard to refer to widely adopted level of SQL standard, sorry for the confusion there. Schema scripts are not really in the same ballpark, as they're not truly normative but rather simply provided for convenience/guidance.

Which is why I've written this PR so that it falls back to the current behavior for all other databases.

I understand that, but it adds complexity that we really don't want to deal with in Spring Session. I'm sure there are other users with their own cases for database specific SQL in other places so if we add such behavior in one place, there will surely be requests for other places as well.

There's simply isn't any pressing need to add this complexity, as customization of SQL is already possible. I agree that it isn't ideal since it requires you to manually declare JdbcOperationsSessionRepository bean, but we'll try to improve things for 2.2 in #1199.

For the most optimal solution to leverage capabilities of your RDBMS, I'd recommend going custom SessionRepository route - this could be quite beneficial in case of database like Oracle. You could publish it as a 3rd party extension and we'll link it in our docs. I've had a similar idea for PostgreSQL for a while now but never had time to realize it. Alternatively, perhaps a jOOQ based SessionRepository implementation could also be an option.

Of those, all have automated integration tests already except for DB2 and Oracle. I'm quite willing to drop DB2; I just included it for completeness. Oracle I can assert that I've tested, and that's the best I can do - unfortunately, as you said, licensing for Oracle is problematic. Perhaps you could manually run the tests?

I'm afraid anything manual is just to risky. Regarding DB2, I've just added integration tests yesterday.

I don't agree that this is a workaround - this is the solution.

We'll have to disagree there, because as explained in #1213, the original problem is that concurrent requests operate on the same attribute overwriting each other - his just makes the request that lost race condition (and threw an exception) to win the race condition.

To summarize:

• we'd like to avoid SQL dialect handling in Spring Session
• we'll improve configuration capabilities for custom SQL
• integration testing isn't possible for Oracle in the same manner as it is for most other databases, which is a huge risk for any Oracle specific SQL
• the original race condition problem still exists

[candrews]

• integration testing isn't possible for Oracle in the same manner as it is for most other databases, which is a huge risk for any Oracle specific SQL

That is bad.... but maybe it can be made a bit better.
Oracle offers the OJDBC drivers in their maven repository, but you need to authenticate. We could store Oracle credentials in an encrypted Travis CI variable, and have Spring Session's tests depend on OJDBC and get it from Oracle's maven repository only if that variable is set.
Users would only be able to run the Oracle tests if they also set the variable on their systems, but at least Spring Session's Travis CI would run them automatically.
I could prepare a PR to this.

[vpavic]

Thanks for the offer @candrews, but again that kind of complexity is what we'd like to avoid.

[shark300]

I've got a "bad SQL grammar" exception on Oracle 18c if the SQL query has an ending semicolon.

(Yes, I know it's a declined PR, but we're using this as a workaround until the final solution.)

[candrews]

Since this PR was declined, GitHub doesn't seem to be updating it when I push to the branch for it.

I had fixed this a while ago in my branch, so I suggest you grab it from there: https://github.com/candrews/spring-session/tree/attributes-duplicate-key