Skip to content

Add async API support with httpx migration#64

Open
dalemyers wants to merge 1 commit intomainfrom
async-support
Open

Add async API support with httpx migration#64
dalemyers wants to merge 1 commit intomainfrom
async-support

Conversation

@dalemyers
Copy link
Copy Markdown
Collaborator

@dalemyers dalemyers commented Apr 9, 2026

Summary

  • Adds a full async API under simple_ado._async (also accessible as simple_ado.aio) using httpx
  • Async code is the source of truth; sync code is auto-generated via scripts/generate_sync.py
  • Migrates HTTP layer from requests to httpx (supports both sync and async)
  • Preserves backward compatibility: types.py shim, .ok compat wrapper on exceptions, sync __getitem__ auto-refresh, silent GC cleanup via __del__
  • Test infrastructure mirrors code generation: async tests in tests/unit/_async/ are source of truth, sync tests are generated

Add full async API support to simple_ado in a backwards-compatible way.
The async code in simple_ado/_async/ is the single source of truth;
synchronous code is auto-generated from it by scripts/generate_sync.py.
Existing sync imports and usage are unchanged.

Architecture:
- simple_ado/_async/ contains the hand-written async source (24 files)
- scripts/generate_sync.py transforms async → sync via text replacement
  (strip async/await, ADOAsync* → ADO*, AsyncIterator → Iterator, etc.)
  and formats output with black in-memory for idempotent generation
- Generated sync files overwrite the top-level simple_ado/ modules and
  carry a "DO NOT EDIT" header
- Shared modules (models, comments, exceptions, ado_types) live at the
  top level and are imported by both async and sync code

HTTP layer changes:
- Replace requests with httpx (sync httpx.Client / async httpx.AsyncClient)
- Add stream_get() / stream_post() async context managers for streaming
- get(stream=True) / post(stream=True) still work (with deprecation
  warning) for backwards compatibility
- Narrow retryable status codes to {400,408,429,500,502,503,504} instead
  of the full 4xx range — deterministic failures like 401/403/404 no
  longer retry
- Add 300s default timeout via httpx.Timeout
- ADOHTTPClient gains close(), __enter__/__exit__, and __del__ for
  proper httpx.Client lifecycle management

Auth changes:
- Add ADOAsyncAuth base class with async get_authorization_header()
- Add ADOAsyncTokenAuth, ADOAsyncBasicAuth, ADOAsyncAzIDAuth using
  azure.identity.aio.DefaultAzureCredential
- Auth classes gain close() for resource cleanup

ADOWorkItem changes:
- Sync __getitem__ retains auto-refresh on missing fields
- Async __getitem__ raises KeyError immediately (can't be async);
  use await work_item.get_field(key) for auto-refresh behavior
- Add set() convenience method for both sync and async

Test infrastructure:
- Replace responses with respx for httpx mocking
- Add pytest-asyncio for async test support
- Async tests live in tests/unit/_async/ and are auto-generated into
  tests/unit/ by the same transform script
- Add tests for HTTP client rate limiting, retryable status codes,
  and work item field access patterns

Usage:
  # Existing sync — unchanged
  from simple_ado import ADOClient, ADOTokenAuth
  client = ADOClient(tenant="org", auth=ADOTokenAuth("token"))

  # New async
  from simple_ado._async import ADOAsyncClient, ADOAsyncTokenAuth
  async with ADOAsyncClient(tenant="org", auth=ADOAsyncTokenAuth("token")) as client:
      await client.verify_access()

Breaking changes:
- ADOHTTPException.response is now httpx.Response (was requests.Response)
  wrapped in a _CompatResponse that adds .ok for backwards compatibility
- requests is no longer a dependency; httpx is required instead

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@Cokile Cokile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall Assessment

This is a large, well-structured PR that migrates the HTTP layer from requests to httpx and introduces a full async API via simple_ado._async (aliased as simple_ado.aio). The "async-first, sync-generated" approach using scripts/generate_sync.py is a reasonable pattern for maintaining parity. However, there are several issues worth addressing.


Critical Issues

1. Breaking change in _is_retryable_get_failure — retryable range expanded silently

The existing sync code retries on status_code in range(400, 500) (all 4xx errors). The new code explicitly lists {400, 408, 429, 500, 502, 503, 504}. This is actually a fix (retrying 401/403/404 was wrong), but the sync generated code should also reflect this. Verify that generate_sync.py properly translates the new async retry logic — the current sync http_client.py still has the old range(400, 500) check.

2. response.ok vs response.is_success semantic mismatch

_CompatResponse.ok returns status_code < 400, but httpx.Response.is_success checks 200 <= status_code < 300. The governance code switches from .ok to .is_success, which narrows the success range — 3xx responses that previously passed will now raise. Double-check that no callers relied on 3xx being treated as "ok."

3. Missing close() / resource cleanup on sync ADOHTTPClient

The sync ADOClient gains close(), __enter__, and __exit__. But the underlying sync ADOHTTPClient must actually close the httpx.Client session. Since the diff for http_client.py was too large to render, confirm that ADOHTTPClient.close() calls self._session.close() on the httpx.Client.


Medium Issues

4. types.py deprecation shim uses wildcard import

from simple_ado.ado_types import * # noqa: F401,F403
from simple_ado.ado_types import TeamFoundationId as TeamFoundationId # noqa: F811

The second line is redundant since * already imports TeamFoundationId. If the intent is to make it explicitly re-exported for type checkers, the as TeamFoundationId alias is the right approach, but the # noqa: F811 suppression suggests awareness of the duplication. Consider just using __all__ re-export instead of wildcard.

5. generate_sync.py is 845 lines — needs test coverage

This is a code-generation script that produces the entire sync API surface. A bug here silently corrupts all sync code. Consider adding at least a round-trip test: generate sync, import module, verify key classes exist and have correct method signatures vs the async source.

6. Module-level import at end of __init__.py

from simple_ado import _async as aio  # noqa: F401
__all__ += ["aio"]

This import at module bottom is fragile — it runs after all class definitions, but if _async/__init__.py has any import-time side effects that reference sync classes, circular imports could arise. Consider using a lazy import pattern or documenting this constraint.

7. ADOAsyncBasicAuth stores credentials in memory

Both sync and async BasicAuth now cache the header string in _cached_header. The old code used @functools.lru_cache(maxsize=1) which was equivalent but slightly cleaner. The new approach works but the password persists in self.password and self._cached_header — consider whether password should be cleared after first header generation if security is a concern.

8. download_from_response_stream in async utilities opens file synchronously

In simple_ado/_async/utilities.py:

with open(output_path, "wb") as output_file:
    async for data in response.aiter_bytes(chunk_size=chunk_size):
        output_file.write(data)

This mixes async iteration with synchronous file I/O. For truly non-blocking async, use aiofiles or run file writes in an executor. As-is, this blocks the event loop during each write().


Minor / Style Issues

9. The cast(SplitResult, urllib.parse.urlsplit(location)) in builds.py is unnecessary — urlsplit already returns SplitResult.

10. response.read() calls before raising ADOHTTPException in the artifact download redirect loop are good practice (needed for httpx streaming responses), but add a brief comment explaining why — it's not obvious.

11. The __setitem_async__ method on ADOAsyncWorkItem is never actually callable via work_item[key] = value syntax (Python doesn't support async __setitem__). Consider removing it — users should use await work_item.set(key, value) instead.

12. Duplicate AlertSeverity and TaskAgentPoolActionFilter enum definitions — these enums are duplicated between sync and async code rather than shared from a common module. If they're truly identical (and they appear to be), move them to a shared simple_ado/enums.py or similar to reduce drift risk.


Positive Aspects

  • Clean separation: async code as source of truth with generated sync code is a solid approach
  • Backward compatibility shims (types.py, _CompatResponse.ok) are thoughtful
  • Context manager support (__enter__/__exit__, __aenter__/__aexit__) for proper resource cleanup
  • Comprehensive test mirroring between async and sync
  • Rate limiting logic properly ported to both variants
  • The get_field() method with auto_refresh is a nice addition over the implicit refresh in __getitem__

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants