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

Fixes bug 1207085 - Do not consider 0 as a falsy value in API. #3002

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 3 additions & 3 deletions requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,9 @@ psycopg2==2.6.1
# sha256: U6gisBsElN-WWMBOhyFhgOXsvoWxeGc92Eix-AnVwQA
# sha256: u5PYXKflDWjkGXhSu226YJi7dyUA-C9dsv9KgHEVBck
elasticsearch==1.2
# sha256: vjtAX2ezbybvBNNKkSJSvps-Q0bxtfXQK_0jzH3ZQeg
# sha256: LUb7Hnm9I9XqRjMDxvW0U1bj3LyHzeau5XioOynYK8A
elasticsearch-dsl==0.0.3
# sha256: 8UuM7WZr4UJlQTP-ZznWVrMZxKnsMiitFEvQ8o42pDU
# sha256: Tpdx2a8EZIlHF_PQUFBrYhfHmXcO4AtKaAO5xkWQAY4
elasticsearch-dsl==0.0.8
# sha256: QQvqlt8kefozmGxAw-76nB57qVp_cHEcMVhgu-LWbH8
# sha256: 2Fg3nvWYjUU0u4kJQy1pdCIQCq_ycimdZhM5g2ttrps
urllib3==1.9.1
Expand Down
6 changes: 6 additions & 0 deletions socorro/unittest/external/es/test_supersearch.py
Original file line number Diff line number Diff line change
Expand Up @@ -1451,3 +1451,9 @@ def test_get_return_query_mode(self):
ok_('query' in query)
ok_('aggs' in query)
ok_('size' in query)

def test_get_with_zero(self):
res = self.api.get(
_results_number=0,
)
eq_(len(res['hits']), 0)
8 changes: 5 additions & 3 deletions webapp-django/crashstats/crashstats/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -674,10 +674,12 @@ def kwargs_to_params(self, kwargs):

for param in self.get_annotated_params():
name = param['name']
if param['required'] and not kwargs.get(name):
raise RequiredParameterError(name)
value = kwargs.get(name)
if not value:

# 0 is a perfectly fine value, it should not be considered "falsy".
if not value and value is not 0:
if param['required']:
raise RequiredParameterError(name)
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

So, if this continue happens, does that not mean the value isn't made available? Can't you just do:

if not value and value != 0:
    if param['required']:
        raise RequiredParameterError(name)
    continue

Copy link
Contributor

Choose a reason for hiding this comment

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

I can't put my finger on it but it feels strange that you're doing the ... != 0 check twice.


if isinstance(value, param['type']):
Expand Down
10 changes: 10 additions & 0 deletions webapp-django/crashstats/crashstats/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,16 @@ def test_kwargs_to_params_basics(self):
eq_(result['from_date'], datetime.date.today().strftime('%Y-%m-%d'))
ok_('os' not in result)

# The value `0` is a perfectly fine value and should be kept in the
# parameters, and not ignored as a "falsy" value.
api = models.CrashesByExploitability()
inp = {
'batch': 0,
}
result = api.kwargs_to_params(inp)
# no interesting conversion or checks here
eq_(result, inp)

def test_kwargs_to_params_exceptions(self):
"""the method kwargs_to_params() can take extra care of some special
cases"""
Expand Down
4 changes: 2 additions & 2 deletions webapp-django/crashstats/signature/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ def mocked_supersearch_get(**params):

# Make sure a negative page does not lead to negative offset value.
# But instead it is considered as the page 1 and thus is not added.
ok_('_results_offset' not in params)
eq_(params.get('_results_offset'), 0)

hits = []
for i in range(140):
Expand Down Expand Up @@ -511,7 +511,7 @@ def test_signature_comments_pagination(self):
def mocked_supersearch_get(**params):
assert '_columns' in params

if '_results_offset' in params:
if params.get('_results_offset') != 0:
hits_range = range(100, 140)
else:
hits_range = range(100)
Expand Down
28 changes: 15 additions & 13 deletions webapp-django/crashstats/supersearch/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,25 +20,24 @@

class TestViews(BaseTestViews):

@staticmethod
def setUpClass():
@classmethod
def setUpClass(cls):
super(cls, TestViews).setUpClass()
TestViews.custom_switch = Switch.objects.create(
name='supersearch-custom-query',
active=True,
)

@staticmethod
def tearDownClass():
try:
TestViews.custom_switch.delete()
except AssertionError:
# test_search_waffle_switch removes those switches before, causing
# this error
pass
@classmethod
def tearDownClass(cls):
TestViews.custom_switch.delete()
super(cls, TestViews).tearDownClass()

def test_search_waffle_switch(self):
# Delete the custom-query switch but keep the generic one around.
TestViews.custom_switch.delete()
# Deactivate the switch to verify it's not accessible.
TestViews.custom_switch.active = False
TestViews.custom_switch.save()

url = reverse('supersearch.search_custom')
response = self.client.get(url)
eq_(response.status_code, 404)
Expand All @@ -47,6 +46,9 @@ def test_search_waffle_switch(self):
response = self.client.get(url)
eq_(response.status_code, 404)

TestViews.custom_switch.active = True
TestViews.custom_switch.save()

def test_search(self):
self._login()
url = reverse('supersearch.search')
Expand Down Expand Up @@ -582,7 +584,7 @@ def mocked_supersearch_get(**params):

# Make sure a negative page does not lead to negative offset value.
# But instead it is considered as the page 1 and thus is not added.
ok_('_results_offset' not in params)
eq_(params.get('_results_offset'), 0)

hits = []
for i in range(140):
Expand Down