Bug bash fixes: Add QueryResult.__aiter__ for async (ADO 6431066)#187
Open
abelmilash-msft wants to merge 1 commit into
Open
Bug bash fixes: Add QueryResult.__aiter__ for async (ADO 6431066)#187abelmilash-msft wants to merge 1 commit into
abelmilash-msft wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Addresses six bug-bash issues spanning query truncation, async iteration, auth token caching/sanitisation, and input validation in the records API.
Changes:
- Client-side
SELECT TOP Nenforcement in_query_sql(sync + async), backed by a new_extract_top_limithelper and_SQL_TOP_REregex on_ODataBase. - Auth refactor:
_AuthManager/_AsyncAuthManagercacheAccessTokenwith double-checked locking and proactive refresh, and wrap credential errors in a newAuthenticationErrorso token strings can't leak via raw exception text. records.create()rejects emptydata;retrieve/update/deletevalidate GUIDs via a shared_validate_guidhelper;QueryResult.__aiter__is now a real async generator.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/PowerPlatform/Dataverse/data/_odata_base.py | Adds _SQL_TOP_RE and _extract_top_limit shared helper. |
| src/PowerPlatform/Dataverse/data/_odata.py | Truncates _query_sql results to TOP and skips further pagination. |
| src/PowerPlatform/Dataverse/aio/data/_async_odata.py | Async counterpart of TOP truncation. |
| src/PowerPlatform/Dataverse/core/_auth.py | Adds AccessToken caching, lock-coalesced refresh, sanitized AuthenticationError wrapping. |
| src/PowerPlatform/Dataverse/aio/core/_async_auth.py | Async counterpart of caching/sanitisation. |
| src/PowerPlatform/Dataverse/core/errors.py | Adds AuthenticationError(DataverseError) and exports it. |
| src/PowerPlatform/Dataverse/models/record.py | Rewrites QueryResult.__aiter__ as an async generator. |
| src/PowerPlatform/Dataverse/operations/records.py | Adds _validate_guid, empty-data check in create, GUID checks in CRUD. |
| src/PowerPlatform/Dataverse/aio/operations/async_records.py | Async counterpart of GUID validation and empty-data check. |
| tests/unit/core/test_auth.py | New cache, near-expiry, and sanitisation tests for sync auth. |
| tests/unit/aio/core/test_async_auth.py | Async counterparts including a 20-task coalescing test. |
| tests/unit/data/test_sql_guardrails.py | TOP parsing and truncation enforcement tests. |
| tests/unit/test_records_operations.py | Empty-payload and bad-GUID rejection cases; updates fake IDs to valid GUIDs. |
| tests/unit/aio/test_async_records.py | Async records tests updated to use valid GUIDs. |
| tests/unit/aio/test_async_query.py | New async iteration tests for QueryResult. |
| tests/unit/test_phase3_ga.py | Updates fake IDs to a valid _GUID constant. |
| tests/unit/test_client_dataframe.py | Updates fake IDs to valid GUIDs. |
| tests/unit/test_dataframe_operations.py | Updates fake IDs to valid GUIDs. |
| tests/unit/aio/test_async_dataframe.py | Updates fake IDs to valid GUIDs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+95
to
+109
| if not self._need_new_token(): | ||
| return _TokenPair(resource=scope, access_token=self._token.token) | ||
| with self._lock: | ||
| # Double-check after acquiring lock — another thread may have refreshed. | ||
| if not self._need_new_token(): | ||
| return _TokenPair(resource=scope, access_token=self._token.token) | ||
| try: | ||
| self._token = self.credential.get_token(scope) | ||
| except Exception as exc: | ||
| raise AuthenticationError( | ||
| f"Token acquisition failed for scope '{scope}'. " | ||
| f"Check credential configuration. ({type(exc).__name__})", | ||
| ) from exc | ||
| self._refresh_jitter = random.randint(0, _MAX_REFRESH_JITTER_SECONDS) | ||
| return _TokenPair(resource=scope, access_token=self._token.token) |
Comment on lines
+34
to
+41
| try: | ||
| uuid.UUID(record_id) | ||
| except (ValueError, AttributeError, TypeError): | ||
| raise ValueError( | ||
| f"{param_name} {record_id!r} is not a valid GUID " | ||
| f"(expected 8-4-4-4-12 hex, e.g. 'a1b2c3d4-1234-5678-9abc-def012345678'). " | ||
| f"Pass a uuid.UUID(...) or a string returned by records.create()." | ||
| ) |
QueryResult only defined __iter__, so 'async for r in result' raised TypeError: 'QueryResult' object is not an async iterable. Python's async for does not fall back to __iter__. Added an async __aiter__ that yields the same Record objects, plus 4 unit tests covering presence, full iteration, empty result, and identity with sync iteration.
10488b5 to
fda8de7
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Addresses 6 bugs from the async/auth bug bash.
SELECT TOP Nlimit when server pagination returns more rowsQueryResult.__aiter__is now an actual async generatorAccessToken, refresh ~5 min before expiry with jitter, coalesce concurrent acquisitions under a lockget_tokenexceptions inAuthenticationErrorso bearer tokens / PII don't leak via raw provider stringsrecords.create()raisesValueErrorfor emptydatainstead of POSTing an empty body_validate_guidrejects non-UUIDrecord_id/idsbefore the HTTP call, with a clear errorChanges
Core fixes
data/_odata_base.py—_SQL_TOP_REregex +_extract_top_limit(sql) -> int | Nonehelper, shared by sync and async OData clients.data/_odata.py/aio/data/_async_odata.py—_query_sqlextracts the TOP limit once, then truncatesvalueat 4 points (bare-list response, after first page, mid-pagination, final return) and drops@odata.nextLinkwhen the cap is met. Stops paginating early.core/_auth.py/aio/core/_async_auth.py—_AuthManagernow holds theAccessTokenplus athreading.Lock/asyncio.Lock. Uses double-checked locking to coalesce concurrent token acquisitions. Refreshes atexpires_on − 300swith up to 60s random jitter. Wraps any credential exception inAuthenticationError(...) from excso the original is preserved in__cause__but the bearer token / PII never appears instr(exc).core/errors.py— NewAuthenticationError(DataverseError)exported in__all__.models/record.py—QueryResult.__aiter__rewritten asasync def ... yield recordsoasync for record in result:iterates over an actualAsyncIterator[Record]rather than awaiting a coroutine.operations/records.py/aio/operations/async_records.py— Module-level_validate_guid(record_id, param_name="record_id")helper called fromretrieve,update,delete(single + list forms). Catches(ValueError, AttributeError, TypeError)soNone,int, malformed strings, and missing attrs all raiseValueError("... is not a valid GUID")before any HTTP request.create()raises on emptydata.Tests
tests/unit/aio/core/test_async_auth.py— New: cache reuse, near-expiry refresh, expired-token refresh, 20-task concurrent coalescing, exception sanitization (sentinel token string never appears instr(exc);__cause__preserved).tests/unit/core/test_auth.py— Sync counterparts.tests/unit/data/test_sql_guardrails.py— TOP truncation across the 4 truncation points.tests/unit/test_records_operations.py—NoneandintGUID rejection cases.Tests:
2452 passedon the merged branch.Backwards compatibility
AuthenticationErroris added tocore.errors.__all__; it subclassesDataverseError, so existingexcept DataverseErrorhandlers keep working._validate_guidrejection raisesValueErrorfor inputs that previously produced an opaque HTTP 400/404 — strictly a clearer-error-earlier change, not a behavior reversal.