-
-
Notifications
You must be signed in to change notification settings - Fork 899
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 it possible to pass additional options to create_engine #166
Comments
Lately, my preferred way of getting around this is to subclass SQLAlchemy and override the |
Thanks @davidkhess, it seems a few folks are doing that. I'm looking into how this can be better supported. |
@immunda any update on this? we need to pass in a custom json json_serializer and need a way to specify/override engine_options, https://github.com/mitsuhiko/flask-sqlalchemy/pull/67/files looks good to me, any reason not to use that patch? |
This isn't going to be in the release 2.1 (very soon). However, it should definitely be addressed in v3. @jayzeng It's not a problem with that commit per se, but there's a lot of details to sort out around documentation changes, tests and then managing the deprecation of options which such a solution would essentially override/render redundant. |
Completely understood. It is a pressing issue for us and I think we will just go for the subclass route. @immunda any rough estimate when v3 will be released? |
Also let me know what I can help to get it released asap |
I'm also interested in some way to do this. I need to pass a custom connection function, and right now have to do some monkey patching around to get there. Is there any value in working on or submitting a proposed fix at this point? |
This really needs to be fixed. It could be easily done by accepting a This could literally be a two line change: options = {'convert_unicode': True}
self._sa.apply_pool_defaults(self._app, options)
self._sa.apply_driver_hacks(self._app, info, options)
if echo:
options['echo'] = echo
# next two lines are new
config_engine_opts = self._app.config.get('SQLALCHEMY_ENGINE_OPTS', {})
options.update(config_engine_opts)
self._engine = rv = sqlalchemy.create_engine(info, **options) Or, go slightly more customizable, and put those two lines in a new method @davidism or @immunda would you accept a PR with this approach? |
A response would be great re: whether or not a PR with @rsyring's approach would be accepted. My particular use-case has to do with disconnect handling. |
There's already a PR open: #487. We will get to it when we have time available. No need to ping us, we're still aware of open issues. |
FWIW, SQLAlchemy 1.2 is coming out with a new method of detecting connections in the pool that are no longer valid: https://docs.sqlalchemy.org/en/latest/core/pooling.html#pool-disconnects-pessimistic We really want to use that and without this issue getting resolved, it is going to be much harder to do so. This can be a two line change, as demonstrated above. I'll write the PR, I just want someone to tell me before I make the effort that my approach will work. |
I second this. I need it for using sqlite in memory databases with unittesting, i.e.: http://www.sameratiani.com/2013/09/17/flask-unittests-with-in-memory-sqlite.html |
If anyone working on this project is a consultant, I'd be happy to hire you to get this issue resolved. Please contact me directly regarding. Thanks. |
I would like to see @rsyring's solution implemented. In the meantime, we've opted to move forward with the following subclassing approach: #589 (comment) The subclassing approach is the same solution that @gilbsgilbs has referred to when closing the following related Issue: #577 From what I can see, not being able to pass additional options is an issue that keeps coming up (e.g. #540, #67, #103, #408, and level12/keg#73 to list a few) so we shouldn't rely on having to subclass to work around it. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Per (#540 (comment)), I'll be opening a PR shortly (unless someone beats me to it) that:
After some additional though, I'm also think it would make sense to Adjust |
I needed to pass in an
isolation_level
parameters to thecreate_engine
call. I did it like this:gromgull@689c90c
(on top of the 1.0 tag since that's what we used, but I can make a pull-request with it applied to HEAD if you want)
The text was updated successfully, but these errors were encountered: