[ST-1518] Remove transaction handling from potion.

[ticosax]

It should be framework decision, not lib decision

[ticosax]

I forgot to add tests. They will come soon, but it has already been tested with kompost. So I assume the code won't change.

[barrachri]

@ticosax what is the problem we are trying to fix?

also why the nested transaction?

[ticosax]

That is necessary because we took the decision to let flask decides globally about to commit or rollback the transaction SQLALCHEMY_COMMIT_ON_TEARDOWN = True. We could have decided to let each view deals with the transactions.

That's a liberty that flask-sqlalchemy offers, for some case it works, but not for us. As we chose to not rely on the views to manage the transaction.

We are trying to take control over the transactions/savepoints. We want to have control over the boundaries and the lifetime of the transactions. When external dependencies like potion are explicitly committing/rollback-ing, they prevent our business logic to be supported.
This pull request is made in regard of https://github.com/infarm/kompost/pull/2028 .
Where we want in the tests to isolate each http request/response cycle within their own savepoints. We want the test framework to rollback or commit, not the view. Thanks to those changes we can actually reproduce within the tests what is closer to real life.

Today this snippet, doesn't work.

assert not Model.query.count()
response = client.post()
assert response.status_code == 400
assert not Model.query.count()  # AssertionError 1 is True

with a modified version of https://github.com/infarm/kompost/pull/2028 + this PR it will.

[barrachri]

there's a double try/except.

The one inside our code, then the one inside the with (https://github.com/sqlalchemy/sqlalchemy/blob/6a622c636ca5bc55d96b92652fd43b914a77645c/lib/sqlalchemy/engine/base.py#L1764).

Not to sure we need the one here.

[ticosax]

The one in potion is to transform the exception and detect the conflict. In any case potion is raising an exception if one is triggered. It means in terms of transaction management, the context manager of sqlalchemy is the solely decision maker. the potion's one doesn't interfere with that.

[ticosax]

I remove the signal handlers outside the savepoint.
And I added some tests.

[nemmis]

To summarize my understanding.

We want to avoid having Potion (that uses flask-sqlalchemy) committing the current transaction when models are created/updated/removed. The previous code is committing because of the default parameter commit, e.g create(self, properties, commit=True).

At this point we could simply set commit=False and we would flush the session which keeps the current transactional concept, except if the operation fails (in this case the transaction is rollbacked).

In addition, #2028 rollbacks the transaction in Potion's exception handler.

If I understand correctly, the only reason to add a savepoint (session.begin_nested()) is for our tests as we do not want to rollback the data preparation step.

We can have different failure scenarios (where the transaction has to be handled correctly):

1. the model cannot be created because a validation steps fails (e.g try to create a HarvestRecord)
2. database operations fail during a flush

The flow diagrams are:

• Production: Model Validation Error
  1. Open a transaction S0
  2. We try to create a model, e.g HarvestRecord, but a ModelValidationError is raised
  3. Potion _exception_handler() catches this exception and issues a Rollback, back to S0
• Production: Persistence error
  1. Open a transaction (S0)
  2. We create a model, a savepoint is created within the transaction (S1)
  3. session.flush() fails, the transaction is rolled back, we are back to S1 and an exception is raised
  4. Potion _exception_handler() catches the flush exception and issues a Rollback, back to S0

For the tests, I am unsure what exactly happens. Right now I do not see why begin_nested is needed.

[nemmis]

I got into it. I would need an explanation on why begin_nested() is needed (my understanding is that it is for our tests but I do not see the entire flow of operation through)

[nemmis]

In addition do you know what the logic of SQLALCHEMY_COMMIT_ON_TEARDOWN is?

[ticosax]

50mn of brainstorming concluded this is good to go. Thanks @nemmis