sqlalchemy-adapter
sqlalchemy-adapter copied to clipboard
pycasbin
Need ability to not use internally constructed session
#36 while looking innocent enough is a breaking change and should have rolled minor versions at least and not been v0.3.1. Deleting the _commit() functionality and reworking the internals so all inheritance exploded was definitely not something I was expecting for a patch roll.
I agree with the intent of the change, it is correct sessions are short lived so having this in theory is a correct change.
The issue comes from when session management is handled outside of the scope of casbin. I am a pretty big proponent of sessions should be explicitly managed and not hidden away in the library. I also must absolutely have the ability to force my casbin session and my other DB changes to be executed as part of a single operation.
The way I was previously able to do this was hijacking the _session variable as part of the adapter and the self._commit() function. I have an inherited adapter that just made sure _commit was a noop and I could adjust the _session variable to my upstream session and things would work out ok.
This change also explodes things like autocommit out of postgresql if that is how the engine is bound because session.commit() will always explode because no background implicit session is created to be committed like this. Fortunately I hadn't upgraded that project code base yet.
Based on this change, I feel like two adapters being exposed from this library might make the most sense. the first being the core adapter which is basically this class minus the definition of the constructor and the _session_scope(self) function. This basically says "go ahead and role your extensions, we will guarantee to keep this interface" and a second one that extends the core with the default implementation so people will more basic needs still have a working out of the box experience.
PR is here: https://github.com/pycasbin/sqlalchemy-adapter/pull/58
Closed as stale