Skip to content

Commit 30a349b

Browse files
author
Abel Milash
committed
Fix 6 bug bash bugs: SQL TOP enforcement, QueryResult __aiter__, token caching/coalescing, auth exception sanitization, empty data validation, GUID validation
Bug 1 (ADO 6431970): Client-side enforcement of SELECT TOP n for ?sql= endpoint Bug 2 (ADO 6431066): Add __aiter__ to QueryResult for async for support Bug 3 (ADO 6431085): Token caching + lock-based coalescing in auth managers Bug 4 (ADO 6431086): Wrap credential exceptions in AuthenticationError to prevent token leaks Bug 5 (ADO 6431942): Reject empty data in records.create() Bug 6 (ADO 6431966): Validate GUIDs in retrieve/update/delete before server call All 2431 unit tests pass.
1 parent b940024 commit 30a349b

19 files changed

Lines changed: 804 additions & 201 deletions

src/PowerPlatform/Dataverse/aio/core/_async_auth.py

Lines changed: 41 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -12,9 +12,16 @@
1212

1313
from __future__ import annotations
1414

15+
import asyncio
16+
import random
17+
import time
18+
from typing import Optional
19+
20+
from azure.core.credentials import AccessToken
1521
from azure.core.credentials_async import AsyncTokenCredential
1622

17-
from ...core._auth import _TokenPair
23+
from ...core._auth import _TokenPair, _REFRESH_WINDOW_SECONDS, _MAX_REFRESH_JITTER_SECONDS
24+
from ...core.errors import AuthenticationError
1825

1926

2027
class _AsyncAuthManager:
@@ -30,16 +37,47 @@ def __init__(self, credential: AsyncTokenCredential) -> None:
3037
if not isinstance(credential, AsyncTokenCredential):
3138
raise TypeError("credential must implement azure.core.credentials_async.AsyncTokenCredential.")
3239
self.credential: AsyncTokenCredential = credential
40+
self._token: Optional[AccessToken] = None
41+
self._lock = asyncio.Lock()
42+
self._refresh_jitter: int = 0
43+
44+
def _need_new_token(self) -> bool:
45+
"""Return ``True`` when the cached token is missing or near expiry."""
46+
if self._token is None:
47+
return True
48+
remaining = self._token.expires_on - time.time()
49+
if remaining <= 0:
50+
return True
51+
return remaining < (_REFRESH_WINDOW_SECONDS - self._refresh_jitter)
3352

3453
async def _acquire_token(self, scope: str) -> _TokenPair:
3554
"""
3655
Acquire an access token asynchronously for the specified OAuth2 scope.
3756
57+
Tokens are cached and reused until they are within
58+
:data:`~PowerPlatform.Dataverse.core._auth._REFRESH_WINDOW_SECONDS` of
59+
expiry. An :class:`asyncio.Lock` serialises concurrent callers so that
60+
only one coroutine calls the underlying credential at a time
61+
(double-check locking pattern).
62+
3863
:param scope: OAuth2 scope string, typically ``"https://<org>.crm.dynamics.com/.default"``.
3964
:type scope: :class:`str`
4065
:return: Token pair containing the scope and access token.
4166
:rtype: ~PowerPlatform.Dataverse.core._auth._TokenPair
4267
:raises ~azure.core.exceptions.ClientAuthenticationError: If token acquisition fails.
4368
"""
44-
token = await self.credential.get_token(scope)
45-
return _TokenPair(resource=scope, access_token=token.token)
69+
if not self._need_new_token():
70+
return _TokenPair(resource=scope, access_token=self._token.token)
71+
async with self._lock:
72+
# Double-check after acquiring lock — another coroutine may have refreshed.
73+
if not self._need_new_token():
74+
return _TokenPair(resource=scope, access_token=self._token.token)
75+
try:
76+
self._token = await self.credential.get_token(scope)
77+
except Exception as exc:
78+
raise AuthenticationError(
79+
f"Token acquisition failed for scope '{scope}'. "
80+
f"Check credential configuration. ({type(exc).__name__})",
81+
) from exc
82+
self._refresh_jitter = random.randint(0, _MAX_REFRESH_JITTER_SECONDS)
83+
return _TokenPair(resource=scope, access_token=self._token.token)

src/PowerPlatform/Dataverse/aio/data/_async_odata.py

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -677,6 +677,11 @@ async def _query_sql(self, sql: str) -> list[dict[str, Any]]:
677677
# any table resolution.
678678
sql = self._sql_guardrails(sql)
679679

680+
# Extract TOP N limit for client-side enforcement. The Dataverse
681+
# ?sql= endpoint ignores TOP in the SQL string, so the SDK must
682+
# truncate the result set to honour the caller's intent.
683+
top_limit = self._extract_top_limit(sql)
684+
680685
r = await self._execute_raw(await self._build_sql(sql))
681686
try:
682687
body = r.json()
@@ -686,14 +691,19 @@ async def _query_sql(self, sql: str) -> list[dict[str, Any]]:
686691
# Collect first page
687692
results: list[dict[str, Any]] = []
688693
if isinstance(body, list):
689-
return [row for row in body if isinstance(row, dict)]
694+
rows = [row for row in body if isinstance(row, dict)]
695+
return rows[:top_limit] if top_limit else rows
690696
if not isinstance(body, dict):
691697
return results
692698

693699
value = body.get("value")
694700
if isinstance(value, list):
695701
results = [row for row in value if isinstance(row, dict)]
696702

703+
# Skip pagination entirely when first page already satisfies TOP.
704+
if top_limit and len(results) >= top_limit:
705+
return results[:top_limit]
706+
697707
# Follow pagination links until exhausted
698708
raw_link = body.get("@odata.nextLink") or body.get("odata.nextLink")
699709
next_link: str | None = raw_link if isinstance(raw_link, str) else None
@@ -758,10 +768,13 @@ async def _query_sql(self, sql: str) -> list[dict[str, Any]]:
758768
if not isinstance(page_value, list) or not page_value:
759769
break
760770
results.extend(row for row in page_value if isinstance(row, dict))
771+
# Stop pagination early when TOP limit is reached.
772+
if top_limit and len(results) >= top_limit:
773+
break
761774
raw_link = page_body.get("@odata.nextLink") or page_body.get("odata.nextLink")
762775
next_link = raw_link if isinstance(raw_link, str) else None
763776

764-
return results
777+
return results[:top_limit] if top_limit else results
765778

766779
# ---------------------- Entity set resolution -----------------------
767780
async def _entity_set_from_schema_name(self, table_schema_name: str) -> str:

src/PowerPlatform/Dataverse/aio/operations/async_records.py

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
from ...core.errors import HttpError
1111
from ...models.record import QueryResult, Record
1212
from ...models.upsert import UpsertItem
13+
from ...operations.records import _validate_guid
1314

1415
if TYPE_CHECKING:
1516
from ...models.filters import FilterExpression
@@ -93,6 +94,10 @@ async def create(
9394
])
9495
print(f"Created {len(guids)} accounts")
9596
"""
97+
if not data:
98+
raise ValueError(
99+
f"data is empty -- provide at least one record with column values to create in '{table}'."
100+
)
96101
async with self._client._scoped_odata() as od:
97102
entity_set = await od._entity_set_from_schema_name(table)
98103
if isinstance(data, dict):
@@ -158,10 +163,13 @@ async def update(
158163
if isinstance(ids, str):
159164
if not isinstance(changes, dict):
160165
raise TypeError("For single id, changes must be a dict")
166+
_validate_guid(ids, "ids")
161167
await od._update(table, ids, changes)
162168
return None
163169
if not isinstance(ids, list):
164170
raise TypeError("ids must be str or list[str]")
171+
for rid in ids:
172+
_validate_guid(rid, "ids")
165173
await od._update_by_ids(table, ids, changes)
166174
return None
167175

@@ -211,6 +219,7 @@ async def delete(
211219
"""
212220
async with self._client._scoped_odata() as od:
213221
if isinstance(ids, str):
222+
_validate_guid(ids, "ids")
214223
await od._delete(table, ids)
215224
return None
216225
if not isinstance(ids, list):
@@ -219,6 +228,8 @@ async def delete(
219228
return None
220229
if not all(isinstance(rid, str) for rid in ids):
221230
raise TypeError("ids must contain string GUIDs")
231+
for rid in ids:
232+
_validate_guid(rid, "ids")
222233
if use_bulk_delete:
223234
return await od._delete_multiple(table, ids)
224235
for rid in ids:
@@ -269,6 +280,7 @@ async def retrieve(
269280
contact = record.get("primarycontactid") or {}
270281
print(contact.get("fullname"))
271282
"""
283+
_validate_guid(record_id)
272284
async with self._client._scoped_odata() as od:
273285
try:
274286
raw = await od._get(

src/PowerPlatform/Dataverse/core/_auth.py

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -11,9 +11,18 @@
1111

1212
from __future__ import annotations
1313

14+
import random
15+
import threading
16+
import time
1417
from dataclasses import dataclass
18+
from typing import Optional
1519

16-
from azure.core.credentials import TokenCredential
20+
from azure.core.credentials import AccessToken, TokenCredential
21+
22+
from .errors import AuthenticationError
23+
24+
_REFRESH_WINDOW_SECONDS = 300 # Proactive refresh 5 min before expiry
25+
_MAX_REFRESH_JITTER_SECONDS = 60 # Random jitter to spread refresh load
1726

1827

1928
@dataclass(repr=False)
@@ -55,16 +64,46 @@ def __init__(self, credential: TokenCredential) -> None:
5564
if not isinstance(credential, TokenCredential):
5665
raise TypeError("credential must implement azure.core.credentials.TokenCredential.")
5766
self.credential: TokenCredential = credential
67+
self._token: Optional[AccessToken] = None
68+
self._lock = threading.Lock()
69+
self._refresh_jitter: int = 0
70+
71+
def _need_new_token(self) -> bool:
72+
"""Return ``True`` when the cached token is missing or near expiry."""
73+
if self._token is None:
74+
return True
75+
remaining = self._token.expires_on - time.time()
76+
if remaining <= 0:
77+
return True
78+
return remaining < (_REFRESH_WINDOW_SECONDS - self._refresh_jitter)
5879

5980
def _acquire_token(self, scope: str) -> _TokenPair:
6081
"""
6182
Acquire an access token for the specified OAuth2 scope.
6283
84+
Tokens are cached and reused until they are within
85+
:data:`_REFRESH_WINDOW_SECONDS` of expiry. A :class:`threading.Lock`
86+
serialises concurrent callers so that only one thread calls the
87+
underlying credential at a time (double-check locking pattern).
88+
6389
:param scope: OAuth2 scope string, typically ``"https://<org>.crm.dynamics.com/.default"``.
6490
:type scope: :class:`str`
6591
:return: Token pair containing the scope and access token.
6692
:rtype: ~PowerPlatform.Dataverse.core._auth._TokenPair
6793
:raises ~azure.core.exceptions.ClientAuthenticationError: If token acquisition fails.
6894
"""
69-
token = self.credential.get_token(scope)
70-
return _TokenPair(resource=scope, access_token=token.token)
95+
if not self._need_new_token():
96+
return _TokenPair(resource=scope, access_token=self._token.token)
97+
with self._lock:
98+
# Double-check after acquiring lock — another thread may have refreshed.
99+
if not self._need_new_token():
100+
return _TokenPair(resource=scope, access_token=self._token.token)
101+
try:
102+
self._token = self.credential.get_token(scope)
103+
except Exception as exc:
104+
raise AuthenticationError(
105+
f"Token acquisition failed for scope '{scope}'. "
106+
f"Check credential configuration. ({type(exc).__name__})",
107+
) from exc
108+
self._refresh_jitter = random.randint(0, _MAX_REFRESH_JITTER_SECONDS)
109+
return _TokenPair(resource=scope, access_token=self._token.token)

src/PowerPlatform/Dataverse/core/errors.py

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,6 +127,26 @@ def __init__(self, message: str, *, subcode: Optional[str] = None, details: Opti
127127
super().__init__(message, code="sql_parse_error", subcode=subcode, details=details, source="client")
128128

129129

130+
class AuthenticationError(DataverseError):
131+
"""
132+
Exception raised when token acquisition fails.
133+
134+
The message is sanitised to prevent credential exceptions from leaking
135+
token strings into logs or tracebacks. The original exception is
136+
preserved as ``__cause__`` for advanced debugging.
137+
138+
:param message: Sanitised human-readable message.
139+
:type message: :class:`str`
140+
:param subcode: Optional error identifier.
141+
:type subcode: :class:`str` | None
142+
:param details: Optional additional context.
143+
:type details: :class:`dict` | None
144+
"""
145+
146+
def __init__(self, message: str, *, subcode: Optional[str] = None, details: Optional[Dict[str, Any]] = None):
147+
super().__init__(message, code="authentication_error", subcode=subcode, details=details, source="client")
148+
149+
130150
class HttpError(DataverseError):
131151
"""
132152
Exception raised for HTTP request failures from the Dataverse Web API.
@@ -198,4 +218,4 @@ def __init__(
198218
)
199219

200220

201-
__all__ = ["DataverseError", "HttpError", "ValidationError", "MetadataError", "SQLParseError"]
221+
__all__ = ["AuthenticationError", "DataverseError", "HttpError", "ValidationError", "MetadataError", "SQLParseError"]

src/PowerPlatform/Dataverse/data/_odata.py

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -648,6 +648,11 @@ def _query_sql(self, sql: str) -> list[dict[str, Any]]:
648648
# any table resolution.
649649
sql = self._sql_guardrails(sql)
650650

651+
# Extract TOP N limit for client-side enforcement. The Dataverse
652+
# ?sql= endpoint ignores TOP in the SQL string, so the SDK must
653+
# truncate the result set to honour the caller's intent.
654+
top_limit = self._extract_top_limit(sql)
655+
651656
r = self._execute_raw(self._build_sql(sql))
652657
try:
653658
body = r.json()
@@ -657,14 +662,19 @@ def _query_sql(self, sql: str) -> list[dict[str, Any]]:
657662
# Collect first page
658663
results: list[dict[str, Any]] = []
659664
if isinstance(body, list):
660-
return [row for row in body if isinstance(row, dict)]
665+
rows = [row for row in body if isinstance(row, dict)]
666+
return rows[:top_limit] if top_limit else rows
661667
if not isinstance(body, dict):
662668
return results
663669

664670
value = body.get("value")
665671
if isinstance(value, list):
666672
results = [row for row in value if isinstance(row, dict)]
667673

674+
# Skip pagination entirely when first page already satisfies TOP.
675+
if top_limit and len(results) >= top_limit:
676+
return results[:top_limit]
677+
668678
# Follow pagination links until exhausted
669679
raw_link = body.get("@odata.nextLink") or body.get("odata.nextLink")
670680
next_link: str | None = raw_link if isinstance(raw_link, str) else None
@@ -729,10 +739,13 @@ def _query_sql(self, sql: str) -> list[dict[str, Any]]:
729739
if not isinstance(page_value, list) or not page_value:
730740
break
731741
results.extend(row for row in page_value if isinstance(row, dict))
742+
# Stop pagination early when TOP limit is reached.
743+
if top_limit and len(results) >= top_limit:
744+
break
732745
raw_link = page_body.get("@odata.nextLink") or page_body.get("odata.nextLink")
733746
next_link = raw_link if isinstance(raw_link, str) else None
734747

735-
return results
748+
return results[:top_limit] if top_limit else results
736749

737750
# ---------------------- Entity set resolution -----------------------
738751
def _entity_set_from_schema_name(self, table_schema_name: str) -> str:

src/PowerPlatform/Dataverse/data/_odata_base.py

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -801,6 +801,24 @@ def _build_get_relationship(self, schema_name: str) -> _RawRequest:
801801
r"\bSELECT\b\s+(?:DISTINCT\s+)?(?:TOP\s+\d+(?:\s+PERCENT)?\s+)?\*\s",
802802
re.IGNORECASE,
803803
)
804+
# Extract TOP N (integer row count) from SELECT. Does NOT match
805+
# TOP N PERCENT -- percentage limits cannot be enforced client-side
806+
# because the total row count is unknown before the query completes.
807+
_SQL_TOP_RE = re.compile(
808+
r"\bSELECT\b\s+(?:DISTINCT\s+)?TOP\s+(\d+)\b(?!\s+PERCENT)",
809+
re.IGNORECASE,
810+
)
811+
812+
@staticmethod
813+
def _extract_top_limit(sql: str) -> int | None:
814+
"""Return the integer TOP limit from a SQL SELECT, or ``None``.
815+
816+
Matches ``SELECT TOP n`` and ``SELECT DISTINCT TOP n`` but
817+
**not** ``TOP n PERCENT`` (percentage limits cannot be enforced
818+
client-side).
819+
"""
820+
m = _ODataBase._SQL_TOP_RE.search(sql)
821+
return int(m.group(1)) if m else None
804822

805823
def _sql_guardrails(self, sql: str) -> str:
806824
"""Apply safety guardrails to a SQL query before sending to the server.

src/PowerPlatform/Dataverse/models/record.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
from __future__ import annotations
77

88
from dataclasses import dataclass, field
9-
from typing import Any, Dict, Iterator, KeysView, List, Optional, Union, ValuesView, ItemsView
9+
from typing import Any, AsyncIterator, Dict, Iterator, KeysView, List, Optional, Union, ValuesView, ItemsView
1010

1111
__all__ = ["Record", "QueryResult"]
1212

@@ -132,6 +132,10 @@ def __init__(self, records: List[Record]) -> None:
132132
def __iter__(self) -> Iterator[Record]:
133133
return iter(self.records)
134134

135+
async def __aiter__(self) -> AsyncIterator[Record]:
136+
for record in self.records:
137+
yield record
138+
135139
def __len__(self) -> int:
136140
return len(self.records)
137141

0 commit comments

Comments
 (0)