Enhance FetchXmlQuery and QueryOperations with input validation and paging improvements

Author: Samson Gebre

src/PowerPlatform/Dataverse/models/fetchxml_query.py

@@ -5,10 +5,12 @@
 
 from __future__ import annotations
 
+import warnings
 import xml.etree.ElementTree as _ET
 from typing import Iterator, List, TYPE_CHECKING
-from urllib.parse import unquote as _url_unquote
+from urllib.parse import unquote as _url_unquote, quote as _url_quote
 
+from ..core.errors import ValidationError
 from .record import QueryResult, Record
 
 if TYPE_CHECKING:
@@ -21,6 +23,17 @@
     "odata.include-annotations=" '"Microsoft.Dynamics.CRM.fetchxmlpagingcookie,' 'Microsoft.Dynamics.CRM.morerecords"'
 )
 
+# Documented Dataverse GET request URL limit. See:
+# learn.microsoft.com/power-apps/developer/data-platform/webapi/compose-http-requests-handle-errors#maximum-url-length
+# FetchXML queries with many attributes or conditions are the most common way to reach it.
+# $batch POST doubles this to 64 KB.
+_MAX_URL_LENGTH = 32_768
+# Guards against infinite paging loops caused by a bug in cookie propagation or an
+# unexpected server response. At the default Dataverse page size of 5,000 rows this
+# cap allows up to 50 million records before raising; it is not a practical record
+# limit but a circuit-breaker against runaway iteration.
+_MAX_PAGES = 10_000
+
 
 class FetchXmlQuery:
     """Inert FetchXML query object. No HTTP request is made until
@@ -81,12 +94,27 @@ def execute_pages(self) -> Iterator[QueryResult]:
         """
         current_xml = self._xml
         page_num = 1
+        page_count = 0
 
         with self._client._scoped_odata() as od:
             entity_set = od._entity_set_from_schema_name(self._entity_name)
             base_url = f"{od.api}/{entity_set}"
 
             while True:
+                page_count += 1
+                if page_count > _MAX_PAGES:
+                    raise ValidationError(
+                        f"FetchXML paging exceeded {_MAX_PAGES} pages. "
+                        "This may indicate a runaway query or a bug in paging cookie propagation."
+                    )
+
+                encoded_len = len(base_url) + len("?fetchXml=") + len(_url_quote(current_xml, safe=""))
+                if encoded_len > _MAX_URL_LENGTH:
+                    raise ValidationError(
+                        f"FetchXML request URL exceeds {_MAX_URL_LENGTH} characters after encoding. "
+                        "Simplify the query or reduce attributes/conditions."
+                    )
+
                 r = od._request(
                     "get",
                     base_url,
@@ -103,29 +131,46 @@ def execute_pages(self) -> Iterator[QueryResult]:
 
                 yield QueryResult(page_records)
 
-                more = bool(data.get("@Microsoft.Dynamics.CRM.morerecords", False)) if isinstance(data, dict) else False
+                more_raw = data.get("@Microsoft.Dynamics.CRM.morerecords", False) if isinstance(data, dict) else False
+                more = more_raw is True or (isinstance(more_raw, str) and more_raw.lower() == "true")
                 if not more:
                     break
 
                 raw_cookie = (
                     data.get("@Microsoft.Dynamics.CRM.fetchxmlpagingcookie", "") if isinstance(data, dict) else ""
                 )
-                if not raw_cookie:
-                    break
-
-                # The annotation is outer XML: <cookie pagenumber="N" pagingcookie="DOUBLE_ENCODED" />
-                # The pagingcookie attribute is double URL-encoded; decode twice to get raw cookie XML.
-                try:
-                    cookie_el = _ET.fromstring(raw_cookie)
-                except _ET.ParseError:
-                    break
-                inner_encoded = cookie_el.get("pagingcookie", "")
-                if not inner_encoded:
-                    break
-                cookie = _url_unquote(_url_unquote(inner_encoded))
-                page_num = int(cookie_el.get("pagenumber", str(page_num + 1)))
 
+                if raw_cookie:
+                    try:
+                        cookie_el = _ET.fromstring(raw_cookie)
+                        inner_encoded = cookie_el.get("pagingcookie", "")
+                        if inner_encoded:
+                            cookie = _url_unquote(_url_unquote(inner_encoded))
+                            page_num = int(cookie_el.get("pagenumber", str(page_num + 1)))
+                            fetch_el = _ET.fromstring(current_xml)
+                            fetch_el.set("paging-cookie", cookie)
+                            fetch_el.set("page", str(page_num))
+                            current_xml = _ET.tostring(fetch_el, encoding="unicode")
+                            continue
+                    except (_ET.ParseError, ValueError):
+                        pass  # Fall through to simple paging
+
+                # Simple paging fallback: server returned morerecords=true but no paging
+                # cookie. Dataverse omits the cookie when the query cannot use cookie-based
+                # paging (e.g. FetchXML ordered by a link-entity column). We continue with
+                # page-number-only paging rather than truncating, but warn because simple
+                # paging has a 50,000-record server cap and performance degrades at high page
+                # numbers. The caller may be able to avoid this by reordering on the root
+                # entity instead.
+                warnings.warn(
+                    "Dataverse did not return a paging cookie; falling back to simple paging "
+                    "(page-number increment only). Simple paging is capped at 50,000 records "
+                    "and degrades in performance at high page numbers. Consider reordering on "
+                    "a root-entity column to enable cookie-based paging.",
+                    UserWarning,
+                    stacklevel=2,
+                )
+                page_num += 1
                 fetch_el = _ET.fromstring(current_xml)
-                fetch_el.set("paging-cookie", cookie)
                 fetch_el.set("page", str(page_num))
                 current_xml = _ET.tostring(fetch_el, encoding="unicode")

src/PowerPlatform/Dataverse/operations/query.py

@@ -8,9 +8,10 @@
 import warnings
 import xml.etree.ElementTree as _ET
 from typing import Any, Dict, List, Optional, TYPE_CHECKING
+from urllib.parse import quote as _url_quote
 
-from ..core.errors import MetadataError
-from ..models.fetchxml_query import FetchXmlQuery
+from ..core.errors import MetadataError, ValidationError
+from ..models.fetchxml_query import FetchXmlQuery, _MAX_URL_LENGTH
 from ..models.record import Record
 from ..models.query_builder import QueryBuilder
 
@@ -193,15 +194,35 @@ def fetchxml(self, xml: str) -> FetchXmlQuery:
             for page in query.execute_pages():
                 process(page.to_dataframe())
         """
-        root_el = _ET.fromstring(xml.strip())
+        if not isinstance(xml, str):
+            raise ValidationError("xml must be a string")
+        xml = xml.strip()
+        if not xml:
+            raise ValidationError("xml must not be empty")
+        # Fast-fail before any HTTP is attempted; execute_pages() re-checks the full URL
+        # (base + encoded XML) on each page.
+        if len(_url_quote(xml, safe="")) > _MAX_URL_LENGTH:
+            raise ValidationError(
+                f"FetchXML exceeds the Dataverse URL length limit ({_MAX_URL_LENGTH:,} characters) when encoded. "
+                "Use a $batch POST request to send FetchXML in the request body where the limit is 64 KB."
+            )
+        # Parse only to verify well-formedness and extract the entity name needed for the
+        # request URL. Structural and semantic validation is intentionally left to the server
+        # to avoid duplicating rules that may diverge from Dataverse's own enforcement.
+        # ElementTree does not resolve external entities or expand recursive internal entity
+        # references, so pathological inputs of that kind raise ParseError rather than
+        # consuming resources.
+        try:
+            root_el = _ET.fromstring(xml)
+        except _ET.ParseError as exc:
+            raise ValidationError(f"xml is not well-formed: {exc}") from exc
         entity_el = root_el.find("entity")
         if entity_el is None:
             raise ValueError("FetchXML must contain an <entity> child element")
         entity_name = entity_el.get("name", "")
         if not entity_name:
             raise ValueError("FetchXML <entity> element must have a 'name' attribute")
-
-        return FetchXmlQuery(xml.strip(), entity_name, self._client)
+        return FetchXmlQuery(xml, entity_name, self._client)
 
     # --------------------------------------------------------------- sql_columns
 

tests/unit/test_phase4_ga.py

@@ -87,7 +87,8 @@ def test_empty_result_returns_empty_query_result(self):
 
     def test_pagination_fetches_all_pages(self):
         """execute_pages() drives the HTTP loop; each page yields one QueryResult."""
-        cookie_raw = "%25253Cpagingcookie%252520pagingcookie%25253D%252522"
+        # Annotation is outer XML; pagingcookie attribute is double URL-encoded inner cookie.
+        cookie_raw = '<cookie pagenumber="2" pagingcookie="%253Cc%252F%253E" istracking="False" />'
         page1 = self._mock_response([{"name": "A"}], more=True, cookie=cookie_raw)
         page2 = self._mock_response([{"name": "B"}], more=False)
         self.client._odata._request.side_effect = [page1, page2]
@@ -98,7 +99,8 @@ def test_pagination_fetches_all_pages(self):
 
     def test_pagination_second_request_includes_page_and_cookie(self):
         """execute_pages() injects the decoded paging cookie into the second request."""
-        cookie_raw = "%25253Cpagingcookie%252520test%25253D%252522"
+        # pagingcookie="%253Cc%252F%253E": double URL-decode gives "<c/>" (the inner cookie XML).
+        cookie_raw = '<cookie pagenumber="2" pagingcookie="%253Cc%252F%253E" istracking="False" />'
         page1 = self._mock_response([{"name": "A"}], more=True, cookie=cookie_raw)
         page2 = self._mock_response([{"name": "B"}], more=False)
         self.client._odata._request.side_effect = [page1, page2]
@@ -168,7 +170,7 @@ def test_no_deprecation_warning_emitted(self):
 
     def test_execute_pages_returns_iterator_of_query_result(self):
         """execute_pages() yields QueryResult objects, one per HTTP page."""
-        cookie_raw = "%25253Cpagingcookie%252520pagingcookie%25253D%252522"
+        cookie_raw = '<cookie pagenumber="2" pagingcookie="%253Cc%252F%253E" istracking="False" />'
         page1 = self._mock_response([{"name": "A"}], more=True, cookie=cookie_raw)
         page2 = self._mock_response([{"name": "B"}], more=False)
         self.client._odata._request.side_effect = [page1, page2]
@@ -180,7 +182,7 @@ def test_execute_pages_returns_iterator_of_query_result(self):
 
     def test_execute_pages_one_http_call_per_page(self):
         """Each execute_pages() iteration fires exactly one HTTP request."""
-        cookie_raw = "%25253Cpagingcookie%252520pagingcookie%25253D%252522"
+        cookie_raw = '<cookie pagenumber="2" pagingcookie="%253Cc%252F%253E" istracking="False" />'
         page1 = self._mock_response([{"name": "A"}], more=True, cookie=cookie_raw)
         page2 = self._mock_response([{"name": "B"}], more=False)
         self.client._odata._request.side_effect = [page1, page2]
@@ -193,7 +195,7 @@ def test_execute_pages_one_http_call_per_page(self):
 
     def test_execute_pages_per_page_records(self):
         """Each page yielded by execute_pages() contains only its own records."""
-        cookie_raw = "%25253Cpagingcookie%252520pagingcookie%25253D%252522"
+        cookie_raw = '<cookie pagenumber="2" pagingcookie="%253Cc%252F%253E" istracking="False" />'
         page1 = self._mock_response([{"name": "A"}], more=True, cookie=cookie_raw)
         page2 = self._mock_response([{"name": "B"}, {"name": "C"}], more=False)
         self.client._odata._request.side_effect = [page1, page2]
@@ -204,6 +206,123 @@ def test_execute_pages_per_page_records(self):
         self.assertEqual(pages[0].first()["name"], "A")
         self.assertEqual(pages[1].first()["name"], "B")
 
+    # ------------------------------------------------------------------
+    # Input validation
+    # ------------------------------------------------------------------
+
+    def test_non_string_input_raises_validation_error(self):
+        from PowerPlatform.Dataverse.core.errors import ValidationError
+
+        with self.assertRaises(ValidationError):
+            self.client.query.fetchxml(123)
+
+    def test_empty_string_raises_validation_error(self):
+        from PowerPlatform.Dataverse.core.errors import ValidationError
+
+        with self.assertRaises(ValidationError):
+            self.client.query.fetchxml("")
+
+    def test_whitespace_only_raises_validation_error(self):
+        from PowerPlatform.Dataverse.core.errors import ValidationError
+
+        with self.assertRaises(ValidationError):
+            self.client.query.fetchxml("   ")
+
+    def test_malformed_xml_raises_validation_error(self):
+        from PowerPlatform.Dataverse.core.errors import ValidationError
+
+        with self.assertRaises(ValidationError):
+            self.client.query.fetchxml("<fetch><unclosed>")
+
+    def test_url_too_long_raises_validation_error(self):
+        """XML whose URL-encoded form exceeds 32,768 chars is rejected before any HTTP."""
+        from PowerPlatform.Dataverse.core.errors import ValidationError
+
+        # Alphanumeric chars are URL-safe and don't expand; a 32,769-char name attribute
+        # value pushes the encoded XML over the limit.
+        long_name = "a" * 32_769
+        big_xml = f'<fetch><entity name="{long_name}"><attribute name="x"/></entity></fetch>'
+        with self.assertRaises(ValidationError):
+            self.client.query.fetchxml(big_xml)
+
+    # ------------------------------------------------------------------
+    # Paging behaviour
+    # ------------------------------------------------------------------
+
+    def test_morerecords_string_true_continues_paging(self):
+        """morerecords annotation as string "true" (not bool) is handled correctly."""
+        cookie_raw = '<cookie pagenumber="2" pagingcookie="%253Cc%252F%253E" istracking="False" />'
+        page1_payload = {
+            "value": [{"name": "A"}],
+            "@Microsoft.Dynamics.CRM.morerecords": "true",
+            "@Microsoft.Dynamics.CRM.fetchxmlpagingcookie": cookie_raw,
+        }
+        page2_payload = {
+            "value": [{"name": "B"}],
+            "@Microsoft.Dynamics.CRM.morerecords": False,
+        }
+        r1, r2 = MagicMock(), MagicMock()
+        r1.json.return_value = page1_payload
+        r2.json.return_value = page2_payload
+        self.client._odata._request.side_effect = [r1, r2]
+
+        result = self.client.query.fetchxml(self._fetch_xml()).execute()
+        self.assertEqual(len(result), 2)
+        self.assertEqual(self.client._odata._request.call_count, 2)
+
+    def test_simple_paging_fallback_emits_user_warning(self):
+        """No cookie returned with morerecords=True triggers a UserWarning."""
+        page1 = self._mock_response([{"name": "A"}], more=True, cookie="")
+        page2 = self._mock_response([{"name": "B"}], more=False)
+        self.client._odata._request.side_effect = [page1, page2]
+
+        with warnings.catch_warnings(record=True) as caught:
+            warnings.simplefilter("always")
+            list(self.client.query.fetchxml(self._fetch_xml()).execute_pages())
+
+        user_warnings = [w for w in caught if issubclass(w.category, UserWarning)]
+        self.assertEqual(len(user_warnings), 1)
+        self.assertIn("simple paging", str(user_warnings[0].message).lower())
+
+    def test_simple_paging_fallback_fetches_all_pages(self):
+        """Simple paging fallback continues iterating; caller still gets all records."""
+        page1 = self._mock_response([{"name": "A"}], more=True, cookie="")
+        page2 = self._mock_response([{"name": "B"}], more=False)
+        self.client._odata._request.side_effect = [page1, page2]
+
+        with warnings.catch_warnings(record=True):
+            warnings.simplefilter("always")
+            result = self.client.query.fetchxml(self._fetch_xml()).execute()
+
+        self.assertEqual(len(result), 2)
+        self.assertEqual(self.client._odata._request.call_count, 2)
+
+    def test_malformed_cookie_falls_back_to_simple_paging(self):
+        """A cookie annotation that is not parseable as XML triggers simple paging fallback."""
+        page1 = self._mock_response([{"name": "A"}], more=True, cookie="not-valid-xml")
+        page2 = self._mock_response([{"name": "B"}], more=False)
+        self.client._odata._request.side_effect = [page1, page2]
+
+        with warnings.catch_warnings(record=True) as caught:
+            warnings.simplefilter("always")
+            result = self.client.query.fetchxml(self._fetch_xml()).execute()
+
+        self.assertEqual(len(result), 2)
+        user_warnings = [w for w in caught if issubclass(w.category, UserWarning)]
+        self.assertEqual(len(user_warnings), 1)
+
+    def test_max_pages_exceeded_raises(self):
+        """Paging loop raises ValidationError after exceeding _MAX_PAGES."""
+        from PowerPlatform.Dataverse.core.errors import ValidationError
+
+        cookie_raw = '<cookie pagenumber="2" pagingcookie="%253Cc%252F%253E" istracking="False" />'
+        always_more = self._mock_response([{"name": "A"}], more=True, cookie=cookie_raw)
+        self.client._odata._request.return_value = always_more
+
+        with patch("PowerPlatform.Dataverse.models.fetchxml_query._MAX_PAGES", 3):
+            with self.assertRaises(ValidationError):
+                list(self.client.query.fetchxml(self._fetch_xml()).execute_pages())
+
 
 # ---------------------------------------------------------------------------
 # Removed SQL helpers — raise AttributeError