Repository navigation
Align paired sync/aio clients with Azure SDK contracts - #103
hallvictoria wants to merge 19 commits into
Conversation
86d43e0 to
21c4d5c
Compare
David Burg (daviburg)
left a comment
There was a problem hiding this comment.
[Dobby] Connector SDK review: two medium-priority runtime defects reproduced on the current head; see the separate inline comments. The earlier constructor-timeout forwarding finding no longer reproduces after the latest SDK update, so it is not included as an open finding.
David Burg (daviburg)
left a comment
There was a problem hiding this comment.
[Dobby] Comprehensive paired Connector SDK review
Assessment: changes are needed before merge. This consolidates independent GPT-6.1 Sol, Gemini 4 Argon, and Grok 4.7 reviews, followed by runtime reproductions and generator/artifact checks. The findings are based on behavior and source evidence, not model agreement alone.
Reviewed revisions: Python SDK commit 0f4c68b and BPM generator commit b9c9fd2, iteration 23 of paired BPM PR 17490020.
Six findings, posted individually on the owning repository
| # | Priority | Owner | Finding |
|---|---|---|---|
| 1 | P1 | This SDK | Default synchronous response buffering raises after consuming a normal fixed-length successful response. See inline comment. |
| 2 | P2 | BPM generator | Jira issue paging discards opaque nextPageToken and silently returns only page one. Individual generator finding. |
| 3 | P2 | BPM generator | Newly inferred .NET Jira pagers bind to a synthetic nextLink, not the actual nextPage. Individual generator finding. |
| 4 | P2 | This SDK | Migrated synchronous examples still call dictionary .get(...) on ItemPaged. See inline comment. |
| 5 | P2 | This SDK | Synchronous response-header loading can exceed the documented total timeout. See inline comment. |
| 6 | P2 | This SDK | The initial path-relative LRO Location lacks the initiating request URI as its resolution base. See inline comment. |
The four SDK findings have separate inline comments in this review. Generator findings are anchored on BPM rather than duplicated on generated client files.
Validation actually performed
- Rebuilt the latest BPM compiler and ran its exact check-in test filter: 857 passed.
- Full local Python suite: 4,964 passed, 1 failed. The slow-drip timeout regression leaked raw
urllib3.exceptions.ReadTimeoutError; an isolated six-run repeat failed once and passed five times. This is not a clean full-suite result. - Focused sync-runtime/polling slice: 32 passed. Declared Azure Core 1.30.0 runtime/polling/LRO slice: 34 passed.
- Flake8, mypy across 214 source files, strict MkDocs/API-reference validation, and wheel packaging passed. All 198 static sample-compatibility checks passed, but executing the two actual Jira examples exposed finding 4.
- Independently captured live
westusSwagger into a new empty cache and replayed generation with outbound ARM access blocked. All 198 connector modules matched this SDK head byte-for-byte with BPMb9c9fd2; all 100 captured response hashes remained unchanged. A missing-cache negative control failed to connect through the blocked proxy. This proves current reproducibility, not the author's historical input snapshot. - Regenerated matching .NET output from the same Swagger with both the PR merge-base and current generator. The Jira continuation regression in finding 3 is introduced. Inherited dangling discovery/type-collapse detector results were not reported as new defects.
Generator integration and provenance gates
- Current reproducibility: passed against
b9c9fd2; reviewed-source coverage: passed for the paired generator PR. - Upstream integration: pending because the BPM PR remains active and unmerged.
- The PR descriptions need correction: this SDK description declares generator
b7a57c9, whose output differs in all 198 modules, and both summaries still claim committed Swagger snapshots/manifest artifacts that were removed. The current catalog already matchesb9c9fd2; this is stale provenance prose, not evidence of hand-edited connector files.
Earlier feedback
Constructor timeout forwarding is fixed. Subsequent sibling-relative and parent-relative polling Location updates are also fixed. Finding 6 concerns the distinct initial Location base. The synchronous timeout fix remains incomplete and introduced the normal-response failure described in finding 1.
No approval, merge, or code change is implied by this COMMENT review. The six findings remain open for the author to address.
Description
Align all 99 generated connectors with Azure SDK contracts while retaining paired synchronous root and asynchronous
azure.connectors.aioclients and shared root-owned models.Paired Generator PR
46ee9dba871b8bb29a2c8b179042d1128d6d1656.1c6a985f0d52b5500d4efecdf3f503db5ce46077.Changes
ItemPaged/AsyncItemPaged, and retainedbegin_*pollers.nextPageTokencontinuation, including resumption, empty pages, token exhaustion, andisLasttermination.ServiceResponseTimeoutError.Generation provenance
All 99 connector pairs were regenerated with the revision above from a fresh live BTS4 (
f34b22a3-2202-4fb1-b040-1332bd928c84)westuscapture. The 99 Swagger inputs are retained outside the product repository for this validation session. A second offline generation from those inputs matched all 198 connector modules byte-for-byte (SHA-256 comparisons: zero mismatches).This PR does not contain committed Swagger snapshots or a generation manifest. Earlier descriptions claiming those artifacts, and older generator revision references, were stale and are corrected here.
Validation before pushing
git diff --check.Hosted CI is checked separately on the pushed revision; the local evidence above does not claim every hosted OS/Python matrix job has already executed.
Checklist