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

Context manager for selecting content DB #3

Closed
wants to merge 0 commits into from

Conversation

jamalex
Copy link

@jamalex jamalex commented Jun 28, 2016

Summary

Adds a context manager/decorator to select content databases across a given set of database operations.

TODO

  • Have tests been written for the new code? Some, but more coverage still needed
  • Has documentation been written/updated? Inline docs
  • New dependencies (if any) added to requirements file

@jamalex jamalex changed the title Js api Context manager for selecting content DB Jun 28, 2016
@@ -0,0 +1,133 @@
"""
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the file where most of the work is done.

@rtibbles
Copy link
Owner

@jamalex There were some weird conflicts here, as I think I had rebased my branch. Cherry picked it in, but unfortunately stole the blame for one of the commits in the merge conflict resolution :/

Is in my branch now.

@rtibbles rtibbles closed this Jun 28, 2016
@jamalex
Copy link
Author

jamalex commented Jun 28, 2016

Oh, ok -- I did some rebasing against your js_api branch, but I guess you rebased again after that. ⚾ As long as it all makes it in!

Note on this stuff: it works for most cases. One problem (which I largely mitigated, but might still surface) is that Django's DB routing gets called at the time a queryset is resolved, not the time it's created, so if a queryset is created inside the context manager and then returned to elsewhere before lazy resolution occurs, it's possible it will throw an error. I think the cases in which this would happen should be rare now, with some changes I made and through upgrading to the bleeding edge version of django-mptt that includes this fix from a week ago: django-mptt/django-mptt#478

@rtibbles
Copy link
Owner

Yeah - I have further mitigated this issue by removing any mention of specific databases within the content API methods - so now they have to be called from within a context manager.

The one case where this has still cropped up is in the Django REST framework Serializers, where a queryset is returned from methods and then further processing done. Eli has worked around this by continuing to use the .using method in this context.

rtibbles pushed a commit that referenced this pull request Jul 15, 2016
refactor button styling a bit
rtibbles pushed a commit that referenced this pull request Aug 3, 2016
updates based on outstanding PR
rtibbles pushed a commit that referenced this pull request Aug 11, 2016
rtibbles pushed a commit that referenced this pull request Feb 8, 2017
updated docs around dependencies and virtualenv
rtibbles pushed a commit that referenced this pull request Jun 24, 2017
Fixes & Buildkite for Android
rtibbles pushed a commit that referenced this pull request Aug 22, 2018
rtibbles pushed a commit that referenced this pull request Sep 10, 2018
Add public channel endpoints and tests
rtibbles pushed a commit that referenced this pull request Dec 18, 2018
rtibbles pushed a commit that referenced this pull request Jan 18, 2024
rtibbles pushed a commit that referenced this pull request Oct 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants