fix: session get bind signature

[dpgaspar]

The SignallingSession get_bind is being called without any parameters, this is probably due to the new SQLAlchemy proxied mechanism for registering scoped sessions

(None, None, None, None, False) {}

This is a simple fix that just uses the exact method signature from SQLAlchemy for get_bind of the current session

• fixes Possible issue on session get_bind #953

Checklist:

• Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
• Add or update relevant docs, in the docs folder and in code.
• Add an entry in CHANGES.rst summarizing the change and linking to the issue.
• Add .. versionchanged:: entries in any relevant code docs.
• Run pre-commit hooks and fix any issues.
• Run pytest and tox, no tests failed.

[davidism]

Needs to be rebased to 2.x branch, since it's a fix against 2.x. You don't need to re-request my review, I get notifications on events already.

[ashb]

Adding *args is less fragile

[dpgaspar]

Needs to be rebased to 2.x branch, since it's a fix against 2.x. You don't need to re-request my review, I get notifications on events already.

I can do that, but if this affects master wouldn't it be desirable to merge this to master, then cherry pick into 2.x?

I can also open a new PR from 2.x to 2.x with this patch?

[ashb]

I'd love to know what is causing the default args to be put in to args in the first place.

From some playing it's something to do with the scoped_session/session factory.

This doesn't work without this PR or something like it:

session.get_bind()

But this does:

session().get_bind()

[dpgaspar]

I'd love to know what is causing the default args to be put in to args in the first place.

From some playing it's something to do with the scoped_session/session factory.

This doesn't work without this PR or something like it:

session.get_bind()

But this does:

session().get_bind()

For sure, on SQLAlchemy < 1.4 was pretty simple, get_bind would get called with:
return getattr(self.registry(), name)(*args, **kwargs)

But now, I confess I don't follow what is going on

[ashb]

[22] > /home/ash/.virtualenvs/flask-sqla/lib/python3.9/site-packages/sqlalchemy/util/langhelpers.py(658)instrument()
-> proxy_fn = _exec_code_in_env(code, env, fn.__name__)
(Pdb++) pp code
'def get_bind(self, mapper=None, clause=None, bind=None, _sa_skip_events=None, _sa_skip_for_implicit_returning=False):\n    return self._proxied.get_bind(mapper, clause, bind, _sa_skip_events, _sa_skip_for_implicit_returning)'
(Pdb++) print(code)
def get_bind(self, mapper=None, clause=None, bind=None, _sa_skip_events=None, _sa_skip_for_implicit_returning=False):
    return self._proxied.get_bind(mapper, clause, bind, _sa_skip_events, _sa_skip_for_implicit_returning)

https://github.com/sqlalchemy/sqlalchemy/blob/93ea31df86fc925a9e1df2a7dc80dd39f7ae4b9e/lib/sqlalchemy/util/langhelpers.py#L613

This seems to be the source of the extra arguments.

@zzzeek Is there a particular reason the proxy is including the defaults as explicit parameters, rather than letting python defaults do their thing? (i.e. using *args, **kwargs)?

[dpgaspar]

@davidism
I opened another PR from branch 2.x to branch 2.x: #956
(it if helps)

[ashb]

We're going to have to use the *args approach as SQLA 1.3 only has 2 args.

(That, or update the dep on SQLAlchemy to >=1.4)

Edit: Daniel gave me access to his fork, and I've made this change there

[zzzeek]

[22] > /home/ash/.virtualenvs/flask-sqla/lib/python3.9/site-packages/sqlalchemy/util/langhelpers.py(658)instrument()
-> proxy_fn = _exec_code_in_env(code, env, fn.__name__)
(Pdb++) pp code
'def get_bind(self, mapper=None, clause=None, bind=None, _sa_skip_events=None, _sa_skip_for_implicit_returning=False):\n    return self._proxied.get_bind(mapper, clause, bind, _sa_skip_events, _sa_skip_for_implicit_returning)'
(Pdb++) print(code)
def get_bind(self, mapper=None, clause=None, bind=None, _sa_skip_events=None, _sa_skip_for_implicit_returning=False):
    return self._proxied.get_bind(mapper, clause, bind, _sa_skip_events, _sa_skip_for_implicit_returning)

https://github.com/sqlalchemy/sqlalchemy/blob/93ea31df86fc925a9e1df2a7dc80dd39f7ae4b9e/lib/sqlalchemy/util/langhelpers.py#L613

This seems to be the source of the extra arguments.

@zzzeek Is there a particular reason the proxy is including the defaults as explicit parameters, rather than letting python defaults do their thing? (i.e. using *args, **kwargs)?

I would assume so they get included in the docstrings.

im not seeing the whole context here, it looks like you're subclassing Session which is fine. Is it going wrong with scoped_session()?

[ashb]

@zzzeek Yeah, subclassing Session.

Given the dynamic function is via a string-eval, that won't show up in docstrings I don't think? Isn't functools.update_wrapper or similar a better approach for getting docstrings?

Very rough reproduction steps

class ExpSQLAlchemy(SQLAlchemy):
    def create_session(self, options):
        return sessionmaker(class_=SignallingSession, db=self, **options)


app = Flask(__name__)
app.config["SQLALCHEMY_DATABASE_URI"] = "sqlite:///example.sqlite"
db = ExpSQLAlchemy(app)

session = db.session
# This is fine
print(db.session().get_bind())
# This errors with "wrong number of positional arguments)
print(session.get_bind())

Because of the subclassing as it was defined;

     def get_bind(self, mapper=None, **kwargs):

that used to work, but on SQLA 1.4 that now doesn't, as the proxy code passes them all as positional.

[zzzeek]

not sure if you noticed check out sqlalchemy/sqlalchemy#6285 but I just put out 1.4.8, so might not get to this for some days

waiting to hear about upstream fix

[dpgaspar]

@zzzeek Thank you

[dpgaspar]

Going to close this, since SQLAlchemy is fixed and flask-sqlalchemy master looks good now