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

Rewrite connection module #429

Merged
merged 6 commits into from Jul 8, 2022
Merged

Rewrite connection module #429

merged 6 commits into from Jul 8, 2022

Conversation

insspb
Copy link
Collaborator

@insspb insspb commented Dec 27, 2020

I had a time to look at connection module, that originally was made in 2016-2017. In nearest past there was a lot of question and bugs related to connection.
By my opinion our old connection module did a lot of work, that should not be done in this package at all.
For example there is some things that by my opinion is not correct:

  • Setting of some connection defaults - we should not do it, as same defaults will be set in upstream mongoengine/pymongo packages. We do not do anything with these defaults, but do the work that hard to track. Also as it is hard to track changes in parent packages, that should be implemented here.
  • Replacing mongomock urls - in this package mongomock support was dropped in version 0.8.1, but mongomock settings handled by mongoengine upstream package. We should not try to do the work, that is done in package with much more contributtors.
  • Different behavior on dict or list settings - old implementation had two types in return of connection functions. In case one db used it was dict, in case of multiple dbs it was list. It is not correct and can be considered as unexpected behavior.

So at the end this Pull request will:

  • Removed unexpected set of defaults.
    User is now fully responsible to set correct defaults, or defaults from upstream mongoengine and pymongo will be used.
  • Connection property of flask-mongoengine MongoEngine class now will always return dict with one or many database connections.
  • Connection handling now the same for one connection or many connections. Before this PR it was difference when 1 or many DB used.
  • Updated tests for new behavior.

TODO before merge:

  • Documentation update

Should fix:

I honestly hope for any comments, and for some test users, as it is deep flask-mongoengine behavior change. Code reviews are welcomed on any stage.

@insspb insspb added type: enhancement Enhancement update for old feature type: breaking-change Marks an important and likely breaking old interface change. log:breaking-change Changelog mark label. Marks breaking changes. log:fixed Changelog mark label. Marks fixed bug issues. stage: WIP Work In Progress labels Dec 27, 2020
@insspb insspb self-assigned this Dec 27, 2020
@insspb insspb force-pushed the rewrite-connection-module branch from c99d73f to 3a24265 Compare July 1, 2022 22:04
@insspb insspb force-pushed the rewrite-connection-module branch from 16f859d to f0e70f6 Compare July 6, 2022 21:38
@insspb insspb force-pushed the rewrite-connection-module branch from f0e70f6 to f6d11c2 Compare July 8, 2022 03:21
@insspb insspb merged commit 6b18011 into master Jul 8, 2022
@insspb insspb deleted the rewrite-connection-module branch July 8, 2022 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
log:breaking-change Changelog mark label. Marks breaking changes. log:fixed Changelog mark label. Marks fixed bug issues. stage: WIP Work In Progress type: breaking-change Marks an important and likely breaking old interface change. type: enhancement Enhancement update for old feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant