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

Fixes SlugField with no_dereference #2048

Merged
merged 1 commit into from Feb 26, 2019
Merged

Fixes SlugField with no_dereference #2048

merged 1 commit into from Feb 26, 2019

Conversation

noirbizarre
Copy link
Contributor

This one was tricky !!!

This PR fixes the udata search index command and more specificaly the .no_dereference() usage with SlugField.

Root cause

In MongoEngine 0.16, no_reference() behavior change to use copy.deepcopy on fields (sadly Mongoengine make extensive use of copy.deepcopy which is bad for performances).
On the other side, the SlugField need to keep a reference to its owning instance to be able to properly slugify on save.
As a consequence, copy.deepcopy was trying to copy the SlugField.instance field which is a model and cannot be deepcopied easily

Fix

copy.deepcopy allows to override the deepcopy behavior by implementing a __deepcopy__(self, memo) method. This is done in this PR by making a proper new SlugField instance

Future thought

The extensive use of copy.deepcopy might have a bad impact on performance. In order to improve that, we might need to implement some custom __deepcopy__(self, memo) methods on other objects because this is not done by MongoEngine itself.
As flask-restplus also make use of copy.deepcopy, implementing custom __deepcopy__(self, memo) methods on models might improve performance both on DB and API sides.

@noirbizarre noirbizarre added this to the 1.6.5 milestone Feb 25, 2019
@noirbizarre noirbizarre requested a review from a team February 25, 2019 17:23
@noirbizarre noirbizarre merged commit fd88b70 into opendatateam:master Feb 26, 2019
@noirbizarre noirbizarre deleted the no-dereference branch February 26, 2019 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant