Enable transaction management for JdbcOperationsSessionRepository operations

[vpavic]

This PR adds option to enable transactional execution of write operations in JdbcOperationsSessionRepository.

Transactional execution is enabled by using @EnableJdbcHttpSession(transactional = true) and providing PlatformTransactionManager or , in case of manual configuration, by setting PlatformTransactionManager directly on JdbcOperationsSessionRepository using appropriate setter method. Transactions will be executed using propagation REQUIRES_NEW.

This change fixes #446 and #459.

@rwinch please review. If you're OK with this approach I'll proceed to update the PR documentation updates.

I've signed the CLA.

[rwinch]

@vpavic Thanks for the PR!

I think I'd prefer to allow the user to create the transactional boundary by exposing a bean of a particular name.

• This aligns with Spring Session's other points of customization. For example, this is how we users can provide custom serializers.
• Additionally, using a bean provides more flexibility for the user than the boolean flag on the annotation (which can only be on or off).

What are your thoughts?

[vpavic]

@rwinch I see your point here - you'd prefer to take a less opinionated route on how to approach the transaction management within the JdbcOperationsSessionRepository, and instead of focusing on providing concrete solutions for detected problems with transactions we'd equip the users with flexiblity (by setting their own TransactionOperations implementation) so they can come up with their own prefered solution.

I guess this makes sense, and it actually makes the job quite easier for us in terms of implementation as well as testing. Documentation should provide some concrete examples for this approach.

Do you prefer to keep offer this only for write operations, or read ones as well?

[rwinch]

@vpavic Thanks for the fast reply. I'd say we should do transactions or reads also.

[vpavic]

@rwinch I've updated the PR.

Please share your thoughts. Documentation is still pending.

[kherrala]

I would still prefer using REQUIRES_NEW propagation behaviour as a sane default, because I would personally expect this to "just work" without extra configuration. My previous suggestion was to avoid introducing unnecessary configuration for the time being, but the behaviour on rollback is not quite the same with session persist after succesful commit.

[vpavic]

@kherrala FWIW I agree with you on REQUIRES_NEW being the sane default.

However @rwinch has so far insisted on opt-in approach for transactional behavior, with argument that transactions as an expensive operation might not be needed/wanted by all users.

[rwinch]

@kherrala @vpavic Thanks for your feedback.

I've been doing some more thinking about this and I think you two have convinced me that my approach was not ideal. We should work out of the box, so I think you two are right that we should create the transactions by default.

What's more is we can always make it optional later on and still remain passive. If we start without requiring it and it causes problems, we are stuck with this default to remain passive.

@vpavic I'm sorry on flip-flopping on this, but hopefully getting to the "right answer" is some consultation. Do you mind updating the PR to take these changes into account.

[vpavic]

I'm sorry on flip-flopping on this, but hopefully getting to the "right answer" is some consultation.

@rwinch I'm always in favor of thinking things through a bit more even at the expense of delaying things so 👍 on that from my side.

No problems on updating the PR, I'll take care of that soon.

Now that we've decided what the default behavior is, the question is what means do we provide for changing the defaults?

What's more is we can always make it optional later on and still remain passive. If we start without requiring it and it causes problems, we are stuck with this default to remain passive.

If I'm reading this correctly you wouldn't provide an option for users to disable transactions right now, but rather at some later stage if there's need for this?

[rwinch]

so 👍 on that from my side.

Glad to hear that :)

No problems on updating the PR, I'll take care of that soon.

Awesome! Thanks :)

If I'm reading this correctly you wouldn't provide an option for users to disable transactions right now, but rather at some later stage if there's need for this?

Correct

[vpavic]

@rwinch I've updated the PR.

[rwinch]

Thanks for the PR! This is now merged into master