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

Use a configurable list of views as an alternative to @db_write #11

Closed
wants to merge 8 commits into from
Closed

Conversation

aptiko
Copy link
Contributor

@aptiko aptiko commented Feb 13, 2013

Fixes #7. There is now a MULTIDB_PINNING_VIEWS setting in which views are listed. like this:

MULTIDB_PINNING_VIEWS = ('myapp.core.views.myview',
                         'anotherapp.contrib.greatapp.views.yourview')

The views listed pin the thread.

Btw, how am I supposed to run the tests for multidb? Because I'm doing it in an odd way (putting multidb in the INSTALLED_APPS of a Django application and running ./manage.py test multidb).

@@ -33,6 +37,12 @@ def process_request(self, request):
# In case the last request this thread served was pinned:
unpin_this_thread()

def process_view(self, request, view_func, view_args, view_kwargs):
"""Pin the thread if the current view is in MULTIDB_PINNING_VIEWS."""
view_name = view_func.__module__ + '.' + view_func.__name__
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this will fail on class-based views, which don't have a __name__ attribute.

@aptiko
Copy link
Contributor Author

aptiko commented Feb 14, 2013

Actually it's working as is with class-based views, but I added a test for it. flake8 was complaining about @override_settings, although it's not clear to me why. In any case, I changed the test logic to incorporate class-based views so I'm not using it any more.

I also removed the global variable multidb.middleware.PINNING_VIEWS, because it was preventing the tests from running normally (the global variable assumed that the value of MULTIDB_PINNING_VIEWS remains constant throughout a running instance, which is normally true but isn't so while running tests).

@aptiko
Copy link
Contributor Author

aptiko commented Mar 1, 2013

Ping

@jsocol
Copy link
Collaborator

jsocol commented Mar 1, 2013

@aptiko This is strange--I just rebased this onto master and ran the tests, and this is what I got:

FAILED (errors=7, failures=1)

I'm not immediately sure why, though. I tried on both Django 1.3.7 and 1.4.5. Any chance you can run them and see if you get the same issue? It's a lot of AttributeError: 'Settings' object has no attribute 'ROOT_URLCONF' .

@aptiko
Copy link
Contributor Author

aptiko commented Mar 4, 2013

Could you tell me how I am supposed to run the tests? Because I'm doing it in an odd way (putting multidb in the INSTALLED_APPS of a Django application and running ./manage.py test multidb).

If I try python setup.py test all I get is this:

/usr/lib/python2.6/distutils/dist.py:266: UserWarning: Unknown distribution option: 'test_requires'
  warnings.warn(msg)
running test

@jsocol
Copy link
Collaborator

jsocol commented Mar 4, 2013

Normally you would create a virtualenv, install the test requirements with "pip install -r requirements.txt" and then just run "fab test".

@aptiko
Copy link
Contributor Author

aptiko commented Mar 4, 2013

OK, I fixed these tests for Django 1.4. For Django 1.3 tests don't run properly, because I'm using some functionality that was probably not there in 1.3. So one question is if we need to support 1.3 now that 1.5 is out (according to Django policy, now that 1.5 is out, 1.3 is not being supported any more).

On 1.5, however, tests don't run at all. I can't find what the problem is. It says "cannot import MasterSlaveRouter". There is some kind of circularity. When it tries to import multidb.tests.settings, it tries to import multidb.tests first—I think this is normal Python. But then multidb.tests tries to import stuff from multidb, which in turn attempts to read stuff from settings. The error occurs at line 40 of multidb/__init.__.py:

if getattr(settings, 'SLAVE_DATABASES'):

Maybe moving settings.py out of directory tests would solve the problem. If my theory is correct, however, what I can't understand is why this manifests itself only with Django 1.5.

Note that this compatibility issue also seems to exist in 7b61629; therefore it does not seem to be specific to my changes; which means this pull request could be accepted (if we don't need 1.3 compatibility) and a new issue should be opened for 1.5.

Antonis Christofides added 4 commits April 23, 2013 19:04
The middleware for MULTIDB_PINNING_VIEWS was assuming that the view
function has a __name__. This is true in function and class-based views;
but there are also views that are objects, such as
django.contrib.syndication.views.Feed. The middleware was raising an
exception when attempting to process such views. This is now fixed.
For generic views the class name needs to be listed in
MULTIDB_PINNING_VIEWS, not the .as_view method.
The pinning cookie should be set not only if the request is POST, but
also if it is in MULTIDB_PINNING_VIEWS.
@aptiko
Copy link
Contributor Author

aptiko commented Jun 12, 2013

I'm closing this pull request because I've messed up my fork's branches, and will create a new one in its place.

@aptiko aptiko closed this Jun 12, 2013
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.

Use a configurable list of views as a @db_write alternative for GET requests that change the database
2 participants