Add opt-in grouping for ORM filters

CHANGELOG.md

@@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 ### Added
 
 - service: the chainable filter accepts the OData operator names `__le` / `__ge` as lookups, next to the Django-style `__lte` / `__gte` - Vitor Coelho
+- service: opt-in `filter(group=True, ...)` preserves grouping when combining ORM filters and chained calls (#280) - Umut Alisoglu
 
 ### Fixed
 

docs/usage/querying.rst

@@ -96,6 +96,38 @@ Print unique identification (Id) of all employees with name John Smith:
         print(smith.EmployeeID)
 
 
+Grouping ORM filter conditions
+------------------------------
+
+Pass ``group=True`` to keep each condition grouped when combining ``and`` and
+``or``. This is useful for ``__in`` lookups, which expand to alternatives:
+
+.. code-block:: python
+
+    request = service.entity_sets.Employees.get_entities().filter(
+        ID__in=[2, 3], NameLast="Smith", group=True)
+    # $filter: ((ID eq 2 or ID eq 3) and (NameLast eq 'Smith'))
+
+Both alternatives must now also match ``NameLast``. Without grouping, OData's
+operator precedence lets the first alternative match regardless of the name.
+The same option groups ``Q`` arguments and conditions inside a ``Q`` object:
+
+.. code-block:: python
+
+    from pyodata.v2.service import FilterExpression as Q
+
+    request = service.entity_sets.Employees.get_entities().filter(
+        Q(ID=2) | Q(ID=3), NameLast="Smith", group=True)
+
+On a chained call, ``group=True`` also groups the whole preceding filter before
+adding the new conditions. A string filter still replaces the preceding filter,
+as it does without grouping. Grouping is selected per call and defaults to
+``False``, preserving existing query serialization.
+
+``group`` is a filter option; to filter an entity property with that exact name,
+pass it inside ``Q(group=value)``.
+
+
 Get a count of entities
 -----------------------
 

pyodata/v2/service.py

@@ -1199,10 +1199,11 @@ class GetEntitySetFilterChainable:
         'eq'
     ]
 
-    def __init__(self, entity_type, filter_expressions, exprs):
+    def __init__(self, entity_type, filter_expressions, exprs, group=False):
         self._entity_type = entity_type
         self._filter_expressions = filter_expressions
         self._expressions = exprs
+        self._group = group
 
     @property
     def expressions(self):
@@ -1267,6 +1268,8 @@ def _decode_expression(self, expr, val):
         return expression
 
     def _combine_expressions(self, expressions):
+        if self._group and len(expressions) > 1:
+            expressions = [f'({expression})' for expression in expressions]
         return ' and '.join(expressions)
 
     # pylint: disable=too-many-return-statements
@@ -1330,16 +1333,24 @@ def __getattr__(self, name):
         proprty = self._entity_type.proprty(name)
         return GetEntitySetFilter(proprty)
 
-    def _set_filter(self, filter_val):
+    def _set_filter(self, filter_val, group=False):
+        if group:
+            if not filter_val:
+                return
+            filter_val = f'({filter_val})'
+            if self._filter:
+                self._filter = f'({self._filter})'
         filter_text = self._filter + ' and ' if self._filter else ''
         filter_text += filter_val
         self._filter = filter_text
 
-    def filter(self, *args, **kwargs):
+    def filter(self, *args, group=False, **kwargs):
+        """Set filters, optionally grouping conditions before joining them with and."""
         if args and len(args) == 1 and isinstance(args[0], str):
-            self._filter = args[0]
+            self._filter = f'({args[0]})' if group and args[0] else args[0]
         else:
-            self._set_filter(str(GetEntitySetFilterChainable(self._entity_type, args, kwargs)))
+            self._set_filter(str(GetEntitySetFilterChainable(self._entity_type, args, kwargs, group=group)),
+                             group=group)
 
         return self
 

tests/test_filter_grouping.py

@@ -0,0 +1,142 @@
+"""Opt-in grouping must preserve the meaning of combined filter conditions."""
+
+import ast
+import re
+
+import pytest
+import requests
+import responses
+
+from pyodata.v2.service import FilterExpression as Q, Service
+
+
+ROWS = [
+    {'ID': 1, 'NameLast': 'Smith', 'NickName': 'Al'},
+    {'ID': 2, 'NameLast': 'Jones', 'NickName': 'Tim'},
+    {'ID': 3, 'NameLast': 'Smith', 'NickName': 'Tim'},
+    {'ID': 4, 'NameLast': 'Jones', 'NickName': 'Al'},
+    {'ID': 5, 'NameLast': 'Smith', 'NickName': 'Other'},
+    {'ID': 6, 'NameLast': 'Müller', 'NickName': 'René'},
+]
+
+
+@pytest.fixture
+def employees(schema):
+    """Build requests without making an HTTP connection."""
+    return Service('http://odata.example.com', schema, requests).entity_sets.Employees.get_entities()
+
+
+def selected_ids(expression):
+    """Use Python's independent parser for the tested eq/and/or OData subset."""
+    tree = ast.parse(re.sub(r'\beq\b', '==', expression), mode='eval')
+
+    def evaluate(node, row):
+        if isinstance(node, ast.Constant):
+            return node.value
+        if isinstance(node, ast.Name):
+            return row[node.id]
+        if isinstance(node, ast.Compare) and len(node.ops) == 1 and isinstance(node.ops[0], ast.Eq):
+            return evaluate(node.left, row) == evaluate(node.comparators[0], row)
+        if isinstance(node, ast.BoolOp):
+            values = (evaluate(value, row) for value in node.values)
+            if isinstance(node.op, ast.And):
+                return all(values)
+            if isinstance(node.op, ast.Or):
+                return any(values)
+        raise AssertionError(f'Unexpected filter node: {ast.dump(node)}')
+
+    return {row['ID'] for row in ROWS if evaluate(tree.body, row)}
+
+
+@pytest.mark.parametrize('build, expected, expected_filter, expected_url', [
+    pytest.param(
+        lambda request: request.filter(Q(ID=1) | Q(NickName='Tim'), NameLast='Smith', group=True),
+        {1, 3}, "((NameLast eq 'Smith') and ((ID eq 1) or (NickName eq 'Tim')))",
+        'http://odata.example.com/Employees?%24filter='
+        '%28%28NameLast+eq+%27Smith%27%29+and+%28%28ID+eq+1%29+or+%28NickName+eq+%27Tim%27%29%29%29',
+        id='or-with-keyword'),
+    pytest.param(
+        lambda request: request.filter(ID__in=[2, 3], NameLast='Smith', group=True),
+        {3}, "((ID eq 2 or ID eq 3) and (NameLast eq 'Smith'))",
+        'http://odata.example.com/Employees?%24filter='
+        '%28%28ID+eq+2+or+ID+eq+3%29+and+%28NameLast+eq+%27Smith%27%29%29',
+        id='in-with-keyword'),
+    pytest.param(
+        lambda request: request.filter(Q(ID__in=[2, 3], NameLast='Smith'), group=True),
+        {3}, "((ID eq 2 or ID eq 3) and (NameLast eq 'Smith'))",
+        'http://odata.example.com/Employees?%24filter='
+        '%28%28ID+eq+2+or+ID+eq+3%29+and+%28NameLast+eq+%27Smith%27%29%29',
+        id='in-within-q'),
+    pytest.param(
+        lambda request: request.filter(Q(ID__in=[2, 3], NameLast='Smith') | Q(ID=5), group=True),
+        {3, 5}, "(((ID eq 2 or ID eq 3) and (NameLast eq 'Smith')) or (ID eq 5))",
+        'http://odata.example.com/Employees?%24filter='
+        '%28%28%28ID+eq+2+or+ID+eq+3%29+and+%28NameLast+eq+%27Smith%27%29%29+or+%28ID+eq+5%29%29',
+        id='compound-or'),
+    pytest.param(
+        lambda request: request.filter(Q(ID__in=[2, 3]) & Q(NameLast='Smith'), group=True),
+        {3}, "((ID eq 2 or ID eq 3) and (NameLast eq 'Smith'))",
+        'http://odata.example.com/Employees?%24filter='
+        '%28%28ID+eq+2+or+ID+eq+3%29+and+%28NameLast+eq+%27Smith%27%29%29',
+        id='binary-and'),
+    pytest.param(
+        lambda request: request.filter(Q(ID=2) | Q(ID=3), Q(NameLast='Smith'), group=True),
+        {3}, "(((ID eq 2) or (ID eq 3)) and (NameLast eq 'Smith'))",
+        'http://odata.example.com/Employees?%24filter='
+        '%28%28%28ID+eq+2%29+or+%28ID+eq+3%29%29+and+%28NameLast+eq+%27Smith%27%29%29',
+        id='multiple-q'),
+    pytest.param(
+        lambda request: request.filter(Q(ID=2) | Q(ID=3)).filter(NameLast='Smith', group=True),
+        {3}, "((ID eq 2) or (ID eq 3)) and (NameLast eq 'Smith')",
+        'http://odata.example.com/Employees?%24filter='
+        '%28%28ID+eq+2%29+or+%28ID+eq+3%29%29+and+%28NameLast+eq+%27Smith%27%29',
+        id='group-previous-call'),
+    pytest.param(
+        lambda request: request.filter(ID__in=[2, 3], group=True).filter(NameLast='Smith'),
+        {3}, "(ID eq 2 or ID eq 3) and NameLast eq 'Smith'",
+        'http://odata.example.com/Employees?%24filter='
+        '%28ID+eq+2+or+ID+eq+3%29+and+NameLast+eq+%27Smith%27',
+        id='group-before-legacy-tail'),
+    pytest.param(
+        lambda request: request.filter('ID eq 2 or ID eq 3').filter(NameLast='Smith', group=True),
+        {3}, "(ID eq 2 or ID eq 3) and (NameLast eq 'Smith')",
+        'http://odata.example.com/Employees?%24filter='
+        '%28ID+eq+2+or+ID+eq+3%29+and+%28NameLast+eq+%27Smith%27%29',
+        id='raw-before-group'),
+    pytest.param(
+        lambda request: request.filter('ID eq 2 or ID eq 3', group=True),
+        {2, 3}, '(ID eq 2 or ID eq 3)',
+        'http://odata.example.com/Employees?%24filter=%28ID+eq+2+or+ID+eq+3%29',
+        id='raw-group'),
+    pytest.param(
+        lambda request: request.filter(ID=2).filter('ID eq 3', group=True),
+        {3}, '(ID eq 3)',
+        'http://odata.example.com/Employees?%24filter=%28ID+eq+3%29',
+        id='raw-replaces-previous'),
+    pytest.param(
+        lambda request: request.filter(NameLast='Müller', NickName='René', group=True),
+        {6}, "((NameLast eq 'Müller') and (NickName eq 'René'))",
+        'http://odata.example.com/Employees?%24filter='
+        '%28%28NameLast+eq+%27M%C3%BCller%27%29+and+%28NickName+eq+%27Ren%C3%A9%27%29%29',
+        id='unicode-encoding'),
+])
+@responses.activate
+def test_grouped_filter_selects_expected_rows(employees, build, expected, expected_filter, expected_url):
+    query = build(employees)
+    expression = query.get_query_params()['$filter']
+    assert expression == expected_filter
+    assert selected_ids(expression) == expected
+    responses.add(responses.GET, expected_url, json={'d': {'results': []}})
+    assert query.execute() == []
+    assert responses.calls[0].request.url == expected_url
+
+
+def test_default_grouping_keeps_legacy_query(employees):
+    """Existing applications keep their exact query text unless they opt in."""
+    expression = employees.filter(ID__in=[2, 3], NameLast='Smith').get_query_params()['$filter']
+    assert expression == "ID eq 2 or ID eq 3 and NameLast eq 'Smith'"
+    assert selected_ids(expression) == {2, 3}
+
+
+def test_empty_grouped_filter_is_a_noop(employees):
+    assert '$filter' not in employees.filter(group=True).get_query_params()