Django 2.0 compatibility #1582

mpauly · 2018-01-02T10:43:17Z

Hi,
this PR adds Django 2.0 compatibility.
As the PR #1557 did not see any progress recently I implemented everything needed for Django 2.0 compatibility here.

In particular I rewrote the rebuild_index command to strip certain arguments that are not used in the clear_index and update_index commands respectively. This is implemented in a relatively adhoc way, however looking at the amount of changes made to rebuild_index in the past I figured this might be the easiest way to implement this change.

Additionally I added Django 2.0 to the tests and excluded the combination Django2.0 + Python2.7 from the test suite so that the tests still pass. Maybe one should also consider adding python 3.6 to the test suite, however I did not do that yet.

Also note the change I made to test_haystack/solr_tests/test_solr_backend.py. This was necessary due to the recent update in pysolr. That update broke the django-haystack tests, as pysolr.__version__ is now a string instead of a tuple. The fix I implemented should be backwards compatible.

Please let me know if there is anything this MR needs to get merged, cheers,
Martin

This reverts commit 7a9ac38.

… command

acdha · 2018-01-02T14:22:47Z

test_haystack/solr_tests/test_solr_backend.py

@@ -1458,7 +1458,7 @@ def test_boost(self):
        ])


-@unittest.skipIf(pysolr.__version__ < (3, 1, 1), 'content extraction requires pysolr > 3.1.0')
+@unittest.skipIf(tuple(pysolr.__version__.split('.')) < (3, 1, 1), 'content extraction requires pysolr > 3.1.0')


This could also use pysolr.version_info but that will trigger a deprecation warning on setuptools >= 8.0 since that's no longer a simple tuple.

That actually seems to have an additional problem as the parsed version still contains strings instead of integers.
Either we could do a pysolr.version_info < parse_version('3.1.1') with the corresponding import or we just drop that annotation? Seems relatively unlikely that anyone is running the test suite with a version of pysolr from 2013, no?
What do you prefer?

Dropping it does make sense - if anyone is running tests on an install that old it seems unlikely that they'd have upgraded haystack but frozen pysolr

Okay, I dropped the annotation.

mpauly · 2018-01-03T18:01:56Z

By the way, thank you @acdha for replying so fast and maintaining django-haystack!

Anything else that needs updating in this MR?

acdha · 2018-01-03T18:16:16Z

I wanted to review it in more detail first but at first glance it looked good

mpauly · 2018-01-19T20:56:34Z

@acdha did you already have time to take a look at this MR? any way I can support you in getting this merged?

dani0805 · 2018-01-20T11:11:57Z

I can vouch for this, we have been using @mpauly fork for 2 days in our dev branch and it works like a charm.

acdha · 2018-01-21T01:31:37Z

@mpauly the main thing is testing – since I’m not using that combination at the moment I haven’t done more than look at the code. I’d like to e.g. confirm test coverage isn’t omitting anything important.

mpauly · 2018-01-21T18:49:08Z

@acdha I just added a test to see if ModelSearchIndex properly excludes related fields by default - seems this was not tested before. I also updated the corresponding line in haystack/indexes.py as my changes were not working as they should on Django 2.0. Now everything should be working fine and the tests pass on my travis, somehow on yours they don't - see https://travis-ci.org/mpauly/django-haystack.

Is there any reason why field.is_relation was not used before? That seems to be the most generic way to check if a field is a related_field.

mpauly · 2018-01-30T20:18:54Z

@acdha I just merged master into this branch (I probably should have rebased - sorry about that)
Additionally I can report that we are using this branch for a few days in our staging environment and at least in our setup it seems to work well.

mpauly · 2018-02-12T08:53:29Z

It seems that before I was not running the solr tests correctly, so I had to do more updates to fix some failing tests. Over the weekend I did the following changes:

I dropped support for Django < 1.11 - I also tried to cleanup the tox.ini, the travis file and setup.py
I added Python 3.6 to the tests in travis
I had to replace get_coords() by coords in multiple places to fix some tests
I updated some of the test configuration to work properly with Django 2.0

There might potentially be bugs in some functionality related to the geo lookups, however tests are passing with Django 1.11 and 2.0. We are also running this fork in production with Django 2.0 and have not experienced any problems yet.

timgraham

You could add:

'Framework :: Django :: 1.11',
'Framework :: Django :: 2.0',

to setup.py.

timgraham · 2018-02-12T13:21:55Z

.travis.yml

-        - DJANGO_VERSION=">=1.8,<1.9" VERSION_ES=">=2.0.0,<3.0.0"
-        - DJANGO_VERSION=">=1.9,<1.10" VERSION_ES=">=2.0.0,<3.0.0"
-        - DJANGO_VERSION=">=1.10,<1.11" VERSION_ES=">=2.0.0,<3.0.0"
+        - DJANGO_VERSION=">=1.11,<1.12" VERSION_ES=">=1.0.0,<2.0.0"


<1.12 -> <2.0

timgraham · 2018-02-12T13:21:57Z

.travis.yml


 matrix:
    allow_failures:
        - python: 'pypy'
+        - python: 2.7


These builds should be excluded rather than "allowed_failures".

timgraham · 2018-02-12T13:50:06Z

haystack/management/commands/rebuild_index.py

@@ -33,5 +34,11 @@ def add_arguments(self, parser):
        )

    def handle(self, **options):
-        call_command('clear_index', **options)
-        call_command('update_index', **options)
+        clear_options = copy.copy(options)


options.copy()?

timgraham · 2018-02-12T13:51:07Z

test_haystack/solr_tests/test_admin.py

@@ -2,11 +2,12 @@

 from __future__ import absolute_import, division, print_function, unicode_literals

+import django


timgraham · 2018-02-12T13:51:33Z

setup.py

@@ -12,7 +12,7 @@
    from setuptools import setup

 install_requires = [
-    'Django>=1.8,<1.12',
+    'Django>=1.11,<2.1',


I wouldn't specify an upper bound -- it might be that this will work fine with Django 2.1 without any changes.

timgraham · 2018-02-12T13:53:54Z

tox.ini

 deps =
-    Django>=1.8,<1.9
+    Django>=1.11,<1.12


timgraham · 2018-02-12T13:57:16Z

test_haystack/test_management_commands.py

        """
        Confirm that command-line option parsing produces the same results as using call_command() directly,
        mostly as a sanity check for the logic in rebuild_index which combines the option_lists for its
        component commands.
        """
        from haystack.management.commands.rebuild_index import Command

+        update_mock.return_value = ''


You can put return_value='' in the @patch calls.

timgraham · 2018-02-12T13:57:41Z

test_haystack/test_indexes.py

@@ -623,6 +629,7 @@ def test_excludes(self):
        self.assertTrue(isinstance(self.emsi.fields['pub_date'], indexes.DateTimeField))
        self.assertTrue('text' in self.emsi.fields)
        self.assertTrue(isinstance(self.emsi.fields['text'], indexes.CharField))
+        self.assertFalse('related_models' in self.m2mmsi.fields)


assertNotIn

timgraham · 2018-02-12T13:58:05Z

test_haystack/test_management_commands.py

@@ -69,6 +69,9 @@ def test_rebuild_index_using(self, m1, m2):
    @patch('haystack.management.commands.update_index.Command.handle')
    @patch('haystack.management.commands.clear_index.Command.handle')
    def test_rebuild_index(self, mock_handle_clear, mock_handle_update):
+        mock_handle_clear.return_value = ''


You can put return_value='' in the @patch calls.

timgraham · 2018-02-12T13:58:37Z

test_haystack/test_management_commands.py

+        args, kwargs = clear_mock.call_args
+
+        self.assertIn('interactive', kwargs)
+        self.assertEqual(False, kwargs['interactive'])


for boolean comparisons: self.assertIs(kwargs['interactive'], False)

mpauly · 2018-02-12T18:47:56Z

@timgraham Thanks for the comments, all of them should be implemented now.

webtweakers · 2018-02-21T18:32:22Z

I can confirm that @mpauly's fork works fine with Django 2.0. Tested rebuild_index command, searching, faceting and autocomplete in combination with ElasticSearch. No problems here.

michaelmior · 2018-02-22T20:19:59Z

In my case, django-haystack 2.7.0 works fine with Django 2 without any changes. Would be great to have this explucitly supported though.

nadimtuhin · 2018-02-27T19:17:12Z

waiting for d2 support :D

JonLevischi · 2018-03-02T18:08:00Z

Any idea when this branch will be officially committed to the master package?
Thanks

acdha · 2018-03-02T18:13:10Z

It's waiting for more people to test it. If you have time to volunteer in that regard it'll happen faster.

RabidCicada · 2018-03-02T18:16:06Z

@acdha WRT more people testing, what are we looking for? 5, 10? I don't have a production system. But I'm happy to test with my rinky dink setup, and vouch if it works for me. How many vouchsafes are we short? I don't think people, myself, included like fuzzy "more people" statements.

acdha · 2018-03-02T18:19:27Z

I was thinking more different combinations: i.e. I don't use ElasticSearch with Haystack much so at the very least I think it'd make sense to make sure someone has tested it with at least Solr, ES, and Whoosh.

RabidCicada · 2018-03-02T18:22:04Z

Ahh...very good clarification. Thanks. I wont be able to help much on that front :)

nadimtuhin · 2018-03-02T18:23:29Z

tested with django 2 & elasticsearch on aws, everything worked fine

JonLevischi · 2018-03-02T19:05:50Z

Apparently it will work with Whoosh on AWS as well.

mpauly · 2018-03-03T15:52:03Z

We are running Xapian-haystack, that works well with this branch

@mpauly

Thanks to @mpauly and @timgraham for working on this and @dani0805, @andrewbenedictwallace, @RabidCicada, @webtweakers, @nadimtuhin, and @JonLevischi for testing.

@mpauly

Thanks to @mpauly and @timgraham for working on this and @dani0805, @andrewbenedictwallace, @RabidCicada, @webtweakers, @nadimtuhin, and @JonLevischi for testing.

acdha · 2018-03-09T15:56:28Z

This is now in https://pypi.python.org/pypi/django-haystack/2.8.0

Martin Pauly added 16 commits December 31, 2017 18:05

Exclude unused options for call of clear_index and update_index

313cd52

Fix import order

7c029d1

Add the corresponding option for update_index

c030dd6

Fix missing attribute rel

e3ec7ae

Update imports

37451ca

Update tests

c1da134

Update authors

a725049

Trigger travis build

7a9ac38

Revert "Trigger travis build"

33c855b

This reverts commit 7a9ac38.

Update test - the interactive kwarg is only passed to the clear_index…

80fa925

… command

Reverse order

9e782ae

Mocking order

616535d

Fix tox

1df110c

Fix a bug due to string __version__ of pysolr

6e5cf5c

Deal with tuples and strings

8ac784f

Django 2.0 is not compatible with python 2.7

6b45e96

acdha reviewed Jan 2, 2018

View reviewed changes

Assuming that everyone who wants to run these tests upgrades pysolr

6ae072b

dani0805 mentioned this pull request Jan 18, 2018

Official release compatible with current Django LTS (1.11) #1574

Closed

Martin Pauly added 2 commits January 21, 2018 19:25

In Django 2.0 ForeinKeys must have on_delete

b4b4e7d

Added a test for exclusion of M2M fields for ModelSearchIndex

79d58b0

Martin Pauly added 2 commits January 30, 2018 20:31

Merge https://github.com/django-haystack/django-haystack

bb647bc

Fix intendation error in tox.ini

05e5a6c

Martin Pauly added 8 commits February 11, 2018 14:56

Update requirements in setup.py

2ce34db

Drop tests for Django < 1.11

de9de79

Ignore python2 Django2 combination

60d83b1

Replace get_coords() by coords in more places

eb1d3e2

Dropped a few unnecessary interactive=False

11b374d

Django 2.0 changes to tests

eb82ab1

For some reason the mock needs to return something

0cb4d84

drop support for old django versions

12c75a2

timgraham reviewed Feb 12, 2018

View reviewed changes

Implemented TG's review comments

40cbee5

acdha mentioned this pull request Mar 2, 2018

rebuild_index() not working with Django 2.* #1592

Closed

2 tasks

acdha mentioned this pull request Mar 9, 2018

Django 2.0 rebuild_index Error #1593

Closed

2 tasks

acdha merged commit 40cbee5 into django-haystack:master Mar 9, 2018

praseodym mentioned this pull request May 3, 2018

Search is broken in master WISVCH/dienst2#196

Closed

		@@ -2,11 +2,12 @@

		from __future__ import absolute_import, division, print_function, unicode_literals

		import django

Django 2.0 compatibility #1582

Django 2.0 compatibility #1582

Conversation

mpauly commented Jan 2, 2018

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

mpauly commented Jan 3, 2018

acdha commented Jan 3, 2018

mpauly commented Jan 19, 2018

dani0805 commented Jan 20, 2018

acdha commented Jan 21, 2018

mpauly commented Jan 21, 2018

mpauly commented Jan 30, 2018

mpauly commented Feb 12, 2018

timgraham left a 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

Choose a reason for hiding this comment

mpauly commented Feb 12, 2018

webtweakers commented Feb 21, 2018

michaelmior commented Feb 22, 2018

nadimtuhin commented Feb 27, 2018

JonLevischi commented Mar 2, 2018

acdha commented Mar 2, 2018 via email

RabidCicada commented Mar 2, 2018

acdha commented Mar 2, 2018 via email

RabidCicada commented Mar 2, 2018

nadimtuhin commented Mar 2, 2018

JonLevischi commented Mar 2, 2018

mpauly commented Mar 3, 2018

acdha commented Mar 9, 2018