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

feat: SQLAlchemy 1.4 support #1783

Closed
wants to merge 3 commits into from
Closed

Conversation

uranusjr
Copy link

Description

An alternative way to fix #1710. Flask-SQLAlchemy 2 broke the clause=None parameter, but since mapper=None, clause=None is simply the default since SQLAlchemy 0.7.2 (released more than ten years ago), we can simply fix the problem by not passing those parameters at all.

ADDITIONAL INFORMATION

@codecov
Copy link

codecov bot commented Jan 13, 2022

Codecov Report

Merging #1783 (13e7fd5) into master (e2b744c) will not change coverage.
The diff coverage is 50.00%.

❗ Current head 13e7fd5 differs from pull request most recent head b3b53ca. Consider uploading reports for the commit b3b53ca to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1783   +/-   ##
=======================================
  Coverage   77.05%   77.05%           
=======================================
  Files          56       56           
  Lines        8175     8175           
=======================================
  Hits         6299     6299           
  Misses       1876     1876           
Flag Coverage Δ
python 77.05% <50.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
flask_appbuilder/cli.py 57.00% <0.00%> (ø)
flask_appbuilder/security/sqla/manager.py 71.38% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e2b744c...b3b53ca. Read the comment docs.

Copy link
Owner

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, this would be great, but while testing it I got:

DB Creation and initialization failed: get_bind() got an unexpected keyword argument 'bind'

I think that get_bind from flask-sqlalchemy is still incompatible with SQLAlchemy 4.X.

Bump the Flask-SQLAlchemy on requirements.txt also.

@uranusjr
Copy link
Author

I regenerated the entire requirements.txt with pip-compile instead. I think this should work?

These arguments create an incompatibility with Flask-SQLAlchemy 2.
However, since 'mapper=None, clause=None' is simply the default since
SQLAlchemy 0.7.2 (release in 2011), we can simply remove them to be
compatible with all reasonably recent SQLAlchemy and Flask-SQLAlchemy
versions.
@uranusjr
Copy link
Author

uranusjr commented Jan 14, 2022

Oh, since all test jobs share the same requirements.txt, the 3.6 test does not run anymore since some of the dependencies have dropped 3.6 (since it already reached EOL). What is a good approach to this? IMO the test jobs should not share requirements.txt because the file depends on Python version. They should instead directly install the package from setup.py. Alternatively, if a fully-pinned requirements file is desired, each Python version should independently generate one file, instead of all versions sharing one file.

@dpgaspar
Copy link
Owner

Oh, since all test jobs share the same requirements.txt, the 3.6 test does not run anymore since some of the dependencies have dropped 3.6 (since it already reached EOL). What is a good approach to this? IMO the test jobs should not share requirements.txt because the file depends on Python version. They should instead directly install the package from setup.py. Alternatively, if a fully-pinned requirements file is desired, each Python version should independently generate one file, instead of all versions sharing one file.

Right but besides that, try your change locally with just SQLAlchemy>=1.4.
I've started this: #1786, and have surpassed the get_bind issue on flask-sqlalchemy. But bumped on another problem this time with FAB vs SQLAlchemy

@dpgaspar
Copy link
Owner

dpgaspar commented Jan 14, 2022

@uranusjr thank you to giving it a shot on fixing this issue. Going to close it in favour of #1786

@dpgaspar dpgaspar closed this Jan 14, 2022
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.

Relax SQLAlchemy version dependency
2 participants