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

make SA engine configuration more flexible #684

Merged
merged 7 commits into from Apr 17, 2019

Conversation

Projects
None yet
8 participants
@rsyring
Copy link
Collaborator

commented Mar 8, 2019

fixes #166 - make it possible to pass additional options to create_engine()
fixes #671, #681 - deprecation warnings from SA 1.3

@rsyring

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 8, 2019

Current Travis failures:

  • Python 3.3 fails due to tox supporting 3.4+
  • Python 2.6 fails due to SQLAlchemy needing Py 2.7+
  • docs_html: /home/travis/build/pallets/flask-sqlalchemy/.tox/docs_html/lib/python3.6/site-packages/flask_sqlalchemy/init.py:docstring of flask_sqlalchemy.SignallingSession.get_bind:63:Unknown
    interpreted text role "paramref".

@davidism I'm not sure where FSA sits in terms of Python version support. Created an issue to discuss that: #685

The docs error I can look into but I figured I'd see if you knew what the problem is. This PR doesn't change that file, so I think the problem existed previously.

@rsyring rsyring referenced this pull request Mar 8, 2019

Closed

Tox docs build failing #686

@rsyring

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 9, 2019

When #687 is approved and merged, I'll rebase this PR on that branch and that should resolved all the CI issues.

@rsyring

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 11, 2019

I need to review how/why get_engine() caches the engine before this is merged.

@rsyring

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 14, 2019

I need to review how/why get_engine() caches the engine before this is merged.

I'd like to punt on that discussion and have opened #698 for discussion.

@davidism I've rebased this PR, Travis is passing, and I believe the commits in this PR are distinct enough to not squash. Let me know if you would like me to change anything and/or if I'm good to merge.

Show resolved Hide resolved tests/test_config.py
Show resolved Hide resolved flask_sqlalchemy/utils.py Outdated
Show resolved Hide resolved flask_sqlalchemy/__init__.py Outdated

dkellner added a commit to pyeve/eve-sqlalchemy that referenced this pull request Mar 23, 2019

Pin SQLAlchemy version due to warnings in Flask-SQLAlchemy
When pallets/flask-sqlalchemy#684 is merged, we should
be good to use the newest SQLAlchemy again.

@rsyring rsyring referenced this pull request Mar 26, 2019

Closed

Remove "convert_unicode" #707

@rsyring rsyring force-pushed the enhance-engine-config branch from 5cf891e to 56d408c Mar 26, 2019

@rsyring rsyring requested a review from davidism Mar 26, 2019

@rsyring

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 26, 2019

Code updated to address two review requests. The lingering one with respect to mock usage remains. If the status quo is acceptable, then this is ready for merge and I'll prep a 2.4 release for @davidism's review.

@rsyring

This comment has been minimized.

Copy link
Collaborator Author

commented Mar 26, 2019

The doc tests are failing in CI with:

/home/travis/build/pallets/flask-sqlalchemy/docs/changelog.rst.rst:23:broken link: https://github.com/pallets/flask-sqlalchemy/pull/556 (403 Client Error: Forbidden for url: https://github.com/pallets/flask-sqlalchemy/pull/556)

ERROR: InvocationError for command '/home/travis/build/pallets/flask-sqlalchemy/.tox/docs/bin/sphinx-build -W -b linkcheck -d /home/travis/build/pallets/flask-sqlalchemy/.tox/docs/tmp/doctrees docs docs/_build/linkcheck' (exited with code 2)

That tox run passes when I test it locally, maybe GitHub is having some issues. Anyway, I don't think that failure should hold up the merge of this PR. I'll kick off the job a couple more times to see if I can get it to pass.

Definitely think GitHub is having issues, another CI run gives a different PR number for the failure:

/home/travis/build/pallets/flask-sqlalchemy/docs/changelog.rst.rst:20:broken link: https://github.com/pallets/flask-sqlalchemy/pull/551 (403 Client Error: Forbidden for url: https://github.com/pallets/flask-sqlalchemy/pull/551)
@davidism

This comment has been minimized.

Copy link
Member

commented Apr 3, 2019

Are there any separate features we can deprecate and roll into this? If so, I'd like those deprecation warnings to be in the next release.

@aydumoulin

This comment has been minimized.

Copy link

commented Apr 11, 2019

@davidism The two deprecations that arose from fast forwarding sqlalchemy to 1.3+ in my postgres flavor usage were:

  • SADeprecationWarning: The create_engine.convert_unicode parameter and corresponding dialect-level parameters are deprecated, and will be removed in a future release. Modern DBAPIs support Python Unicode natively and this parameter is unnecessary
    default.DefaultDialect.__init__(self, **kwargs)
  • SADeprecationWarning: Use .persist_selectable
    info = getattr(mapper.mapped_table, 'info', {})
@davidism

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

That's not what I'm referring to, those are deprecations in SQLAlchemy. I'm referring to features that we want to deprecate.

@woodb

This comment has been minimized.

Copy link

commented Apr 17, 2019

With the recent publishing of security vulnerabilities in SQLAlchemy v1.2.17 through v1.3.0b2 (CVE-2019-7164) getting pushed out to folks via GitHub alerts, this issue is becoming a bigger deal for users that want to upgrade to v1.3.x.

With these security issues abound, I would like to suggest that we consider expediting getting this merged and released (i.e. without waiting for more stuff to put into the next release)

I haven't read the code here, so not sure if a patch version is appropriate, but really would like to see this fix get out so that my test suite can have useful output again, hah.

If there's anything that still needs to be done on this PR to get #671 and #681 closed, please let us know. Thanks!

@rsyring

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 17, 2019

From @davidism regarding his comment on deprecations above:

was specifically talking about engine settings in that comment
Did any individual config options get added before that could be rolled up? I think ECHO is the only one to keep since it's good for debugging. http://flask-sqlalchemy.pocoo.org/2.3/config/
There are mostly pool-related config keys.
And NATVIE_UNICODE was no longer needed, right?
So we might want to issue deprecation warnings if they're set, and add their value to ENGINE_OPTIONS for now.

I'll be doing that work shortly and then prepping a release.

@rsyring rsyring force-pushed the enhance-engine-config branch from 56d408c to 078a04b Apr 17, 2019

@fuhrysteve

This comment has been minimized.

Copy link

commented Apr 17, 2019

@woodb just to be clear to any other concerned observers of this repo, there are no known security vulnerabilities related to CVE-2019-7164 in any currently supported major versions of sqlalchemy as long as you are escaping user input in places in the docs that have clearly stated for many years they are intentionally not escaped.

@rsyring rsyring merged commit 112fe40 into 2.x-maintenance Apr 17, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rsyring rsyring deleted the enhance-engine-config branch Apr 17, 2019

@swuecho

This comment has been minimized.

Copy link

commented May 3, 2019

Thanks! especially for SQLALCHEMY_ENGINE_OPTIONS.

@billyfung

This comment has been minimized.

Copy link

commented May 22, 2019

ditto^
engine options works great for psycopg2 use_batch_mode=True

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.