Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use SQLAlchemy 2.x execution API #388

Merged
merged 6 commits into from
Jan 27, 2024
Merged

Use SQLAlchemy 2.x execution API #388

merged 6 commits into from
Jan 27, 2024

Conversation

npilon
Copy link
Contributor

@npilon npilon commented Jan 5, 2024

SQLAlchemy 2.0 removes the "connectionless" execution option and requires the use of a new context manager connection API. This pull request adjusts the pyramid debugtoolbar SQLAlchemy panel to use this new style.

  • I don't think this new style will work with sqlalchemy older than 1.4; how should that be handled?
  • connection.execute() expects a different bind-param style than is generated here; exec_driver_sql worked for me with psycopg2, but I'm not sure if it's correct for other drivers.

@mmerickel
Copy link
Member

mmerickel commented Jan 5, 2024

I don't have a lot of concerns with making a bw-incompat upgrade to the toolbar... It'd make me feel a lot better if we make sure ci/cd is running a test with both newer and 1.4 versions of sqlalchemy. I would like to preserve compat with 1.4 as it's explicitly there to assist folks upgrading to 2.0 and tons of people (myself included) are not upgraded to 2.0 yet.

Also no one has put work into upgrading the pyramid cookiecutter for sqlalchemy to 2.0.

@npilon
Copy link
Contributor Author

npilon commented Jan 5, 2024

That's a great idea; I'll look into getting CI testing 1.4 and 2.0.

@mmerickel
Copy link
Member

That's a great idea; I'll look into getting CI testing 1.4 and 2.0.

If you look at the existing github action and tox.ini I believe we are testing old versions of Pyramid, so probably something similar to that pattern. Thank you for working on this!

@npilon
Copy link
Contributor Author

npilon commented Jan 8, 2024

If you look at the existing github action and tox.ini I believe we are testing old versions of Pyramid, so probably something similar to that pattern. Thank you for working on this!

I think 8f871cd adds a run specifically for 1.4; I added an environment for 2.0 (not 2.1) but I think that's what we get by default on CI runs, so no point to adding it as its own item yet?

tox.ini Outdated Show resolved Hide resolved
@npilon npilon requested a review from mmerickel January 18, 2024 17:34
@mmerickel
Copy link
Member

I looked into this more and exec_driver_sql appears to be the correct answer here, as the after_cursor_execute returns dbapi-level attributes so everything lines up. Good work.

I pushed some extra improvements to use context managers in a few spots, and the sqla demo was broken so fixed that.

@mmerickel mmerickel merged commit 991f2d4 into Pylons:main Jan 27, 2024
32 checks passed
mmerickel added a commit that referenced this pull request Jan 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants