Fiters are not applied in rest query
Closed this issue · 9 comments
- Rent-o-matic version: 0.1.0
- Python version: 3.6
- Operating System: Linux (Ubuntu)
Description
I've tried to filter results through query string in HTTP request and it occurred that results are not filtered. It is due to mismatch types in function _check in MemRepo because values in domain objects are numbers but filter values are strings.
What I Did
$ http get "http://localhost:5000/storagerooms?filter_size__gt=200"
HTTP/1.0 200 OK
Content-Length: 378
Content-Type: application/json
Date: Thu, 05 Apr 2018 09:27:08 GMT
Server: Werkzeug/0.14.1 Python/3.6.3
[
{
"code": "f853578c-fc0f-4e65-81b8-566c5dffa35a",
"latitude": 51.75436293,
"longitude": -0.09998975,
"price": 39,
"size": 215
},
{
"code": "fe2c3195-aeff-487a-a08f-e0bdc0ec6e9a",
"latitude": 51.74640997,
"longitude": 0.18228006,
"price": 66,
"size": 405
},
{
"code": "913694c6-435a-4366-ba0d-da5334a611b2",
"latitude": 51.45994069,
"longitude": 0.27891577,
"price": 60,
"size": 56
}
]
🙀 is the right emotion. What a bug... Thanks! I'll try to fix it asap.
The main problem is that the repository is attached to a series of posts, and I'd gladly avoid rebasing everithing. I'll see what I can do. Thanks a lot for spotting the bug
I thought I'd have an attempt at solving this. Given the lack of enthusiasm to rebase, here is probably a good place for a possible solution to live for the time being.
First, let's change tests/rest/test_get_storagerooms_list.py:
...
storageroom2_dict = {
'code': 'fe2c3195-aeff-487a-a08f-e0bdc0ec6e9a',
'size': 405,
'price': 66,
'longitude': 0.18228006,
'latitude': 51.74640997,
}
storageroom3_dict = {
'code': '913694c6-435a-4366-ba0d-da5334a611b2',
'size': 56,
'price': 60,
'longitude': 0.27891577,
'latitude': 51.45994069,
}
storageroom4_dict = {
'code': 'eed76e77-55c1-41ce-985d-ca49bf6c0585',
'size': 93,
'price': 48,
'longitude': 0.33894476,
'latitude': 51.39916678,
}
storagerooms1_domain_model = StorageRoom.from_dict(storageroom1_dict)
storagerooms4_domain_model = StorageRoom.from_dict(storageroom4_dict)
storagerooms_dict_list = [storageroom1_dict, storageroom2_dict, storageroom3_dict, storageroom4_dict]
storagerooms = [StorageRoom.from_dict(sr) for sr in storagerooms_dict_list]
...
@mock.patch('rentomatic.use_cases.storageroom_use_cases.StorageRoomListUseCase')
def test_get_with_valid_filter(mock_use_case, client):
mock_use_case().execute.return_value = res.ResponseSuccess([storagerooms1_domain_model, storagerooms4_domain_model])
http_response = client.get('/storagerooms?filter_price__lt=60')
assert json.loads(http_response.data.decode('UTF-8')) == [storageroom1_dict, storageroom4_dict]
assert http_response.status_code == 200
assert http_response.mimetype == 'application/json'
To fix, edit rentomatic/rest/storageroom.py:
for arg, values in request.args.items():
if arg.startswith('filter_'):
parsed_values = values
if arg.replace('filter_', '').split('__')[0] in ['price', 'size']:
parsed_values = int(values)
qrystr_params['filters'][arg.replace('filter_', '')] = parsed_values
This won't work for any filters with more than one parameter, e.g. /storagerooms?filter_price__lt=60+70, but since the examples given all have single values I don't see it as a problem.
Thanks both for the contribution! I'm catching up with some of the things that I left undone, and this is the next one. @AshleyByeUK I will consider your solution, even though it's probably time to review the whole project and article (also because as you mention in issue #3 I never completed what I wanted to. Thanks for the patience!
Hi! So I completely misunderstood the issue, and the solution was much simpler than I thought. Thanks @AshleyByeUK for the solution but in the end I fixed it where it should have been fixed, that is the MemRepo class. I'm also updating the post to explain the reason the bug wasn't spotted. Thanks!
I've just tried the update and one test fails - listing from the repo with the code filter. Both in my own version and one cloned from your repository:
================================================================================================================== FAILURES ==================================================================================================================
___________________________________________________________________________________________________ test_repository_list_with_filters_code ___________________________________________________________________________________________________
storagerooms = [{'code': 'f853578c-fc0f-4e65-81b8-566c5dffa35a', 'latitude': '51.75436293', 'longitude': '-0.09998975', 'price': 39, ...code': 'eed76e77-55c1-41ce-985d-ca49bf6c0585', 'latitude': '51.39916678', 'longitude': '0.33894476', 'price': 48, ...}]
def test_repository_list_with_filters_code(storagerooms):
repo = memrepo.MemRepo(storagerooms)
_check_results(
> repo.list(filters={'code': '913694c6-435a-4366-ba0d-da5334a611b2'}), [storageroom3])
tests/repository/test_memrepo.py:94:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
rentomatic/repository/memrepo.py:32: in list
result = [e for e in result if self._check(e, key, value)]
rentomatic/repository/memrepo.py:32: in <listcomp>
result = [e for e in result if self._check(e, key, value)]
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <rentomatic.repository.memrepo.MemRepo object at 0x10b0e7240>, element = {'code': 'f853578c-fc0f-4e65-81b8-566c5dffa35a', 'latitude': '51.75436293', 'longitude': '-0.09998975', 'price': 39, ...}, key = 'code'
value = '913694c6-435a-4366-ba0d-da5334a611b2'
def _check(self, element, key, value):
if '__' not in key:
key = key + '__eq'
key, operator = key.split('__')
if operator not in ['eq', 'lt', 'gt']:
raise ValueError('Operator {} is not supported'.format(operator))
operator = '__{}__'.format(operator)
> return getattr(element[key], operator)(int(value))
E ValueError: invalid literal for int() with base 10: '913694c6-435a-4366-ba0d-da5334a611b2'
rentomatic/repository/memrepo.py:22: ValueError
==================================================================================================== 1 failed, 41 passed in 0.67 seconds =====================================================================================================How about:
value_type = type(value)
return getattr(element[key], operator)(value_type(value))And you are right, my fault, I ran the tests on the old branch instead of the new one 🙈 By the way my previous solution didn't take into account float numbers as well.
@AshleyByeUK your solution unfortunately doesn't change anything, as you are just casting the value to the type it already has. The test pass because the case showed by @mikus is not tested at repository level (_check() is not tested). At this point I think we need to implement something a tad less trivial and cover all possible cases. I wanted to keep the repository simple to ease the understanding but evidently it is not possible.
Ok, it looks like this time I have a proper solution. I still have to include it in the article, but the code has been updated. Since the repository is aware of the keys (it stores the objects) it can selectively manage their comparison. So the code now is
if key in ['size', 'price']:
return getattr(element[key], operator)(int(value))
elif key in ['latitude', 'longitude']:
return getattr(element[key], operator)(float(value))
return getattr(element[key], operator)(value)This makes all the test pass and fixes the issue spotted by @mikus.
Thanks both for the help!