Address PR review: add QueryParams TypedDict, unbounded query guard, filter_raw warning

Author: Abel Milash

src/PowerPlatform/Dataverse/models/query_builder.py

@@ -46,14 +46,32 @@
 
 from __future__ import annotations
 
-from typing import Any, Collection, Dict, Iterable, List, Optional, Sequence, Union
+from typing import Any, Collection, Dict, Iterable, List, Optional, Sequence, TypedDict, Union
 
 import pandas as pd
 
 from . import filters
 from .record import Record
 
-__all__ = ["QueryBuilder", "ExpandOption"]
+__all__ = ["QueryBuilder", "QueryParams", "ExpandOption"]
+
+
+class QueryParams(TypedDict, total=False):
+    """Typed dictionary returned by :meth:`QueryBuilder.build`.
+
+    Provides IDE autocomplete when passing build results to
+    ``client.records.get()`` manually.
+    """
+
+    table: str
+    select: List[str]
+    filter: str
+    orderby: List[str]
+    expand: List[str]
+    top: int
+    page_size: int
+    count: bool
+    include_annotations: str
 
 
 class ExpandOption:
@@ -390,6 +408,11 @@ def filter_raw(self, filter_string: str) -> QueryBuilder:
         Use this for complex filters not covered by other methods.
         Column names in the filter string should be lowercase.
 
+        .. warning::
+            The filter string is passed directly to Dataverse without validation.
+            Ensure it follows OData filter syntax; a malformed expression will result
+            in a ``400 Bad Request`` error from the server.
+
         :param filter_string: Raw OData filter expression.
         :return: Self for method chaining.
 
@@ -585,19 +608,19 @@ def expand(self, *relations: Union[str, ExpandOption]) -> QueryBuilder:
 
     # --------------------------------------------------------------- build
 
-    def build(self) -> dict:
+    def build(self) -> QueryParams:
         """Build query parameters dictionary.
 
-        Returns a dictionary suitable for passing to the OData layer.
-        All ``filter_*()`` and ``where()`` clauses are AND-joined into
-        a single ``filter`` string in call order.
+        Returns a :class:`QueryParams` dictionary suitable for passing to
+        the OData layer.  All ``filter_*()`` and ``where()`` clauses are
+        AND-joined into a single ``filter`` string in call order.
 
         :return: Dictionary with ``table`` and optionally ``select``,
             ``filter``, ``orderby``, ``expand``, ``top``, ``page_size``,
             ``count``, ``include_annotations``.
-        :rtype: dict
+        :rtype: QueryParams
         """
-        params: dict = {"table": self.table}
+        params: QueryParams = {"table": self.table}
         if self._select:
             params["select"] = list(self._select)
         if self._filter_parts:
@@ -622,6 +645,23 @@ def build(self) -> dict:
             params["include_annotations"] = self._include_annotations
         return params
 
+    # --------------------------------------------------------------- guards
+
+    def _validate_constraints(self) -> None:
+        """Raise if the query has no limiting constraints.
+
+        At least one of ``select``, ``filter``, or ``top`` must be set
+        before executing a query to prevent accidental full-table scans.
+
+        :raises ValueError: If none of ``select()``, ``filter_*()``,
+            ``where()``, or ``top()`` has been called.
+        """
+        if not (self._select or self._filter_parts or self._top is not None):
+            raise ValueError(
+                "Unbounded query: set at least one of select(), filter_*(), "
+                "where(), or top() before calling execute() or to_dataframe()."
+            )
+
     # --------------------------------------------------------------- execute
 
     def execute(self, *, by_page: bool = False) -> Union[Iterable[Record], Iterable[List[Record]]]:
@@ -636,6 +676,11 @@ def execute(self, *, by_page: bool = False) -> Union[Iterable[Record], Iterable[
         instances should use :meth:`build` to get parameters and pass them
         to ``client.records.get()`` manually.
 
+        At least one of ``select()``, ``filter_*()``, ``where()``, or
+        ``top()`` must be called before ``execute()``; otherwise a
+        :class:`ValueError` is raised to prevent accidental full-table
+        scans.
+
         :param by_page: If ``True``, yield pages (lists of
             :class:`~PowerPlatform.Dataverse.models.record.Record` objects)
             instead of individual records. Defaults to ``False``.
@@ -644,6 +689,8 @@ def execute(self, *, by_page: bool = False) -> Union[Iterable[Record], Iterable[
             :class:`~PowerPlatform.Dataverse.models.record.Record` objects
             (default) or pages of records (when ``by_page=True``).
         :rtype: Iterable[Record] or Iterable[List[Record]]
+        :raises ValueError: If no ``select``, ``filter``, or ``top``
+            constraint has been set.
         :raises RuntimeError: If the query was not created via
             ``client.query.builder()``.
 
@@ -668,6 +715,7 @@ def execute(self, *, by_page: bool = False) -> Union[Iterable[Record], Iterable[
                 "Cannot execute: query was not created via client.query.builder(). "
                 "Use build() and pass parameters to client.records.get() instead."
             )
+        self._validate_constraints()
         params = self.build()
         client = self._query_ops._client
 
@@ -703,9 +751,16 @@ def to_dataframe(self) -> pd.DataFrame:
         This method is only available when the QueryBuilder was created
         via ``client.query.builder(table)``.
 
+        At least one of ``select()``, ``filter_*()``, ``where()``, or
+        ``top()`` must be called before ``to_dataframe()``; otherwise a
+        :class:`ValueError` is raised to prevent accidental full-table
+        scans.
+
         :return: DataFrame containing all matching records. Returns an empty
             DataFrame when no records match.
         :rtype: ~pandas.DataFrame
+        :raises ValueError: If no ``select``, ``filter``, or ``top``
+            constraint has been set.
         :raises RuntimeError: If the query was not created via
             ``client.query.builder()``.
 
@@ -722,6 +777,7 @@ def to_dataframe(self) -> pd.DataFrame:
                 "Cannot execute: query was not created via client.query.builder(). "
                 "Use build() and pass parameters to client.dataframe.get() instead."
             )
+        self._validate_constraints()
         params = self.build()
         return self._query_ops._client.dataframe.get(
             params["table"],

tests/unit/models/test_query_builder.py

@@ -733,6 +733,7 @@ def test_execute_returns_flat_records_by_default(self):
 
         qb = QueryBuilder("account")
         qb._query_ops = mock_query_ops
+        qb.select("name")
         records = list(qb.execute())
 
         self.assertEqual(len(records), 3)
@@ -750,32 +751,75 @@ def test_execute_by_page_returns_pages(self):
 
         qb = QueryBuilder("account")
         qb._query_ops = mock_query_ops
+        qb.select("name")
         pages = list(qb.execute(by_page=True))
 
         self.assertEqual(len(pages), 2)
         self.assertEqual(pages[0], page1)
         self.assertEqual(pages[1], page2)
 
-    def test_execute_passes_none_for_empty_options(self):
+    def test_execute_unbounded_raises(self):
+        """execute() with no select/filter/top should raise ValueError."""
+        mock_query_ops = MagicMock()
+        qb = QueryBuilder("account")
+        qb._query_ops = mock_query_ops
+        with self.assertRaises(ValueError) as ctx:
+            qb.execute()
+        self.assertIn("Unbounded query", str(ctx.exception))
+
+    def test_execute_with_only_select_succeeds(self):
+        """execute() with select only should not raise."""
         mock_query_ops = MagicMock()
         mock_client = mock_query_ops._client
         mock_client.records.get.return_value = iter([])
 
         qb = QueryBuilder("account")
         qb._query_ops = mock_query_ops
-        list(qb.execute())
+        qb.select("name")
+        list(qb.execute())  # should not raise
+        mock_client.records.get.assert_called_once()
 
-        mock_client.records.get.assert_called_once_with(
-            "account",
-            select=None,
-            filter=None,
-            orderby=None,
-            top=None,
-            expand=None,
-            page_size=None,
-            count=False,
-            include_annotations=None,
-        )
+    def test_execute_with_only_filter_succeeds(self):
+        """execute() with filter only should not raise."""
+        mock_query_ops = MagicMock()
+        mock_client = mock_query_ops._client
+        mock_client.records.get.return_value = iter([])
+
+        qb = QueryBuilder("account")
+        qb._query_ops = mock_query_ops
+        qb.filter_eq("statecode", 0)
+        list(qb.execute())  # should not raise
+        mock_client.records.get.assert_called_once()
+
+    def test_execute_with_only_top_succeeds(self):
+        """execute() with top only should not raise."""
+        mock_query_ops = MagicMock()
+        mock_client = mock_query_ops._client
+        mock_client.records.get.return_value = iter([])
+
+        qb = QueryBuilder("account")
+        qb._query_ops = mock_query_ops
+        qb.top(10)
+        list(qb.execute())  # should not raise
+        mock_client.records.get.assert_called_once()
+
+    def test_execute_with_only_expand_raises(self):
+        """expand alone is not a sufficient constraint."""
+        mock_query_ops = MagicMock()
+        qb = QueryBuilder("account")
+        qb._query_ops = mock_query_ops
+        qb.expand("primarycontactid")
+        with self.assertRaises(ValueError):
+            qb.execute()
+
+    def test_execute_with_only_count_raises(self):
+        """count alone is not a sufficient constraint."""
+        mock_query_ops = MagicMock()
+        qb = QueryBuilder("account")
+        qb._query_ops = mock_query_ops
+        qb.count()
+        with self.assertRaises(ValueError):
+            qb.execute()
 
     def test_execute_with_where_expressions(self):
         from PowerPlatform.Dataverse.models.filters import eq, gt
@@ -819,12 +863,12 @@ def test_execute_passes_count_and_annotations(self):
 
         qb = QueryBuilder("account")
         qb._query_ops = mock_query_ops
-        qb.count().include_formatted_values()
+        qb.select("name").count().include_formatted_values()
         list(qb.execute())
 
         mock_client.records.get.assert_called_once_with(
             "account",
-            select=None,
+            select=["name"],
             filter=None,
             orderby=None,
             top=None,
@@ -874,29 +918,14 @@ def test_to_dataframe_delegates_to_dataframe_get(self):
         )
         pd.testing.assert_frame_equal(result, expected_df)
 
-    def test_to_dataframe_passes_none_for_empty_options(self):
-        """to_dataframe() with no options should pass None for all optional params."""
-        import pandas as pd
-
+    def test_to_dataframe_unbounded_raises(self):
+        """to_dataframe() with no select/filter/top should raise ValueError."""
         mock_query_ops = MagicMock()
-        mock_client = mock_query_ops._client
-        mock_client.dataframe.get.return_value = pd.DataFrame()
-
         qb = QueryBuilder("account")
         qb._query_ops = mock_query_ops
-        qb.to_dataframe()
-
-        mock_client.dataframe.get.assert_called_once_with(
-            "account",
-            select=None,
-            filter=None,
-            orderby=None,
-            top=None,
-            expand=None,
-            page_size=None,
-            count=False,
-            include_annotations=None,
-        )
+        with self.assertRaises(ValueError) as ctx:
+            qb.to_dataframe()
+        self.assertIn("Unbounded query", str(ctx.exception))
 
     def test_to_dataframe_returns_dataframe(self):
         """to_dataframe() should return a pandas DataFrame."""
@@ -928,12 +957,12 @@ def test_to_dataframe_forwards_count_and_annotations(self):
 
         qb = QueryBuilder("account")
         qb._query_ops = mock_query_ops
-        qb.count().include_formatted_values()
+        qb.select("name").count().include_formatted_values()
         qb.to_dataframe()
 
         mock_client.dataframe.get.assert_called_once_with(
             "account",
-            select=None,
+            select=["name"],
             filter=None,
             orderby=None,
             top=None,

tests/unit/test_query_operations.py

@@ -91,7 +91,7 @@ def test_builder_execute_flat_multiple_pages(self):
         """execute() should flatten records from multiple pages."""
         self.client._odata._get_multiple.return_value = iter([[{"accountid": "1"}], [{"accountid": "2"}]])
 
-        records = list(self.client.query.builder("account").execute())
+        records = list(self.client.query.builder("account").select("name").execute())
 
         self.assertEqual(len(records), 2)
         self.assertEqual(records[0]["accountid"], "1")
@@ -101,7 +101,7 @@ def test_builder_execute_by_page(self):
         """execute(by_page=True) should yield pages."""
         self.client._odata._get_multiple.return_value = iter([[{"accountid": "1"}], [{"accountid": "2"}]])
 
-        pages = list(self.client.query.builder("account").execute(by_page=True))
+        pages = list(self.client.query.builder("account").select("name").execute(by_page=True))
 
         self.assertEqual(len(pages), 2)
         self.assertEqual(len(pages[0]), 1)