Summary

Refactor Whoosh and Elasticsearch support into pluggable backends

Review Request #8254 — Created June 21, 2016 and submitted Aug. 8, 2016, 1:55 p.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-3.0.x

Bugs

Depends On

Commit

be9ff7a...

Reviewers

Groups

reviewboard

People

Description

A previous patch introduced support for Elasticsearch instead of Whoosh
as an alternative search engine backend. Now, both Whoosh and
Elasticsearch are provided as SearchBackends and we produce the search
configuration form dynamically based on what search backends are
available.

We now use a registry to manage these backends. Extensions can now
register additional search backends (such as for Solr or other
Haystack-supported backends) through this interface.

The search settings have been moved out of the general settings into its
own form as the logic just for the form generation was getting rather
complicated.

Testing Done


Ran unit tests.
Verified the settings form worked correctly.

Files

Issues

Description	From	Last Updated
Can you make this field longer?	chipx86	July 11, 2016, 7:34 p.m.
Can you make this field longer?	chipx86	July 11, 2016, 7:34 p.m.
dddd	QH qhr	July 4, 2016, 11:24 a.m.
noqa?	david	July 5, 2016, 1:36 p.m.
Returns?	david	July 5, 2016, 1:36 p.m.
Missing a period. At first I was going to say this should say "Form," but it's managing multiple forms. Can …	chipx86	July 11, 2016, 6:08 p.m.
I think maybe, "If enabled, a search field will be provided for ..."	chipx86	July 11, 2016, 6:09 p.m.
"review requests, diffs, and users."	chipx86	July 11, 2016, 6:09 p.m.
This is redundant. We can get rid of it.	chipx86	July 11, 2016, 6:09 p.m.
This is redundant. We can get rid of it.	chipx86	July 11, 2016, 6:09 p.m.
Missing a description.	chipx86	July 11, 2016, 6:09 p.m.
"... for the base form and every search engine settings form.""" Which I'm not sure we want, actually. Probably just …	chipx86	July 11, 2016, 6:11 p.m.
This is missing a "Returns".	chipx86	July 11, 2016, 6:12 p.m.
Let's expand on the error message to make it more helpful: The search engine "%s" could not be found. If …	chipx86	July 11, 2016, 6:12 p.m.
Missing a "Returns".	chipx86	July 11, 2016, 6:12 p.m.
Missing docs.	chipx86	July 11, 2016, 6:13 p.m.
Let's add the same comment here that we have on other forms: # Reload any important changes into the Django …	chipx86	July 11, 2016, 6:13 p.m.
We're going to need to migrate this setting.	chipx86	July 11, 2016, 6:28 p.m.
Same error as above.	chipx86	July 11, 2016, 6:14 p.m.
Missing docs.	chipx86	July 11, 2016, 6:17 p.m.
Let's pass in files as well, in case we have some backend in the future that requires some uploaded license …	chipx86	July 11, 2016, 6:17 p.m.
We should have a docstring at the top of this file.	chipx86	July 11, 2016, 6:18 p.m.
We should have a docstring for this file.	chipx86	July 11, 2016, 6:18 p.m.
Missing docs.	chipx86	July 11, 2016, 6:21 p.m.
Let's flesh this out so it's clear what the purpose and responsibilities are.	chipx86	July 11, 2016, 6:21 p.m.
Let's have a _name suffix, to distingush it from a class or something.	chipx86	July 11, 2016, 6:19 p.m.
Missing Args.	chipx86	July 11, 2016, 6:22 p.m.
This is kind of vague. I'm not really left with an idea of what goes here. It'd be nice to …	chipx86	July 11, 2016, 6:24 p.m.
Missing *args.	chipx86	July 11, 2016, 6:24 p.m.
Missing a file docstring.	chipx86	July 11, 2016, 6:25 p.m.
"Elasticsearch". (I sure wish the "s" was still capitalized in their name...)	chipx86	July 11, 2016, 6:25 p.m.
Maybe say "... that the elasticsearch module ..."	chipx86	July 11, 2016, 6:25 p.m.
Would be nice to point to the docs. I have this in the Conditions stuff: This works as a :py:ref:`registry …	chipx86	July 11, 2016, 6:26 p.m.
"... with the specified ID."	chipx86	July 11, 2016, 6:26 p.m.
They don't return None if not found. They raise an exception. If we want to return None, we should make …	chipx86	July 11, 2016, 6:27 p.m.
Missing a file docstring.	chipx86	July 11, 2016, 6:27 p.m.
Would be nice to narrow down this selector by ID, if we can. (Not sure if the form has one.)	chipx86	July 11, 2016, 7:39 p.m.
Blank line between these.	chipx86	July 11, 2016, 6:28 p.m.
{% endblock scripts-post %}	chipx86	July 11, 2016, 6:28 p.m.
search before signals.	chipx86	July 26, 2016, 9:28 p.m.
This lost its docstring.	chipx86	July 26, 2016, 9:28 p.m.
No need for the parens and explicit tuple here.	chipx86	July 26, 2016, 9:28 p.m.
Missing Returns:	chipx86	July 26, 2016, 9:29 p.m.
Missing a file docstring.	chipx86	July 26, 2016, 9:29 p.m.
Missing a docstring.	chipx86	July 26, 2016, 9:31 p.m.
No need for the type=	chipx86	July 26, 2016, 9:31 p.m.
'SearchSettingsForm' imported but unused	reviewbot	July 26, 2016, 9:34 p.m.
Col: 80 E501 line too long (113 > 79 characters)	reviewbot	July 26, 2016, 9:33 p.m.
'elasticsearch' imported but unused	reviewbot	July 26, 2016, 9:33 p.m.
Should this string be marked for localization?	david	Aug. 8, 2016, 1:54 p.m.
Col: 80 E501 line too long (113 > 79 characters)	reviewbot	July 26, 2016, 9:35 p.m.
'elasticsearch' imported but unused	reviewbot	July 26, 2016, 9:35 p.m.

Change Summary:

Add screenshots.

Added Files:

reviewboard/search/search_backends/elasticsearch.py (Diff revision 1)

Show all issues

noqa?

brennie July 3, 2016, 8:44 p.m.

noqa is a pyflakes / pycodestyle indicator that this line should not be subject to linting. It prevent Review Bot from complaining about the unused import, as we're just importing it to see that it is installed.

reviewboard/search/search_backends/registry.py (Diff revision 1)
Show all issues
```
Returns?
```

reviewboard/admin/forms.py (Diff revision 1)

Show all issues

dddd

mike_conley July 4, 2016, 11:25 a.m.

Hey qhr,

This is a production Review Board instance. Please don't create test reviews or comments here. I encourage you to use the demo instance instead.

Commit:

-	03be0c1e3729ac9e5beb5e7f5f5bced98625c79d
+	14d135aab4325ab473df1f26407b34eb75e74e33

Diff:

Revision 2 (+465 -166)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/siteconfig.py
	reviewboard/admin/urls.py
	reviewboard/admin/views.py
	reviewboard/search/__init__.py
	reviewboard/search/search_engines.py
	reviewboard/search/search_backends/__init__.py
	reviewboard/search/search_backends/base.py
	6 more

Foundation for all this looks great! Nearly all my comments have to do with documentation (I really want us to flesh these out going forward, so we can help foster a better development community and also make life easier for students.)

whoosh_settings.png (Revision 1)
Show all issues
```
Can you make this field longer?
```
elasticsearch_settings.png (Revision 1)
Show all issues
```
Can you make this field longer?
```

reviewboard/admin/forms.py (Diff revision 2)

Show all issues

Missing a period.
At first I was going to say this should say "Form," but it's managing multiple forms. Can you expand the comment and describe what this class is handling?

reviewboard/admin/forms.py (Diff revision 2)

Show all issues

I think maybe, "If enabled, a search field will be provided for ..."

brennie July 11, 2016, 6:09 p.m.
```
These were all copy pasted :)
```

chipx86 July 11, 2016, 6:10 p.m.

Yeah. Many of these are very, very old. But while we're doing a redesign... :)

reviewboard/admin/forms.py (Diff revision 2)
Show all issues
```
"review requests, diffs, and users."
```
reviewboard/admin/forms.py (Diff revision 2)
Show all issues
```
This is redundant. We can get rid of it.
```
reviewboard/admin/forms.py (Diff revision 2)
Show all issues
```
This is redundant. We can get rid of it.
```
reviewboard/admin/forms.py (Diff revision 2)
Show all issues
```
Missing a description.
```

reviewboard/admin/forms.py (Diff revision 2)

Show all issues

"... for the base form and every search engine settings form."""

Which I'm not sure we want, actually. Probably just want it for the base form. I think we should take the specific arguments we need (data, files) and pass those below.

reviewboard/admin/forms.py (Diff revision 2)
Show all issues
```
This is missing a "Returns".
```

reviewboard/admin/forms.py (Diff revision 2)

Show all issues

Let's expand on the error message to make it more helpful:

The search engine "%s" could not be found. If this was provided by an extension, you'll need to make sure the extension is enabled.

reviewboard/admin/forms.py (Diff revision 2)
Show all issues
```
Missing a "Returns".
```
reviewboard/admin/forms.py (Diff revision 2)
Show all issues
```
Missing docs.
```

reviewboard/admin/forms.py (Diff revision 2)

Show all issues

Let's add the same comment here that we have on other forms:

# Reload any important changes into the Django settings.

reviewboard/admin/siteconfig.py (Diff revision 2)

Show all issues

We're going to need to migrate this setting.

brennie July 11, 2016, 6:16 p.m.

It already is. Take a look at SearchBackend.configuration.

reviewboard/admin/siteconfig.py (Diff revision 2)
Show all issues
```
Same error as above.
```
reviewboard/admin/views.py (Diff revision 2)
Show all issues
```
Missing docs.
```

reviewboard/admin/views.py (Diff revision 2)

Show all issues

Let's pass in files as well, in case we have some backend in the future that requires some uploaded license key or something.

reviewboard/search/__init__.py (Diff revision 2)
Show all issues
```
We should have a docstring at the top of this file.
```
reviewboard/search/search_backends/base.py (Diff revision 2)
Show all issues
```
We should have a docstring for this file.
```
reviewboard/search/search_backends/base.py (Diff revision 2)
Show all issues
```
Missing docs.
```

reviewboard/search/search_backends/base.py (Diff revision 2)

Show all issues

Let's flesh this out so it's clear what the purpose and responsibilities are.

reviewboard/search/search_backends/base.py (Diff revision 2)

Show all issues

Let's have a _name suffix, to distingush it from a class or something.

reviewboard/search/search_backends/base.py (Diff revision 2)
Show all issues
```
Missing Args.
```

reviewboard/search/search_backends/base.py (Diff revision 2)

Show all issues

This is kind of vague. I'm not really left with an idea of what goes here. It'd be nice to flesh this out a bit, maybe talk about the kinds of things this might want to check.

reviewboard/search/search_backends/base.py (Diff revision 2)
Show all issues
```
Missing *args.
```
reviewboard/search/search_backends/elasticsearch.py (Diff revision 2)
Show all issues
```
Missing a file docstring.
```

reviewboard/search/search_backends/elasticsearch.py (Diff revision 2)

Show all issues

"Elasticsearch".
(I sure wish the "s" was still capitalized in their name...)

reviewboard/search/search_backends/elasticsearch.py (Diff revision 2)
Show all issues
```
Maybe say "... that the elasticsearch module ..."
```

reviewboard/search/search_backends/registry.py (Diff revision 2)

Show all issues

Would be nice to point to the docs. I have this in the Conditions stuff:

This works as a :py:ref:`registry <registry-guides>`, allowing additional
choices to be added dynamically by extensions or other code.

reviewboard/search/search_backends/registry.py (Diff revision 2)
Show all issues
```
"... with the specified ID."
```

reviewboard/search/search_backends/registry.py (Diff revision 2)

Show all issues

They don't return None if not found. They raise an exception. If we want to return None, we should make sure it's doing that here.

brennie July 11, 2016, 6:27 p.m.

No they don't.

This inherits from ExceptionFreeGetterMixin.

reviewboard/search/search_backends/whoosh.py (Diff revision 2)
Show all issues
```
Missing a file docstring.
```

reviewboard/templates/admin/search_settings.html (Diff revision 2)

Show all issues

Would be nice to narrow down this selector by ID, if we can. (Not sure if the form has one.)

reviewboard/templates/admin/search_settings.html (Diff revision 2)
Show all issues
```
Blank line between these.
```
reviewboard/templates/admin/search_settings.html (Diff revision 2)
Show all issues
```
{% endblock scripts-post %}
```

Change Summary:

Address Christian's issues.
Address the fact that the form data wasn't actually saving.

Commit:

-	14d135aab4325ab473df1f26407b34eb75e74e33
+	be9ff7aeb21de0c537a954dbe61a41a1f863a301

Diff:

Revision 3 (+532 -166)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/siteconfig.py
	reviewboard/admin/urls.py
	reviewboard/admin/views.py
	reviewboard/htdocs/media/10854188_291516057725053_7460375966763285299_o.jpg
	reviewboard/htdocs/media/1420843716467.png
	reviewboard/search/__init__.py
	reviewboard/search/search_engines.py
	8 more

Added Files:

Just a few minor things left. I'm happy with this otherwise.

reviewboard/admin/siteconfig.py (Diff revision 3)
Show all issues
```
search before signals.
```
reviewboard/admin/siteconfig.py (Diff revision 3)
Show all issues
```
This lost its docstring.
```
reviewboard/admin/siteconfig.py (Diff revision 3)
Show all issues
```
No need for the parens and explicit tuple here.
```

reviewboard/search/search_backends/base.py (Diff revision 3)

Show all issues

Missing Returns:

reviewboard/search/search_backends/registry.py (Diff revision 3)
Show all issues
```
Missing a file docstring.
```
reviewboard/search/search_backends/whoosh.py (Diff revision 3)
Show all issues
```
Missing a docstring.
```
reviewboard/templates/admin/search_settings.html (Diff revision 3)
Show all issues
```
No need for the type=
```

Change Summary:

Addressed Christian's issues.

Diff:

Revision 4 (+139 -61)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/siteconfig.py
	reviewboard/admin/views.py
	reviewboard/htdocs/media/10854188_291516057725053_7460375966763285299_o.jpg
	reviewboard/htdocs/media/1420843716467.png
	reviewboard/search/__init__.py
	reviewboard/search/search_backends/base.py
	reviewboard/search/search_backends/elasticsearch.py
	3 more

Tool: Pyflakes
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/admin/forms.py
    reviewboard/admin/views.py
    reviewboard/search/search_backends/registry.py
    reviewboard/search/search_backends/whoosh.py
    reviewboard/search/__init__.py
    reviewboard/search/search_backends/base.py
    reviewboard/search/search_backends/elasticsearch.py

Ignored Files:
    reviewboard/htdocs/media/10854188_291516057725053_7460375966763285299_o.jpg
    reviewboard/templates/admin/search_settings.html
    reviewboard/htdocs/media/1420843716467.png



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/admin/forms.py
    reviewboard/admin/views.py
    reviewboard/search/search_backends/registry.py
    reviewboard/search/search_backends/whoosh.py
    reviewboard/search/__init__.py
    reviewboard/search/search_backends/base.py
    reviewboard/search/search_backends/elasticsearch.py

Ignored Files:
    reviewboard/htdocs/media/10854188_291516057725053_7460375966763285299_o.jpg
    reviewboard/templates/admin/search_settings.html
    reviewboard/htdocs/media/1420843716467.png

reviewboard/admin/views.py (Diff revision 4)
Show all issues
```
 'SearchSettingsForm' imported but unused
```
reviewboard/search/search_backends/base.py (Diff revision 4)
Show all issues
```
Col: 80
 E501 line too long (113 > 79 characters)
```
reviewboard/search/search_backends/elasticsearch.py (Diff revision 4)
Show all issues
```
 'elasticsearch' imported but unused
```

Diff:

Revision 5 (+140 -62)

Show changes

	reviewboard/admin/forms.py
	reviewboard/admin/siteconfig.py
	reviewboard/admin/views.py
	reviewboard/htdocs/media/10854188_291516057725053_7460375966763285299_o.jpg
	reviewboard/htdocs/media/1420843716467.png
	reviewboard/search/__init__.py
	reviewboard/search/search_backends/base.py
	reviewboard/search/search_backends/elasticsearch.py
	3 more

Tool: Pyflakes
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/admin/forms.py
    reviewboard/admin/views.py
    reviewboard/search/search_backends/registry.py
    reviewboard/search/search_backends/whoosh.py
    reviewboard/search/__init__.py
    reviewboard/search/search_backends/base.py
    reviewboard/search/search_backends/elasticsearch.py

Ignored Files:
    reviewboard/htdocs/media/10854188_291516057725053_7460375966763285299_o.jpg
    reviewboard/templates/admin/search_settings.html
    reviewboard/htdocs/media/1420843716467.png



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/admin/siteconfig.py
    reviewboard/admin/forms.py
    reviewboard/admin/views.py
    reviewboard/search/search_backends/registry.py
    reviewboard/search/search_backends/whoosh.py
    reviewboard/search/__init__.py
    reviewboard/search/search_backends/base.py
    reviewboard/search/search_backends/elasticsearch.py

Ignored Files:
    reviewboard/htdocs/media/10854188_291516057725053_7460375966763285299_o.jpg
    reviewboard/templates/admin/search_settings.html
    reviewboard/htdocs/media/1420843716467.png

reviewboard/search/search_backends/base.py (Diff revision 5)
Show all issues
```
Col: 80
 E501 line too long (113 > 79 characters)
```
reviewboard/search/search_backends/elasticsearch.py (Diff revision 5)
Show all issues
```
 'elasticsearch' imported but unused
```

Ship it!

reviewboard/admin/siteconfig.py (Diff revision 5)
Show all issues
```
Should this string be marked for localization?
```

Status: Closed (submitted)

Change Summary:

Pushed to release-3.0.x (34db670)