Null behaviour of begin method

[hmottestad]

The begin method in the RepositoryConnection specifically states that it handles null as an isolation level, while the same method in the SailConnection does not say this.

If the begin method in the SailConnection should handle null it should be:

• documented
• tested

We should also consider making "getDefaultIsolationLevel" a required method in the Sail, since it would then be required to be able to support nulls.

We should also remove any unneeded null handling, like in the SchemaCachingRDFSInferencerConnection where we check if the isolation level is null, even though we could just pass null to super.begin.

[abrokenjester]

My current take on this is that I agree it should be documented that it accepts null, and that if null is supplied, the Sail should fall back to its default isolation level. This would also be easy to write a simple test for (the SailIsolationLevelTest class would be a good spot for it I guess).

I'm not sure what you mean with making getDefaultIsolationLevel a required method - it's defined right in the Sail interface so it currently already is a required method.

[hmottestad]

I think it shouldn't take null.

[abrokenjester]

I think it shouldn't take null.

Why is that?

[hmottestad]

Because we have two begin methods. One that takes an isolation level and one that doesn't. It makes a lot more sense to require an isolation level when there is a method specifically designed to handle the cases where you don't have an isolation level.

[hmottestad]

Furthermore, starting a transaction explicitly with null as the isolation level should be equivalent to specifying NONE, since null feels a lot more like NONE than it does "unknown" in Java (unlike SQL).

[hmottestad]

Alternatively we deprecate the begin() method (without any arguments) so that we in the future are left with only one begin method in the sail, but multiple in the repository api signaling that the repository api is targeted at users while the sail api is targeted at implementers and is thus as concise as possible.

I actually prefer this approach to be honest.

[abrokenjester]

Because we have two begin methods. One that takes an isolation level and one that doesn't. It makes a lot more sense to require an isolation level when there is a method specifically designed to handle the cases where you don't have an isolation level.

To be honest if we were designing this from scratch I would probably agree with you that a null-check is more elegant, but it's been implemented for quite a while now to accept null as an argument. My concern is mostly that changing the (admittedly implicit) expectation to one where null is not accepted as an argument can lead to errrors in third party code.

As an aside: the reason we have two begin methods is itself a backward compatibility issue: transaction isolation levels were introduced much later than the notion of transactions itself.

Anyway, at the end of the day I'm fine with either approach. We can go ahead and make non-null argument mandatory at the sail level, and if someone complains about their code suddenly breaking we can point out that they were relying on undocumented behavior (so we can argue that it's not a backward incompatible change). I don't know how likely it is that this will happen, just be prepared to not make friends if it does occur.

As for deprecating begin() in the SailConnection: I don't mind, and the 'minimal internal interface' argument makes sense. On the other hand it's properly documented what isolation level it uses, it's not really in our way and we can easily implement a default method in AbstractSailConnection that properly handles it, so sail implementors don't have to override. So while I'm fine with deprecating it and avoiding using it ourselves, I'm not in any rush to remove it in the next major.

[hmottestad]

I'm actually thinking we should both deprecate the empty begin method and at the same time remove support for null in the begin method with isolation level. At the same time we should also introduce a getDefaultIsolationLevel method to the SailConnection. That way there is no way for an implementer to start a transaction without specifying an isolation level.

[abrokenjester]

I think that's what I suggested in the end as well. Except getDefaultisolationlevel already exists. At the sail level. And I don't see any benefit to moving it to the connection.