Adjust guardrails: remove TOP injection (server enforces 5000 cap), add SELECT * + JOIN warning

Live-tested against Aurora VM (aurorabapenv71aff.crm10.dynamics.com):
- Server auto-caps at 5000 rows without TOP -> no client-side injection needed
- Server blocks SELECT * on both single-table and JOIN queries -> SDK expansion confirmed needed
- Added warning when SELECT * used with JOIN (expansion only includes first table columns)

732 tests passing.

Author: Saurabh Badenkal

src/PowerPlatform/Dataverse/data/_odata.py

@@ -838,14 +838,12 @@ def _do_request(url: str, *, params: Optional[Dict[str, Any]] = None) -> Dict[st
         r"^\s*(?:INSERT|UPDATE|DELETE|DROP|TRUNCATE|ALTER|CREATE|EXEC|GRANT|REVOKE|BULK)\b",
         re.IGNORECASE,
     )
-    _SQL_HAS_TOP_RE = re.compile(r"\bTOP\s+\d+", re.IGNORECASE)
-    _SQL_HAS_OFFSET_RE = re.compile(r"\bOFFSET\s+\d+\s+ROWS\b", re.IGNORECASE)
     _SQL_LEADING_WILDCARD_RE = re.compile(r"\bLIKE\s+'%[^']", re.IGNORECASE)
     _SQL_IMPLICIT_CROSS_JOIN_RE = re.compile(
         r"\bFROM\s+[A-Za-z0-9_]+\s+[A-Za-z0-9_]+\s*,\s*[A-Za-z0-9_]+",
         re.IGNORECASE,
     )
-    _SQL_DEFAULT_TOP = 5000
+    _SQL_HAS_JOIN_RE = re.compile(r"\bJOIN\b", re.IGNORECASE)
 
     def _expand_select_star(self, sql: str, table: str) -> str:
         """Replace ``SELECT *`` with explicit column names.
@@ -854,10 +852,26 @@ def _expand_select_star(self, sql: str, table: str) -> str:
         an error ("SELECT * is not supported").  This helper resolves all
         columns via ``_list_columns`` and rewrites the query so the user
         never has to know the server limitation.
+
+        For JOIN queries, the expansion only includes columns from the first
+        (FROM) table.  A warning is emitted so the user knows to specify
+        columns explicitly for multi-table queries.
         """
         if not self._SELECT_STAR_RE.search(sql):
             return sql
 
+        # Warn on SELECT * with JOINs -- expansion uses only the FROM table
+        if self._SQL_HAS_JOIN_RE.search(sql):
+            warnings.warn(
+                "SELECT * with JOIN: the SDK expands * using columns from "
+                "the first table only. Columns from joined tables will not "
+                "be included. Specify columns explicitly for JOINs "
+                "(e.g. SELECT a.name, c.fullname FROM account a "
+                "JOIN contact c ON ...).",
+                UserWarning,
+                stacklevel=4,
+            )
+
         cols = self._list_columns(
             table,
             select=["LogicalName"],
@@ -877,15 +891,17 @@ def _sql_guardrails(self, sql: str) -> str:
         1. **Block write statements** -- ``INSERT``, ``UPDATE``, ``DELETE``,
            ``DROP``, ``TRUNCATE``, ``ALTER``, ``CREATE``, ``EXEC``, ``GRANT``,
            ``REVOKE``, ``BULK`` are rejected with ``ValidationError``.
-        2. **Auto-inject TOP** -- if the query has no ``TOP`` or ``OFFSET``
-           clause, ``TOP 5000`` is injected after ``SELECT`` (or after
-           ``DISTINCT``) and a ``UserWarning`` is emitted so the caller
-           knows the result set was capped.
-        3. **Warn on leading-wildcard LIKE** -- ``LIKE '%...'`` patterns
+        2. **Warn on leading-wildcard LIKE** -- ``LIKE '%...'`` patterns
            force full table scans and hurt shared database performance.
-        4. **Warn on implicit cross joins** -- ``FROM a, b`` (comma syntax)
+        3. **Warn on implicit cross joins** -- ``FROM a, b`` (comma syntax)
            produces cartesian products.
 
+        .. note::
+           The server enforces a 5000-row maximum per query and blocks
+           ``SELECT *`` directly.  The SDK handles ``SELECT *`` via
+           ``_expand_select_star``.  No client-side TOP injection is needed
+           because the server already caps results.
+
         :param sql: The SQL string (already stripped).
         :return: Possibly-rewritten SQL string.
         :raises ValidationError: If the SQL contains a write statement.
@@ -899,31 +915,7 @@ def _sql_guardrails(self, sql: str) -> str:
                 subcode=VALIDATION_SQL_WRITE_BLOCKED,
             )
 
-        # 2. Auto-inject TOP if missing
-        if not self._SQL_HAS_TOP_RE.search(sql) and not self._SQL_HAS_OFFSET_RE.search(sql):
-            # Inject TOP 5000 after SELECT [DISTINCT]
-            def _inject_top(m):
-                return f"{m.group(0)}TOP {self._SQL_DEFAULT_TOP} "
-
-            sql_new = re.sub(
-                r"\bSELECT(\s+DISTINCT)?\s",
-                _inject_top,
-                sql,
-                count=1,
-                flags=re.IGNORECASE,
-            )
-            if sql_new != sql:
-                warnings.warn(
-                    f"Query has no TOP or OFFSET clause. "
-                    f"SDK auto-injected TOP {self._SQL_DEFAULT_TOP} to prevent "
-                    f"unbounded result sets. Add an explicit TOP or "
-                    f"OFFSET...FETCH to suppress this warning.",
-                    UserWarning,
-                    stacklevel=4,
-                )
-                sql = sql_new
-
-        # 3. Warn on leading-wildcard LIKE
+        # 2. Warn on leading-wildcard LIKE
         if self._SQL_LEADING_WILDCARD_RE.search(sql):
             warnings.warn(
                 "Query contains a leading-wildcard LIKE pattern "
@@ -934,7 +926,7 @@ def _inject_top(m):
                 stacklevel=4,
             )
 
-        # 4. Warn on implicit cross joins
+        # 3. Warn on implicit cross joins
         if self._SQL_IMPLICIT_CROSS_JOIN_RE.search(sql):
             warnings.warn(
                 "Query appears to use an implicit cross join "

tests/unit/data/test_sql_guardrails.py

@@ -81,78 +81,37 @@ def test_select_with_delete_in_where_not_blocked(self):
 
 
 # ===================================================================
-# 2. Auto-inject TOP when missing
+# 2. Server enforces TOP 5000 (no client-side injection needed)
 # ===================================================================
 
 
-class TestAutoInjectTop:
-    """Queries without TOP or OFFSET should get TOP 5000 injected."""
+class TestNoTopInjection:
+    """Verify the SDK does NOT inject TOP -- server handles the 5000 cap."""
 
-    def test_no_top_injects_top_5000(self):
+    def test_no_top_passes_through_unchanged(self):
         c = _client()
-        with warnings.catch_warnings(record=True) as w:
-            warnings.simplefilter("always")
-            result = c._sql_guardrails("SELECT name FROM account")
-            assert len(w) >= 1
-            assert "TOP 5000" in str(w[0].message)
-        assert "TOP 5000" in result
+        sql = "SELECT name FROM account"
+        result = c._sql_guardrails(sql)
+        assert result == sql
+        assert "TOP" not in result
 
     def test_existing_top_not_modified(self):
         c = _client()
-        with warnings.catch_warnings(record=True) as w:
-            warnings.simplefilter("always")
-            result = c._sql_guardrails("SELECT TOP 100 name FROM account")
-            top_warnings = [x for x in w if "TOP" in str(x.message)]
-            assert len(top_warnings) == 0
+        result = c._sql_guardrails("SELECT TOP 100 name FROM account")
         assert "TOP 100" in result
-        assert "TOP 5000" not in result
-
-    def test_existing_offset_not_modified(self):
-        c = _client()
-        with warnings.catch_warnings(record=True) as w:
-            warnings.simplefilter("always")
-            result = c._sql_guardrails(
-                "SELECT name FROM account ORDER BY name " "OFFSET 10 ROWS FETCH NEXT 5 ROWS ONLY"
-            )
-            top_warnings = [x for x in w if "TOP" in str(x.message)]
-            assert len(top_warnings) == 0
-        assert "TOP 5000" not in result
 
-    def test_distinct_gets_top_after_select_distinct(self):
+    def test_offset_passes_through(self):
         c = _client()
-        with warnings.catch_warnings(record=True):
-            warnings.simplefilter("always")
-            result = c._sql_guardrails("SELECT DISTINCT name FROM account")
-        assert "SELECT DISTINCT TOP 5000" in result or "SELECT DISTINCT TOP 5000" in result.upper()
+        sql = "SELECT name FROM account ORDER BY name OFFSET 10 ROWS FETCH NEXT 5 ROWS ONLY"
+        result = c._sql_guardrails(sql)
+        assert result == sql
 
-    def test_count_star_gets_top(self):
+    def test_join_without_top_not_modified(self):
         c = _client()
-        with warnings.catch_warnings(record=True) as w:
-            warnings.simplefilter("always")
-            result = c._sql_guardrails("SELECT COUNT(*) FROM account")
-            assert any("TOP 5000" in str(x.message) for x in w)
-        assert "TOP 5000" in result
-
-    def test_join_without_top_gets_top(self):
-        c = _client()
-        with warnings.catch_warnings(record=True) as w:
-            warnings.simplefilter("always")
-            result = c._sql_guardrails(
-                "SELECT a.name, c.fullname FROM account a " "JOIN contact c ON a.accountid = c.parentcustomerid"
-            )
-            assert any("TOP 5000" in str(x.message) for x in w)
-        assert "TOP 5000" in result
-
-    def test_join_with_top_not_modified(self):
-        c = _client()
-        with warnings.catch_warnings(record=True) as w:
-            warnings.simplefilter("always")
-            result = c._sql_guardrails(
-                "SELECT TOP 10 a.name FROM account a " "JOIN contact c ON a.accountid = c.parentcustomerid"
-            )
-            top_warnings = [x for x in w if "TOP" in str(x.message)]
-            assert len(top_warnings) == 0
-        assert "TOP 10" in result
+        sql = "SELECT a.name, c.fullname FROM account a " "JOIN contact c ON a.accountid = c.parentcustomerid"
+        result = c._sql_guardrails(sql)
+        assert result == sql
+        assert "TOP" not in result
 
 
 # ===================================================================
@@ -232,7 +191,52 @@ def test_single_table_no_warning(self):
 
 
 # ===================================================================
-# 5. Integration: _query_sql applies guardrails
+# 5. SELECT * with JOIN warning (from _expand_select_star)
+# ===================================================================
+
+
+class TestSelectStarJoinWarning:
+    """SELECT * with JOIN should warn that only first table columns are used."""
+
+    def test_select_star_with_join_warns(self):
+        c = _client()
+        c._list_columns = MagicMock(return_value=[{"LogicalName": "name"}, {"LogicalName": "accountid"}])
+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter("always")
+            c._expand_select_star(
+                "SELECT * FROM account a JOIN contact c ON a.accountid = c.parentcustomerid",
+                "account",
+            )
+            join_warnings = [x for x in w if "JOIN" in str(x.message)]
+            assert len(join_warnings) == 1
+            assert "first table only" in str(join_warnings[0].message)
+
+    def test_select_star_no_join_no_warning(self):
+        c = _client()
+        c._list_columns = MagicMock(return_value=[{"LogicalName": "name"}])
+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter("always")
+            c._expand_select_star("SELECT * FROM account", "account")
+            join_warnings = [x for x in w if "JOIN" in str(x.message)]
+            assert len(join_warnings) == 0
+
+    def test_no_star_with_join_no_warning(self):
+        c = _client()
+        c._list_columns = MagicMock()
+        with warnings.catch_warnings(record=True) as w:
+            warnings.simplefilter("always")
+            c._expand_select_star(
+                "SELECT a.name, c.fullname FROM account a " "JOIN contact c ON a.accountid = c.parentcustomerid",
+                "account",
+            )
+            # _list_columns not called (no star), so no JOIN warning
+            c._list_columns.assert_not_called()
+            join_warnings = [x for x in w if "JOIN" in str(x.message)]
+            assert len(join_warnings) == 0
+
+
+# ===================================================================
+# 6. Integration: _query_sql applies guardrails
 # ===================================================================
 
 
@@ -246,22 +250,23 @@ def test_write_blocked_before_server_call(self):
             c._query_sql("DELETE FROM account WHERE name = 'x'")
         c._request.assert_not_called()
 
-    def test_top_injected_in_server_request(self):
+    def test_no_top_injection_in_server_request(self):
+        """Server manages the 5000 cap -- SDK should not inject TOP."""
         c = _client()
         c._entity_set_from_schema_name = MagicMock(return_value="accounts")
         mock_response = MagicMock()
         mock_response.json.return_value = {"value": []}
         mock_response.status_code = 200
         c._request = MagicMock(return_value=mock_response)
 
-        with warnings.catch_warnings(record=True):
-            warnings.simplefilter("always")
-            c._query_sql("SELECT name FROM account")
+        c._query_sql("SELECT name FROM account")
 
         call_args = c._request.call_args
         sent_params = call_args[1].get("params", {})
         sent_sql = sent_params.get("sql", "")
-        assert "TOP 5000" in sent_sql
+        # SDK should NOT inject TOP 5000
+        assert "TOP 5000" not in sent_sql
+        assert sent_sql == "SELECT name FROM account"
 
     def test_explicit_top_preserved_in_server_request(self):
         c = _client()
@@ -277,4 +282,3 @@ def test_explicit_top_preserved_in_server_request(self):
         sent_params = call_args[1].get("params", {})
         sent_sql = sent_params.get("sql", "")
         assert "TOP 50" in sent_sql
-        assert "TOP 5000" not in sent_sql