Django 1.9, 1.10, and 1.11 upgrades #125

vkurup · 2017-10-07T01:48:18Z

Starting with #119, aim to get this working on Django 1.11. It currently only works in 1.8

coveralls · 2017-10-07T02:36:39Z

Coverage decreased (-3.7%) to 89.961% when pulling 77d21e8 on vkurup:django-upgrades into b94ec75 on django-cache-machine:master.

coveralls · 2017-10-07T03:11:47Z

Coverage increased (+0.04%) to 93.668% when pulling 36589e9 on vkurup:django-upgrades into b94ec75 on django-cache-machine:master.

coveralls · 2017-10-07T03:29:23Z

Coverage increased (+0.1%) to 93.75% when pulling f6839e4 on vkurup:django-upgrades into b94ec75 on django-cache-machine:master.

coveralls · 2017-10-07T03:37:41Z

Coverage increased (+0.1%) to 93.75% when pulling 3b562d4 on vkurup:django-upgrades into b94ec75 on django-cache-machine:master.

coveralls · 2017-10-07T03:39:34Z

Coverage increased (+0.1%) to 93.75% when pulling b7295ff on vkurup:django-upgrades into b94ec75 on django-cache-machine:master.

coveralls · 2017-10-07T14:08:48Z

Coverage increased (+0.1%) to 93.75% when pulling 7bd4f77 on vkurup:django-upgrades into b94ec75 on django-cache-machine:master.

coveralls · 2017-10-07T14:08:48Z

Coverage increased (+0.1%) to 93.75% when pulling 7bd4f77 on vkurup:django-upgrades into b94ec75 on django-cache-machine:master.

coveralls · 2017-10-07T14:08:48Z

Coverage increased (+0.1%) to 93.75% when pulling 7bd4f77 on vkurup:django-upgrades into b94ec75 on django-cache-machine:master.

coveralls · 2017-10-08T03:34:01Z

Coverage increased (+0.1%) to 93.75% when pulling 24c9779 on vkurup:django-upgrades into b94ec75 on django-cache-machine:master.

coveralls · 2017-10-09T12:37:18Z

Coverage increased (+0.1%) to 93.75% when pulling f98e32f on vkurup:django-upgrades into b94ec75 on django-cache-machine:master.

tobiasmcnulty · 2017-10-10T17:06:01Z

caching/base.py

+        https://github.com/django/django/commit/f3b7c059367a4e82bbfc7e4f0d42b10975e79f0c#diff-5b0dda5eb9a242c15879dc9cd2121379
+        """
+        if self._result_cache is None:
+            self._result_cache = list(self.iterator())


I believe this change will cause a server-side cursor to be used for _fetch_all (if not globally disabled via DISABLE_SERVER_SIDE_CURSORS) on Django 1.11.1+ due two these lines:

https://github.com/vkurup/django-cache-machine/blob/6a547d8e4295985b2a84d4e115b1192cff540dbc/caching/base.py#L181

https://github.com/django/django/blob/stable/1.11.x/django/db/models/query.py#L322-L323

I think we need to make sure _fetch_all() still uses self._iterable_class() directly rather than self.iterator() on Django 1.11+, perhaps via something along the lines of the following:

Separate our iterator() logic into a separate method, e.g., _iterate_on(iterable)

In _fetch_all(), call list(self._iterate_on(iter(self._iterable_class(self)))) for Django 1.11+ or list(self._iterate_on(self.iterator())) for older Djangos (_iterable_class unfortunately does not exist in Django 1.8).

In iterator(), simply return self._iterate_on(super(CachingQuerySet, self).iterator())

Does that seem crazy? What do you think?

Or better yet, rather than check the version directly, use duck typing as you already did here: 24c9779#diff-76150a51cd21d8d6b30aaaab22c7f114R179

_fetch_all() and iterator() are unchanged between Django 1.9-1.10, so it should be safe to use self._iterable_class directly in those versions (in addition to Django 1.11)

_fetch_all():

1.9: https://github.com/django/django/blob/stable/1.9.x/django/db/models/query.py#L1072-L1076

1.10: https://github.com/django/django/blob/stable/1.10.x/django/db/models/query.py#L1085-L1089

iterator():

1.9: https://github.com/django/django/blob/stable/1.9.x/django/db/models/query.py#L325-L330

1.10: https://github.com/django/django/blob/stable/1.10.x/django/db/models/query.py#L323-L328

vkurup · 2017-10-11T19:23:46Z

@tobiasmcnulty I made an update, trying to avoid overriding _fetch_all. This involved the following changes:

Creating CachingModelIterable as a subclass of django's ModelIterable, which allows us to use the superclass's __iter__ definition easily
Changed the constructor of CacheMachine to allow it to share most of its code with CachingModelIterable
Some 'conditional' hacks to make sure we load the correct iterator for Django 1.8 vs Django 1.9+

Let me know what you think!

coveralls · 2017-10-11T19:24:48Z

Coverage increased (+0.6%) to 94.194% when pulling 4f1fb8e on vkurup:django-upgrades into b94ec75 on django-cache-machine:master.

coveralls · 2017-10-11T19:24:48Z

Coverage increased (+0.6%) to 94.194% when pulling 4f1fb8e on vkurup:django-upgrades into b94ec75 on django-cache-machine:master.

coveralls · 2017-10-11T19:24:48Z

Coverage increased (+0.6%) to 94.194% when pulling 4f1fb8e on vkurup:django-upgrades into b94ec75 on django-cache-machine:master.

coveralls · 2017-10-11T19:24:48Z

Coverage increased (+0.6%) to 94.194% when pulling 4f1fb8e on vkurup:django-upgrades into b94ec75 on django-cache-machine:master.

tobiasmcnulty · 2017-10-12T21:21:45Z

caching/base.py

-        except StopIteration:
-            if to_cache or config.CACHE_EMPTY_QUERYSETS:
-                self.cache_objects(to_cache, query_key)
-            raise


This change loses the StopIteration re-raised here -- is that intentional?

Ahh... I think I missed that (I thought I was just simplifying an iteration through a iterator). I'll have to see if I can find a way to test that, or maybe just revert back to the old version of the code.

Ah, perhaps it doesn't matter? https://stackoverflow.com/questions/4019971/how-to-implement-iter-self-for-a-container-object-python#comment67775147_4019987

Actually, this might be a part of Python that I just don't understand well. I'd think that I could just fix this by adding a raise StopIteration on line 128 (at the same indentation level as the if on line 126). But that seems superfluous, since it's the last line of the method and the iteration will end anyways. Am I missing something?

https://www.python.org/dev/peps/pep-0479/#rationale

tobiasmcnulty · 2017-10-12T21:30:51Z

caching/base.py

    def __iter__(self):
+        if hasattr(super(CacheInternalCommonMixin, self), '__iter__'):


Perhaps try:/except AttributeError: to avoid hasattr() usage? The Django 1.8 bit can go away eventually, and there will be no performance hit for Django 1.9+

tobiasmcnulty · 2017-10-12T21:45:41Z

caching/base.py

-        cache.add(query_key, objects, timeout=self.timeout)
-        invalidator.cache_objects(self.model, objects, query_key, query_flush)
+
+class CacheMachine(CacheInternalCommonMixin):


Not entirely sure this will work, but I wonder if CacheInternalCommonMixin, CacheMachine, and CachingModelIterable could all be collapsed into a single class (e.g., by setting ModelIterable = object when it fails to import). Definitely ignore this comment if that would complicate rather than simplify things!

I tried doing that, but gave up early because of issues that I can't remember. But some of those problems might have been 2 small issues in the tests which I since identified, so let me try again.

tobiasmcnulty · 2017-10-12T21:54:58Z

caching/base.py

+                yield obj
+            return
+
+        # Try to fetch from the cache.
        try:
            query_key = self.query_key()
        except query.EmptyResultSet:
            raise StopIteration


Given our findings about StopIteration elsewhere, I think this should be converted to a simple return

tobiasmcnulty · 2017-10-12T21:55:49Z

caching/base.py

@@ -309,11 +332,13 @@ def __iter__(self):
            while True:
                yield next(iterator)
        else:
-            sql = self.raw_query % tuple(self.params)
-            for obj in CacheMachine(self.model, sql, iterator, timeout=self.timeout):
+            for obj in CacheMachine(self, iterator, timeout=self.timeout):
                yield obj
            raise StopIteration


I think this line can be deleted (so we get the implicit return None instead)

try to simplify Django 1.8/Django 1.11 compatibility

coveralls · 2017-10-13T15:25:04Z

Coverage increased (+0.5%) to 94.092% when pulling c220d61 on vkurup:django-upgrades into b94ec75 on django-cache-machine:master.

coveralls · 2017-10-13T15:49:22Z

Coverage increased (+0.5%) to 94.092% when pulling 096f2b2 on vkurup:django-upgrades into b94ec75 on django-cache-machine:master.

tobiasmcnulty · 2017-10-13T15:55:48Z

⛵️

Thanks again for taking this on!

coveralls · 2017-10-13T16:48:39Z

Coverage increased (+0.5%) to 94.092% when pulling 251e5e6 on vkurup:django-upgrades into b94ec75 on django-cache-machine:master.

coveralls · 2017-10-13T16:48:40Z

Coverage increased (+0.5%) to 94.092% when pulling 251e5e6 on vkurup:django-upgrades into b94ec75 on django-cache-machine:master.

tobiasmcnulty and others added 12 commits June 3, 2016 15:32

add newer django+python to tox+travis

bfca2e2

convert assigned lambda funcs to def statements

004261f

Merge branch 'master' into tox-updates

3cf88d7

fix flake8 errors

23b28c6

make travis use tox

f12a36c

specify only one python since tox does that now

d6c527e

pass TRAVIS environment variable into tox environment

aed2def

don't run flake8 as part of travis, use tox

615585b

Update tox and travis

dcc81ab

Include flake8 on travis

2668094

Workaround Travis bug

d450cff

Try to get coverage working

77d21e8

vkurup added 2 commits October 6, 2017 23:04

Remove support for Python < 2.7, Django < 1.8

79281be

Remove obsolete package

36589e9

Django 1.9 support

f6839e4

vkurup added 2 commits October 6, 2017 23:33

Django 1.10 support

3b562d4

Tell Travis to test 1.10

b7295ff

vkurup added 2 commits October 7, 2017 10:03

Django 1.11 support

6a547d8

Cleanup and docs updates

7bd4f77

vkurup changed the title ~~WIP: Django upgrades~~ Django 1.9, 1.10, and 1.11 upgrades Oct 7, 2017

Add tests for failure when running .values() or .values_list()

24c9779

Typo

f98e32f

tobiasmcnulty reviewed Oct 10, 2017

View reviewed changes

Attempt to tease out Django 1.8 differences

4f1fb8e

tobiasmcnulty reviewed Oct 12, 2017

View reviewed changes

tobiasmcnulty and others added 3 commits October 12, 2017 18:57

enable --keepdb for faster test runs

085bf35

try to simplify Django 1.8/Django 1.11 compatibility

52d4ea6

Merge pull request #1 from tobiasmcnulty/django-upgrades

c220d61

try to simplify Django 1.8/Django 1.11 compatibility

Minor doc updates

096f2b2

Bump version for release

251e5e6

vkurup merged commit 7f05b92 into django-cache-machine:master Oct 13, 2017

vkurup deleted the django-upgrades branch October 13, 2017 16:49

vkurup mentioned this pull request Oct 13, 2017

Don't cache values() and values_list() #115

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Django 1.9, 1.10, and 1.11 upgrades #125

Django 1.9, 1.10, and 1.11 upgrades #125

vkurup commented Oct 7, 2017 •

edited

coveralls commented Oct 7, 2017

coveralls commented Oct 7, 2017

coveralls commented Oct 7, 2017 •

edited

coveralls commented Oct 7, 2017 •

edited

coveralls commented Oct 7, 2017 •

edited

coveralls commented Oct 7, 2017

coveralls commented Oct 7, 2017

coveralls commented Oct 7, 2017 •

edited

coveralls commented Oct 8, 2017 •

edited

coveralls commented Oct 9, 2017 •

edited

tobiasmcnulty Oct 10, 2017 •

edited

tobiasmcnulty Oct 10, 2017

vkurup commented Oct 11, 2017

coveralls commented Oct 11, 2017

coveralls commented Oct 11, 2017

coveralls commented Oct 11, 2017

coveralls commented Oct 11, 2017

tobiasmcnulty Oct 12, 2017

vkurup Oct 12, 2017

tobiasmcnulty Oct 12, 2017

vkurup Oct 12, 2017

tobiasmcnulty Oct 12, 2017

tobiasmcnulty Oct 12, 2017

vkurup Oct 12, 2017

tobiasmcnulty Oct 12, 2017

vkurup Oct 12, 2017

tobiasmcnulty Oct 12, 2017

tobiasmcnulty Oct 12, 2017

coveralls commented Oct 13, 2017 •

edited

coveralls commented Oct 13, 2017 •

edited

tobiasmcnulty commented Oct 13, 2017

coveralls commented Oct 13, 2017

coveralls commented Oct 13, 2017 •

edited

		def __iter__(self):
		if hasattr(super(CacheInternalCommonMixin, self), '__iter__'):

Django 1.9, 1.10, and 1.11 upgrades #125

Django 1.9, 1.10, and 1.11 upgrades #125

Conversation

vkurup commented Oct 7, 2017 • edited

coveralls commented Oct 7, 2017

coveralls commented Oct 7, 2017

coveralls commented Oct 7, 2017 • edited

coveralls commented Oct 7, 2017 • edited

coveralls commented Oct 7, 2017 • edited

coveralls commented Oct 7, 2017

coveralls commented Oct 7, 2017

coveralls commented Oct 7, 2017 • edited

coveralls commented Oct 8, 2017 • edited

coveralls commented Oct 9, 2017 • edited

tobiasmcnulty Oct 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vkurup commented Oct 11, 2017

coveralls commented Oct 11, 2017

coveralls commented Oct 11, 2017

coveralls commented Oct 11, 2017

coveralls commented Oct 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 13, 2017 • edited

coveralls commented Oct 13, 2017 • edited

tobiasmcnulty commented Oct 13, 2017

coveralls commented Oct 13, 2017

coveralls commented Oct 13, 2017 • edited

vkurup commented Oct 7, 2017 •

edited

coveralls commented Oct 7, 2017 •

edited

coveralls commented Oct 7, 2017 •

edited

coveralls commented Oct 7, 2017 •

edited

coveralls commented Oct 7, 2017 •

edited

coveralls commented Oct 8, 2017 •

edited

coveralls commented Oct 9, 2017 •

edited

tobiasmcnulty Oct 10, 2017 •

edited

coveralls commented Oct 13, 2017 •

edited

coveralls commented Oct 13, 2017 •

edited

coveralls commented Oct 13, 2017 •

edited