jdbi / jdbi

The Jdbi library provides convenient, idiomatic access to relational databases in Java and other JVM technologies such as Kotlin, Clojure or Scala.
http://jdbi.org/
Apache License 2.0
1.97k stars 341 forks source link

[MSSQL] Snapshot transaction isolation level cannot be used with `inTransaction` methods #2651

Open mjadczak opened 7 months ago

mjadczak commented 7 months ago

The Microsoft SQL Server JDBC driver defines a custom value for the SNAPSHOT transaction level which is unique to SQL Server.

Because the methods on JDBI and Handle objects which allow setting an isolation level, like inTransaction, require a parameter of type TransactionIsolationLevel, which is an enum defined directly by JDBI, this special value cannot be passed in, and we must instead set the default isolation level on the DataSource, or manually manage the handle's lifetime and use the setTransactionLevel override which allows specifying an int, neither of which are ideal.

Some immediate ideas for solutions are:

stevenschlansker commented 7 months ago

Thanks for reporting this.

stevenschlansker commented 3 months ago

Hi @mjadczak , do you have some time to look at https://github.com/jdbi/jdbi/pull/2684 and confirm if it would help?

mjadczak commented 3 months ago

Thank you for looking at this! Just looking at the API-level change, I agree this solves the issue.

I will try and build locally to verify this works sometime this week, but I note that the CI build is failing on the PR, so I'm not sure if I'll be able to?

stevenschlansker commented 3 months ago

Yes, I need to sort that out. This is technically a breaking change since the isolation level is no longer an Enum. I am hoping that the real-world breakage is small, since I doubt many / anyone has used custom isolation levels before.

@hgschmie , do you have thoughts on this?

stevenschlansker commented 3 months ago

Darn, this might be harder to sort out than I thought. In order to be a valid annotation attribute it must remain an Enum type. However, and Enum type can never be extended. So I think the current approach will not work :(

mjadczak commented 3 months ago

Is there a particular reason to enforce that the isolation level is strongly typed, when the underlying JDBC call is not?

Would it maybe be easier to add overrides to the annotations and functions to accept an int instead of the enum for these custom cases? Then perhaps there could be some internal DefinedTransactionIsolationLevel interface that is implemented by 1) the TransactionIsolationLevel enum 2) some internal CustomTransactionIsolationLevel class which wraps a user-passed int so that the changes to places where the enum is currently used are minimal? (I suppose an alternative would be to allow / require the user to implement the interface for custom levels, and take an instance in the functions and a Class in the annotations)

stevenschlansker commented 3 months ago

The reason for an Enum is as far as I can tell simply so you get sensible autocomplete - if the type is int then the IDE has no clue, but if it is an Enum you can get all the choices at your fingertips.

Yes, I did a spike into adding the int overrides. This works great except in one place: the TransactionHandler interface takes the Enum type, not an int, and is expected that users may extend this.

Given the other constraints, the only choice might be to add a new TransactionHandler type that takes the int value and deprecate the old one, which isn't great. Would love to hear better ideas.