Auto-commit configuration support

[javornikolov]

Add support to control transaction commit/rollback

• SetAutoCommit true|false fixture
• Rollback, Commit fixtures to unify Standalone vs Flow Mode syntax and tests
• Update Documentation

This intends to resolve #383

References to another issue related to auto-commit mode: #326, #330

[MMatten]

Do you think this could / should be an option rather than a new fixture?

[MMatten]

What about initialising this to false to minimise impact on existing users test suites?

[javornikolov]

Do you think this could / should be an option rather than a new fixture?

What we currently have via setOption (and dbfit.util.Options) are passive name-value pairs of settings. For connection auto-commit - we're altering the current database connection. I'm not sure about having options with active behavior. Also the scope of Options is maybe a bit different (for FlowMode it's reset per test page; for standalone - I'm not quite sure).

Maybe we can add such option which would affect connections which are opened after the option is set.

Another alternative might be to set session option for setting various options per the session/connection.

[javornikolov]

What about initialising this to false to minimise impact on existing users test suites?

I believe it's false by default - it's set in AbstractDbEnvironment.afterConnectionEstablished.

[MMatten]

May be a change for another day but as DatabaseTest and DatabaseEnvironment extend SequenceFuxture I think it should be possible to add methods to AbstractDbEnvironmment and wrap the environment object as the system under test, instead of creating new Fixture classes. I think DbFit Java used to work this way until it hit a FitLibrary issue which might have been fixed since.

[MMatten]

Our of interest Fitsharp's version has Command Timeout option which is a similar consideration - affects session characteristics.

I guess either works fine.

[MMatten]

Hmmmm. I guess the Fixture classes are only really needed for StandAlone mode when the user could set the default environment object in DbEnvironmentFactory and then use the fixtures without having to have first used a DatavaseEnvironment fixture. In flow mode the methods could live on the Environment and be exposed via DatabaseTest's SUT object.

I think I get it now. Sorry for the rambling!

[MMatten]

I think adding new Commit and Rollback fixtures isn't necessarily the right thing to do. We already have these from DatabaseEnvironment in standalone mode.

[javornikolov]

I think adding new Commit and Rollback fixtures isn't necessarily the right thing to do. We already have these from DatabaseEnvironment in standalone mode.

Right, but invoking commit/rollback was a bit different in standalone vs flow mode and the test page couldn't work in both modes.

[MMatten]

Because the fixturing is different between the modes I think there has to be separate test pages.
Perhaps a test reorg would be better to improve reuse of test page content somehow.
Perhaps a CommonSuite (universal), FlowModeCommonSuite (common to all DBs flow mode tests) and StandAloneModeCommonSuite (common to all DB's stand alone mode tests). [I was think of something similar recently to allow us to get ConnectUsingFile tests back into the build].

[javornikolov]

Because the fixturing is different between the modes I think there has to be separate test pages.

In general, yes - there are some differences between flow and standalone mode. Often the different parts can be isolated into SetUp, TearDown and the main test page to be reused - and SetUp, TearDown are anyway not in the common suite.

In the particular case - I kind of tried to unify syntax and now the page works in both standalone and flow mode. (Transaction control is not something typical for flow mode but some edge cases are emerging).

I was think of something similar recently to allow us to get ConnectUsingFile tests back into the build]

Hmm, DbFit test connections now are now initialized via ConnectUsingFile (with exception of the embedded databases).

[MMatten]

It kind of feels like we're changing the application to suit the tests rather than build the appropriate tests.

Transaction control is the awkward case for reusing test content even within FlowMode for a single adaptor. Eg in PR #371 I had to do something a bit hacky to cater for TransctionsTest.

[javornikolov]

I'll try to see how would it look like if auto-commit support is for standalone mode only.

[MMatten]

Or what about allowing this to be set via a Database Environment fixture method, for standalone mode?

I think this is one of those cases where it's simpler to have two test pages (one for each mode).

If we take the approach of new Commit and Rollback fixtures then we end up with duplicated ways of committing/rolling back and would we start directing people to using these instead of Database Environment? That was my concern.

[javornikolov]

Or what about allowing this to be set via a Database Environment fixture method, for standalone mode?

Yes, sounds possible.

I think this is one of those cases where it's simpler to have two test pages (one for each mode).

Do you mean separate tests for autoCommit=false and for autoCommit=true? Or for standalone and flow mode (in case we add support flow mode at all)?

If we take the approach of new Commit and Rollback fixtures then we end up with duplicated ways of committing/rolling back and would we start directing people to using these instead of Database Environment? That was my concern.

Good point. On the other hand - having different syntax is causing some inconvenience when converting or sharing a test between modes. But if we move auto-commit as method in Database Environment, it's anyway for standalone mode only and no need to bother about compatibility with flow mode syntax.

[MMatten]

Also the scope of Options is maybe a bit different (for FlowMode it's reset per test page; for standalone - I'm not quite sure

I guess that could have been an oversight in the original code as there's no documentation to say otherwise.

[MMatten]

I think this is one of those cases where it's simpler to have two test pages (one for each mode).

Do you mean separate tests for autoCommit=false and for autoCommit=true? Or for standalone and flow mode (in case we add support flow mode at all)?

I meant separate test pages for Flow and StandAlone modes.

[MMatten]

Thinking about the new Commit and Rollback fixtues - I think it's a good enhancement. We just need to document the new alternative ways of committing and rolling back in StandAlone mode.

May be we should push this even further some time by creating a standalone Connect fixture too.

[MMatten]

For the Set Auto Commit option this still feels more like a |Set Option|Auto Commit|true/false| to me but that's only a personal preference anyway.

[javornikolov]

I'll rebase on top of master to pick the refactored Options class. I'll try to implement that the feature as an option |setOption|autocommit|. (Seems that would having SetOption as a fixture).

[MMatten]

Or I guess we can add thin methods to both DatabaseTest and DatabaseEnvironment and use SequenceFixture's feature of handling these like fixtures.

Adding a new SetOption fixture would allow users to not require using a DatabaseEnvironment fixture at all in StandAlone mode (once they'd created the default environment in DbEnvironmentFactory's instance). I wonder if anyone actually uses DbFit like that though.

[MMatten]

More thoughts...If this was a new (passive) option (instead of modifying the connection immediately) then that would mean we'd really have to centralise all statement execution into AbstractDbEnvironment and set the auto commit before every execution. I guess that could be a larger change.

[MMatten]

Ok, so I think I've finally come around to your original idea. May be an active option isn't best for this. An external process, in stand alone mode, might modify the auto commit property anyway and the user might expect an option value to be in effect until they explicitly change it via another Set Option.

Then again...may be that would just need to be documented.

Then again....does anyone actually use standalone mode like this!?

May be I'm over complicating this all.

What are your current thoughts?

[javornikolov]

I just pushed ability to set this via Set Option (without removing the original SetAutoCommit fixture). Tests are not updated yet.
We can experiment a bit and see. The main point in SetOption version is to allow setting it before connection - to "true", "force" or "auto" mode (I think some databases don't support calling setAutoCommit, e.g. Informix).

But I'm facing an issue with autoCommit in Derby (rollback after insert in auto-commit mode somehow roll-backs what has been inserted).

[javornikolov]

The problematic case (in standalone mode):

!|SetOption|autocommit|true|

!|Execute Ddl|create table TESTTBL(ID int, VAL int)|

!|Commit|

!|Insert|TESTTBL|
|ID     |VAL    |
|1      |2      |

!|Rollback|

!|Query|select ID, VAL from TESTTBL|
|ID     |VAL    |
|1      |2      | -- reported as missing

[MMatten]

Does the data even get committed at all, ever? I mean if you remove the rollback does it get committed at the end of the connection. Can you run the DB as a server instead of embedded and does that give a different result?

[javornikolov]

If there is an explicit commit after the insert - then it's committed and preserved after the rollback.

[MMatten]

Does moving the commit after the DDL to be after the insert change anything? Probably not but I was wondering if an explicit commit is setting auto commit to false.

[javornikolov]

I also updated the transaction control docs a bit about autocommit mode.

@MMatten, @benilovj - any thoughts whether we need anything else to merge this PR?

[MMatten]

If we replace SequenceFixture methods with Fixtures won't we break standalone mode tests that include things like: -

|Database Environment|
|Commit|
|Close|

(I have lots of test that use this construct)

or even

|Database Environment|
|Set Option|fixed length string parsing|true|

(I use this less).

I suppose it's less likely that people do something like this in Flow Mode: -

|dbfit.OracleTest|
|Set Option|fixed length string parsing|true|

(I think that should work although it looks strange).

[MMatten]

In DatabaseTest the method's we're replacing with Fixtures are technically 'in flow' whereas Fixtures are alien fixtures ('traverses' in FitLibrary terms as far as I can tell).

Are there any potential impacts we've not considered yet? What about any DbFit fixture subtyping people might have done? I guess unlikely but some concerns were raised when opinion on deprecating the 2.0 .net code was canvassed.

[MMatten]

If this is no longer supported then I'll have lots of ETL tests to update! Can we support both?

[javornikolov]

I believe it's still supported. I removed it to simplify the reference a bit. But maybe we can leave a note that the "old" syntax is still supported?

[MMatten]

Sounds good. Perhaps 'alternate syntax'.

[javornikolov]

Right now things like the following are still working actually:

|Database Environment|
|Commit|

In DatabaseTest the method's we're replacing with Fixtures are technically 'in flow' whereas Fixtures are alien fixtures ('traverses' in FitLibrary terms as far as I can tell).

DatabaseTest is what's used in flow mode. Standalone mode relies on DatabaseEnvironment fixture.

Are there any potential impacts we've not considered yet? What about any DbFit fixture subtyping people might have done? I guess unlikely but some concerns were raised when opinion on deprecating the 2.0 .net code was canvassed.

Well, that's a good point. Maybe we need to increase version number to 3.x.x?

[MMatten]

Right now things like the following are still working actually

Good to know!

DatabaseTest is what's used in flow mode. Standalone mode relies on DatabaseEnvironment fixture.

Yes but methods that return Fit Fixtures are treated as 'alien' objects to DoFixture/SequenceFixture so perhaps some, as yet unknown, unintended impact on any subtypes people might have created although unlikely I guess.

Well, that's a good point. Maybe we need to increase version number to 3.x.x?

Or up to 4.0 even?

[javornikolov]

Or up to 4.0 even?

Ah, that's what I meant :-)

Yes but methods that return Fit Fixtures are treated as 'alien' objects to DoFixture/SequenceFixture so perhaps some, as yet unknown, unintended impact on any subtypes people might have created although unlikely I guess.

We already have many such cases in DatabaseTest (the majority of methods there are returning Fixture). I just used them as an example when implementing this change. I haven't yet investigated deeply what does that mean and might be the consequences.

[MMatten]

Ok. Nothing else from me to discuss on this one. Looks good.

[MMatten]

One more query:

Right now things like the following are still working actually:

|Database Environment|
|Commit|

But what about:

|Database Environment|
|Commit|
|Close|

Won't the new Commit fixture take over processing of the table and then ignore the 'Close'? That will break quite a few of my ETL tests. If so can we still support this (may be by pushing more into the Database Environment)?

[javornikolov]

We should test that, if it's important to have it working - perhaps we should have it in our test suites.

[MMatten]

More info on the breakages: not closing connections (in Teradata) results in sessions staying around in the database (until some kind of periodic garbage collection mops them up) but the accumulation results in max sessions for the DB being breached and tests suites start to fail consistently.

I'd have to update a large number of tests to move the 'Close' into a second 'Database Environment' table (often multiple times per test page).

[javornikolov]

@MMatten, I'm just trying the following for Derby in the auto-commit-support branch and it seems to close the connection (the next statement is failing with java.lang.IllegalArgumentException: No open connection to a database is available which mean that the connection has been closed):

!|DatabaseEnvironment|
|Commit|
|Close|

Do you mean you tested that for Teradata and it fails?

[MMatten]

@javornikolov, that sounds good then.

I haven't run any tests yet with this branch.

My previous note was just to explain why I need this working (rather than leave it to the DBMS to close them down after the client had detached).

My thought was that the new commit Fixture would take over the processing of the Database Environment from the row containing the Commit (as the SequenceFixture method commit returns a Fit Fixture subclass - this is what the FitLibrary DoFixture / SequenceFixture docs suggest) and then the Close would get ignored.

[javornikolov]

OK, good - to me it looks like that we're not breaking the existing behaviour. I'm going to merge this PR.

[MMatten]

Ok. I think the noted behaviour only relates to Fixtures that are in flow. So it's not relevant for our standalone mode. We'd need to be wary in flow mode DatabaseTest though perhaps.