service: group the alternatives of the __in filter lookup

CHANGELOG.md

@@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ### Fixed
 
+- service: the `__in` filter lookup wraps its `or` alternatives in parentheses, so combining it with another lookup (e.g. `filter(ID__in=[1, 2], NickName='Tim')`) no longer changes the meaning of the filter - Vitor Coelho
 - service: `update_entity` accepts `200 OK` as well as `204 No Content`, so updates against services such as SAP SuccessFactors no longer raise `HttpError` (#136) - Sena Köse
 
 ## [1.12.1]

pyodata/v2/service.py

@@ -1298,7 +1298,11 @@ def _build_expression(self, field_name, operator, value):
                 high_bound = target_field.to_literal(value[1])
                 return f'{field_name} gte {low_bound} and {field_name} lte {high_bound}'
             case 'in':
-                literal_values = (f'{field_name} eq {target_field.to_literal(item)}' for item in value)
+                literal_values = [f'{field_name} eq {target_field.to_literal(item)}' for item in value]
+                if len(literal_values) > 1:
+                    # 'and' binds tighter than 'or', so the alternatives must be
+                    # grouped to stay correct when combined with other lookups
+                    return f"({' or '.join(literal_values)})"
                 return ' or '.join(literal_values)
             case 'eq':
                 return f'{field_name} eq {value}'

tests/test_service_v2.py

@@ -2601,7 +2601,7 @@ def test_count_with_chainable_filter_in_operator(service):
 
     responses.add(
         responses.GET,
-        f"{service.url}/Employees/$count?$filter=ID%20eq%201%20or%20ID%20eq%202%20or%20ID%20eq%203",
+        f"{service.url}/Employees/$count?$filter=%28ID%20eq%201%20or%20ID%20eq%202%20or%20ID%20eq%203%29",
         json=3,
         status=200)
 
@@ -2613,6 +2613,37 @@ def test_count_with_chainable_filter_in_operator(service):
     assert request.execute() == 3
 
 
+@responses.activate
+def test_count_with_chainable_filter_in_operator_and_other_lookup(service):
+    """Check that the alternatives of $filter in are grouped when combined with another lookup"""
+
+    # pylint: disable=redefined-outer-name
+
+    responses.add(
+        responses.GET,
+        f"{service.url}/Employees/$count?$filter=%28ID%20eq%201%20or%20ID%20eq%202%29%20and%20NickName%20eq%20%27Tim%27",
+        json=1,
+        status=200)
+
+    employees = service.entity_sets.Employees.get_entities()
+    request = employees.filter(ID__in=[1, 2], NickName="Tim").count()
+
+    assert request.get_query_params()['$filter'] == "(ID eq 1 or ID eq 2) and NickName eq 'Tim'"
+    assert request.execute() == 1
+
+
+@responses.activate
+def test_count_with_chainable_filter_in_operator_single_value(service):
+    """Check that $filter in with a single value is a plain comparison"""
+
+    # pylint: disable=redefined-outer-name
+
+    employees = service.entity_sets.Employees.get_entities()
+    request = employees.filter(ID__in=[1]).count()
+
+    assert request.get_query_params()['$filter'] == "ID eq 1"
+
+
 @responses.activate
 def test_count_with_chainable_filter_startswith_operator(service):
     """Check getting $count with $filter in"""
@@ -3204,4 +3235,4 @@ def hook(response):
 
 def test_service_without_response_hook_works(service):
     """response_hook defaults to None and does not affect normal operation"""
-    assert service.response_hook is None
+    assert service.response_hook is None