Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Elasticsearch 1.0 prep #1781

Closed
wants to merge 1 commit into from

2 participants

Rob Hudson Mathieu Pillard
Rob Hudson
Collaborator

These are changes that prepare us for Elasticsearch 1.0 and also work on < 1.0.

mkt/reviewers/tests/test_views_themes.py
@@ -625,16 +625,16 @@ def search(self, q, flagged=False, rereview=False):
return json.loads(themes_search(request).content)['objects']
def test_pending(self):
- eq_(self.search('theme')[0]['id'], self.addon.id)
+ eq_(self.search('theme')[0]['id'], [self.addon.id])
Mathieu Pillard Collaborator
diox added a note

id is a list ? why ? doesn't it break stuff ?

Rob Hudson Collaborator

I was too quick to change this test. I didn't realize this was testing the whole view. We shouldn't change the result of the API. I'll fix this at the view level and not change the API.

Elasticsearch 1.0 returns a list whenever we use fields in a query, which is what values_dict does. It's because ES can support both the fields native type and a list of that type so they are removing ambiguity in the result.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
media/js/zamboni/themes_review.js
@@ -467,7 +467,7 @@ function buildThemeResultRow(theme, review_url, statuses) {
// Rather resolve URLs in backend, infer from slug.
theme.review_url = review_url.replace(
- '__slug__', theme.slug);
- theme.status = statuses[theme.status];
+ '__slug__', theme.slug[0]);
Mathieu Pillard Collaborator
diox added a note

Same question as the id below, why are slug and status lists now ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Rob Hudson
Collaborator

Updated.

Rob Hudson
Collaborator

re-r? @diox

apps/amo/search.py
@@ -310,18 +310,42 @@ def __len__(self):
class DictSearchResults(SearchResults):
def set_objects(self, hits):
- key = 'fields' if self.fields else '_source'
- self.objects = [r[key] for r in hits]
+ if self.fields:
Mathieu Pillard Collaborator
diox added a note

I'd refactor this to avoid the 2 very similar branches.

Rob Hudson Collaborator

refactored

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
apps/amo/search.py
((26 lines not shown))
class ListSearchResults(SearchResults):
def set_objects(self, hits):
if self.fields:
- getter = itemgetter(*self.fields)
- objs = [getter(r['fields']) for r in hits]
+ objs = []
+ for hit in hits:
+ objs.append(tuple([v] if type(v) != list else v
+ for v in hit['fields'].values()))
else:
Mathieu Pillard Collaborator
diox added a note

again very similar I'd refactor that too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Mathieu Pillard
Collaborator

As discussed on IRC: I really don't like the unnecessary lists all over the place. If we could find a way to avoid them I think it'd be much better.

Rob Hudson robhudson commented on the diff
apps/editors/views_themes.py
@@ -224,6 +224,10 @@ def themes_search(request):
themes = list(themes.values_dict('name', 'slug', 'status'))
for theme, reviewer in zip(themes, reviewers):
+ # Collapse single value fields from a list.
+ theme['id'] = theme['id'][0]
+ theme['slug'] = theme['slug'][0]
+ theme['status'] = theme['status'][0]
Rob Hudson Collaborator

Changing this to not use values_dict means the theme becomes an Addon model object. I could probably update how we add the reviewer to this but this change seems pretty harmless here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
apps/zadmin/views.py
((10 lines not shown))
# Get all overrides for this add-on.
- obj['overrides'] = CompatOverride.objects.filter(addon__id=obj['id'])
+ obj['overrides'] = CompatOverride.objects.filter(
+ addon__id=obj['id'][0])
Rob Hudson Collaborator

I looked at removing values_dict here but I get AttributeError: Manager isn't available; AppCompat is abstract. I'd say this is probably one of the ugliest changes. The AppCompat model isn't a real model and is intended to be used with values_dict so only the _source is returned -- not try to turn it into a django model.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Rob Hudson robhudson Make S's value_dict Elasticsearch 1.0 compatible
With Elasticsearch 1.0, any time you query with `fields` the values
always come back as lists. This assumes this everywhere and forces it
for Elasticsearch < 1.0 to be consistent.
e559f94
Rob Hudson
Collaborator

Ok. This is cleaned up now.

I'm only coercing to list if values or values_dict specifies a subset of fields. Otherwise I'm returning the _source as it is in the index. This matches what ES will do and is less impactful. It was incorrect before b/c values_dict without fields should just return the _source and not try to convert it to a django model object.

Mathieu Pillard
Collaborator

r+

Rob Hudson
Collaborator

Thanks, merged in 0fab353

Rob Hudson robhudson closed this
Rob Hudson robhudson deleted the branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Feb 26, 2014
  1. Rob Hudson

    Make S's value_dict Elasticsearch 1.0 compatible

    robhudson authored
    With Elasticsearch 1.0, any time you query with `fields` the values
    always come back as lists. This assumes this everywhere and forces it
    for Elasticsearch < 1.0 to be consistent.
This page is out of date. Refresh to see the latest.
35 apps/amo/search.py
View
@@ -1,5 +1,4 @@
import logging
-from operator import itemgetter
from django.conf import settings as dj_settings
@@ -310,18 +309,38 @@ def __len__(self):
class DictSearchResults(SearchResults):
def set_objects(self, hits):
- key = 'fields' if self.fields else '_source'
- self.objects = [r[key] for r in hits]
+ objs = []
+
+ if self.fields:
+ # When fields are specified in `values_dict(...)` we return the
+ # fields. Each field is coerced to a list to match the
+ # Elasticsearch >= 1.0 style.
+ for h in hits:
+ hit = {}
+ for field, value in h['fields'].items():
+ if type(value) != list:
+ value = [value]
+ hit[field] = value
+ objs.append(hit)
+ self.objects = objs
+ else:
+ self.objects = [r['_source'] for r in hits]
+
+ return self.objects
class ListSearchResults(SearchResults):
def set_objects(self, hits):
- if self.fields:
- getter = itemgetter(*self.fields)
- objs = [getter(r['fields']) for r in hits]
- else:
- objs = [r['_source'].values() for r in hits]
+ key = 'fields' if self.fields else '_source'
+
+ # When fields are specified in `values(...)` we return the fields. Each
+ # field is coerced to a list to match the Elasticsearch >= 1.0 style.
+ objs = []
+ for hit in hits:
+ objs.append(tuple([v] if key == 'fields' and type(v) != list else v
+ for v in hit[key].values()))
+
self.objects = objs
4 apps/amo/tests/test_search.py
View
@@ -251,7 +251,7 @@ def test_values(self):
eq_(qs._build_query(), {'fields': ['id', 'name']})
def test_values_result(self):
- addons = [(a.id, a.slug) for a in self._addons]
+ addons = [([a.id], [a.slug]) for a in self._addons]
qs = Addon.search().values('slug').order_by('id')
eq_(list(qs), addons)
@@ -264,7 +264,7 @@ def test_empty_values_dict(self):
eq_(qs._build_query(), {})
def test_values_dict_result(self):
- addons = [{'id': a.id, 'slug': a.slug} for a in self._addons]
+ addons = [{'id': [a.id], 'slug': [a.slug]} for a in self._addons]
qs = Addon.search().values_dict('slug').order_by('id')
eq_(list(qs), list(addons))
2  apps/editors/tests/test_views_themes.py
View
@@ -616,7 +616,7 @@ def search(self, q, flagged=False, rereview=False):
'flagged' if flagged else '')}
request = amo.tests.req_factory_factory(
- reverse('editors.themes.list'),
+ reverse('editors.themes.search'),
user=UserProfile.objects.get(username='senior_persona_reviewer'))
request.GET = get_query
return json.loads(themes_search(request).content)['objects']
4 apps/editors/views_themes.py
View
@@ -224,6 +224,10 @@ def themes_search(request):
themes = list(themes.values_dict('name', 'slug', 'status'))
for theme, reviewer in zip(themes, reviewers):
+ # Collapse single value fields from a list.
+ theme['id'] = theme['id'][0]
+ theme['slug'] = theme['slug'][0]
+ theme['status'] = theme['status'][0]
Rob Hudson Collaborator

Changing this to not use values_dict means the theme becomes an Addon model object. I could probably update how we add the reviewer to this but this change seems pretty harmless here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
# Dehydrate.
theme['reviewer'] = reviewer
2  apps/reviews/tests/test_models.py
View
@@ -81,7 +81,7 @@ def setUp(self):
def get_bayesian_rating(self):
q = Addon.search().filter(id=self.addon.id)
- return list(q.values_dict('bayesian_rating'))[0]['bayesian_rating']
+ return list(q.values_dict('bayesian_rating'))[0]['bayesian_rating'][0]
def test_created(self):
eq_(self.get_bayesian_rating(), 0.0)
28 mkt/lookup/views.py
View
@@ -354,11 +354,17 @@ def user_search(request):
# id is added implictly by the ES filter. Add it explicitly:
fields = ['id'] + list(fields)
qs = UserProfile.objects.filter(pk=q).values(*fields)
+ db = True
else:
qs = (UserProfile.search().query(or_=_expand_query(q, fields))
.values_dict(*fields))
qs = _slice_results(request, qs)
+ db = False
for user in qs:
+ if not db:
+ for field in ('id',) + fields:
+ if field in user:
+ user[field] = user[field][0]
user['url'] = reverse('lookup.user_summary', args=[user['id']])
user['name'] = user['username']
results.append(user)
@@ -402,14 +408,20 @@ def app_search(request):
.values_dict(*fields))
qs = _slice_results(request, qs)
for app in qs:
- app['url'] = reverse('lookup.app_summary', args=[app['id']])
- # ES returns a list of localized names but database queries do not.
- if type(app['name']) != list:
- app['name'] = [app['name__localized_string']]
- for name in app['name']:
- dd = app.copy()
- dd['name'] = name
- results.append(dd)
+ if 'name__localized_string' in app:
+ # This is a result from the database.
+ app['url'] = reverse('lookup.app_summary', args=[app['id']])
+ app['name'] = app['name__localized_string']
+ results.append(app)
+ else:
+ # This is a result from elasticsearch which returns lists.
+ app['url'] = reverse('lookup.app_summary', args=[app['id'][0]])
+ for field in ('id', 'app_slug'):
+ app[field] = app.get(field, [None])[0]
+ for name in app['name']:
+ dd = app.copy()
+ dd['name'] = name
+ results.append(dd)
return {'results': results}
8 mkt/reviewers/tests/test_views_themes.py
View
@@ -9,19 +9,19 @@
from nose.tools import eq_
from pyquery import PyQuery as pq
-from access.models import GroupUser
-from addons.models import Persona
import amo
import amo.tests
+from access.models import GroupUser
+from addons.models import Persona
from amo.tests import addon_factory, days_ago
from amo.urlresolvers import reverse
from devhub.models import ActivityLog
from editors.models import RereviewQueueTheme, ReviewerScore, ThemeLock
+from users.models import UserProfile
+
import mkt.constants.reviewers as rvw
from mkt.reviewers.views_themes import _get_themes, home, themes_search
from mkt.site.fixtures import fixture
-from users.models import UserProfile
-
class ThemeReviewTestMixin(object):
fixtures = fixture('group_admin', 'user_admin', 'user_admin_group',
4 mkt/reviewers/views_themes.py
View
@@ -199,6 +199,10 @@ def themes_search(request):
themes = list(themes.values_dict('name', 'slug', 'status'))
for theme, reviewer in zip(themes, reviewers):
+ # Collapse single value fields from a list.
+ theme['id'] = theme['id'][0]
+ theme['slug'] = theme['slug'][0]
+ theme['status'] = theme['status'][0]
# Dehydrate.
theme['reviewer'] = reviewer
return {'objects': themes, 'meta': {'total_count': len(themes)}}
Something went wrong with that request. Please try again.