Address Copilot review: reduce coupling, fix types, store stripped table

- Store stripped table name after validation (was validating stripped
  but storing original)
- Change filter_in param type from list to Sequence[Any] for broader
  compatibility (tuples, sets)
- Delegate execute() to client.records.get() instead of private
  _scoped_odata/_get_multiple internals
- Update tests to mock records.get instead of internal OData methods

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: tpellissier

src/PowerPlatform/Dataverse/models/query_builder.py

@@ -39,7 +39,7 @@
 
 from __future__ import annotations
 
-from typing import Any, Dict, Iterable, List, Optional, Union
+from typing import Any, Dict, Iterable, List, Optional, Sequence, Union
 
 from .filters import FilterExpression, _format_value
 
@@ -70,7 +70,8 @@ class QueryBuilder:
     """
 
     def __init__(self, table: str) -> None:
-        if not table or not table.strip():
+        table = table.strip() if table else ""
+        if not table:
             raise ValueError("table name is required")
         self.table = table
         self._select: List[str] = []
@@ -215,7 +216,7 @@ def filter_not_null(self, column: str) -> QueryBuilder:
 
     # --------------------------------------------------------- filter: special
 
-    def filter_in(self, column: str, values: list) -> QueryBuilder:
+    def filter_in(self, column: str, values: Sequence[Any]) -> QueryBuilder:
         """Add an ``in`` filter: ``column in (val1, val2, ...)``.
 
         :param column: Column name (will be lowercased).
@@ -437,23 +438,21 @@ def execute(self, *, by_page: bool = False) -> Union[Iterable[Dict[str, Any]], I
         params = self.build()
         client = self._query_ops._client
 
-        def _paged() -> Iterable[List[Dict[str, Any]]]:
-            with client._scoped_odata() as od:
-                yield from od._get_multiple(
-                    params["table"],
-                    select=params.get("select"),
-                    filter=params.get("filter"),
-                    orderby=params.get("orderby"),
-                    top=params.get("top"),
-                    expand=params.get("expand"),
-                    page_size=params.get("page_size"),
-                )
+        pages = client.records.get(
+            params["table"],
+            select=params.get("select"),
+            filter=params.get("filter"),
+            orderby=params.get("orderby"),
+            top=params.get("top"),
+            expand=params.get("expand"),
+            page_size=params.get("page_size"),
+        )
 
         if by_page:
-            return _paged()
+            return pages
 
         def _flat() -> Iterable[Dict[str, Any]]:
-            for page in _paged():
+            for page in pages:
                 yield from page
 
         return _flat()

tests/unit/models/test_query_builder.py

@@ -416,24 +416,21 @@ def test_execute_without_query_ops_raises(self):
             qb.execute()
         self.assertIn("client.query.builder()", str(ctx.exception))
 
-    def test_execute_calls_get_multiple(self):
-        """execute() should call _get_multiple via the client with built params."""
+    def test_execute_calls_records_get(self):
+        """execute() should delegate to client.records.get() with built params."""
         mock_query_ops = MagicMock()
         mock_client = mock_query_ops._client
-        mock_odata = MagicMock()
-        mock_client._scoped_odata.return_value.__enter__ = MagicMock(return_value=mock_odata)
-        mock_client._scoped_odata.return_value.__exit__ = MagicMock(return_value=False)
-        mock_odata._get_multiple.return_value = iter([[{"name": "Test"}]])
+        mock_client.records.get.return_value = iter([[{"name": "Test"}]])
 
         qb = QueryBuilder("account")
         qb._query_ops = mock_query_ops
         qb.select("name", "revenue").filter_eq("statecode", 0).order_by("revenue", descending=True).top(100).page_size(
             50
         ).expand("primarycontactid")
 
-        result = list(qb.execute())
+        list(qb.execute())
 
-        mock_odata._get_multiple.assert_called_once_with(
+        mock_client.records.get.assert_called_once_with(
             "account",
             select=["name", "revenue"],
             filter="statecode eq 0",
@@ -446,11 +443,7 @@ def test_execute_calls_get_multiple(self):
     def test_execute_returns_flat_records_by_default(self):
         mock_query_ops = MagicMock()
         mock_client = mock_query_ops._client
-        mock_odata = MagicMock()
-        mock_client._scoped_odata.return_value.__enter__ = MagicMock(return_value=mock_odata)
-        mock_client._scoped_odata.return_value.__exit__ = MagicMock(return_value=False)
-
-        mock_odata._get_multiple.return_value = iter([[{"name": "A"}, {"name": "B"}], [{"name": "C"}]])
+        mock_client.records.get.return_value = iter([[{"name": "A"}, {"name": "B"}], [{"name": "C"}]])
 
         qb = QueryBuilder("account")
         qb._query_ops = mock_query_ops
@@ -464,13 +457,10 @@ def test_execute_returns_flat_records_by_default(self):
     def test_execute_by_page_returns_pages(self):
         mock_query_ops = MagicMock()
         mock_client = mock_query_ops._client
-        mock_odata = MagicMock()
-        mock_client._scoped_odata.return_value.__enter__ = MagicMock(return_value=mock_odata)
-        mock_client._scoped_odata.return_value.__exit__ = MagicMock(return_value=False)
 
         page1 = [{"name": "A"}, {"name": "B"}]
         page2 = [{"name": "C"}]
-        mock_odata._get_multiple.return_value = iter([page1, page2])
+        mock_client.records.get.return_value = iter([page1, page2])
 
         qb = QueryBuilder("account")
         qb._query_ops = mock_query_ops
@@ -483,16 +473,13 @@ def test_execute_by_page_returns_pages(self):
     def test_execute_passes_none_for_empty_options(self):
         mock_query_ops = MagicMock()
         mock_client = mock_query_ops._client
-        mock_odata = MagicMock()
-        mock_client._scoped_odata.return_value.__enter__ = MagicMock(return_value=mock_odata)
-        mock_client._scoped_odata.return_value.__exit__ = MagicMock(return_value=False)
-        mock_odata._get_multiple.return_value = iter([])
+        mock_client.records.get.return_value = iter([])
 
         qb = QueryBuilder("account")
         qb._query_ops = mock_query_ops
         list(qb.execute())
 
-        mock_odata._get_multiple.assert_called_once_with(
+        mock_client.records.get.assert_called_once_with(
             "account",
             select=None,
             filter=None,
@@ -507,17 +494,14 @@ def test_execute_with_where_expressions(self):
 
         mock_query_ops = MagicMock()
         mock_client = mock_query_ops._client
-        mock_odata = MagicMock()
-        mock_client._scoped_odata.return_value.__enter__ = MagicMock(return_value=mock_odata)
-        mock_client._scoped_odata.return_value.__exit__ = MagicMock(return_value=False)
-        mock_odata._get_multiple.return_value = iter([])
+        mock_client.records.get.return_value = iter([])
 
         qb = QueryBuilder("account")
         qb._query_ops = mock_query_ops
         qb.where((eq("statecode", 0) | eq("statecode", 1)) & gt("revenue", 100000))
         list(qb.execute())
 
-        call_args = mock_odata._get_multiple.call_args
+        call_args = mock_client.records.get.call_args
         self.assertEqual(
             call_args.kwargs["filter"],
             "((statecode eq 0 or statecode eq 1) and revenue gt 100000)",

tests/unit/test_query_operations.py

@@ -97,15 +97,14 @@ def test_builder_execute_flat_multiple_pages(self):
 
     def test_builder_execute_by_page(self):
         """execute(by_page=True) should yield pages."""
-        page1 = [{"accountid": "1"}]
-        page2 = [{"accountid": "2"}]
-        self.client._odata._get_multiple.return_value = iter([page1, page2])
+        self.client._odata._get_multiple.return_value = iter([[{"accountid": "1"}], [{"accountid": "2"}]])
 
         pages = list(self.client.query.builder("account").execute(by_page=True))
 
         self.assertEqual(len(pages), 2)
-        self.assertEqual(pages[0], page1)
-        self.assertEqual(pages[1], page2)
+        self.assertEqual(len(pages[0]), 1)
+        self.assertEqual(pages[0][0]["accountid"], "1")
+        self.assertEqual(pages[1][0]["accountid"], "2")
 
     def test_builder_execute_all_params(self):
         """builder().execute() should forward all parameters."""