save from Activerecord can commit external transactions

[federicoemartinez]

I have some code in a transaction:

with session.begin():
       an_object.save()
       another_object.save()

both objects have the ActiveRecordMixin.

That code used to work, and even after going to sqlalchemy 2, it worked.

But now with sqlalchemy-mixins 2.0.3, the save method commits the transaction, even when it wasn't the save method who opened it and that code is broken. I was thinking if it makes sense to check whether there is an active transaction an in that case don't commit. Something like this (of course more tidy):

    def save(self):
        """Saves the updated model to the current entity db.
        """
        # If there is an active transaction, we are not responsible for commit or rollback.
        if self.session.get_transaction():
            self.session.add(self)
            self.session.flush()
            return self

        try:
            self.session.add(self)
            self.session.commit()
            return self
        except:
            self.session.rollback()
            raise

I realized the same happens with delete

[michaelbukachi]

@federicoemartinez This is probably related to the removal of autocommit parameter in SQLAlchemy 2.0.

I think it would be better to introduce an in_transaction parameter so that commit method is explicitly controlled by the user.

def save(self, in_transaction=False):
    if in_transaction:
        self.session.add(self)
        return self
   # Rest of the code
    .....

.

[federicoemartinez]

it is indeed related to that. I thought of:

def save(self, commit=True):

Which is basically your idea, but I think it makes more explicit the behavior of the flag.

In my use case, I have been using my first approach it works, but I see why it might be a problem.

I can try to create a PR if you want.

[michaelbukachi]

it is indeed related to that. I thought of:

def save(self, commit=True):

Which is basically your idea, but I think it makes more explicit the behavior of the flag.

In my use case, I have been using my first approach it works, but I see why it might be a problem.

I can try to create a PR if you want.

Go ahead. I will review it as soon as it's ready.