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

Disable SQLALCHEMY_DATABASE_URI when SQLALCHEMY_BINDS is used #663

Closed
cykerway opened this issue Jan 10, 2019 · 5 comments · Fixed by #731
Closed

Disable SQLALCHEMY_DATABASE_URI when SQLALCHEMY_BINDS is used #663

cykerway opened this issue Jan 10, 2019 · 5 comments · Fixed by #731
Labels
Milestone

Comments

@cykerway
Copy link

cykerway commented Jan 10, 2019

I don't see there is a need for SQLALCHEMY_DATABASE_URI when SQLALCHEMY_BINDS is used. If users don't set SQLALCHEMY_DATABASE_URI then it gets a default value of an in-memory sqlite database. This may confuse users if they forget to set __bind_key__ on a model. In that case, that model doesn't fail but writes into the in-memory database.

I was trying to set SQLALCHEMY_DATABASE_URI to None, but flask-sqlalchemy doesn't seem to allow that with db.create_all(), even if all the models have __bind_key__ set to a valid bind. Can you fix the plugin so that SQLALCHEMY_DATABASE_URI defaults to None, or at least let users set it to None? Thank you.

Possibly related to: #471, #472

@rsyring rsyring added the config label Mar 9, 2019
@rsyring rsyring added this to the 2.4 milestone Mar 9, 2019
@rsyring
Copy link
Contributor

rsyring commented Mar 14, 2019

I do believe we should remove the default value for SQLALCHEMY_DATABASE_URI and instead throw an error when the value is None and binds are not being used. @davidism has a comment in #472 that indicates the current state of things is misleading.

Bumping this milestone to 3.0 since it would be a backwards incompatible change.

@rsyring rsyring modified the milestones: 2.4, 3.0 Mar 14, 2019
@lbeaufort
Copy link
Contributor

Hi, I'd like to take on this issue :)

@lbeaufort
Copy link
Contributor

Update - this change gets pretty messy if I branch of master before this PR is merged, so I was pausing on the work. I could put up a [WIP] PR if needed, possibly branching off the latest changes in #727 and rebasing/merging changes as needed.

@davidism
Copy link
Member

davidism commented May 8, 2019

Merged #727, go ahead with a PR!

@Snyssfx

This comment has been minimized.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging a pull request may close this issue.

5 participants