MPT-22959 Buffer streamed bodies before 401 retry in extension framework auth#366
Conversation
|
CodeAnt AI is reviewing your PR. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📜 Recent review details⏰ Context from checks skipped due to timeout. (2)
📝 WalkthroughWalkthroughAdds request-body buffering to ChangesStreamed body retry buffering
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
codeant-ai
Bot
added
the
size:M
PR Summary by QodoBuffer streamed request bodies before 401 retry in ExtensionFrameworkAuthentication
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| self, request: httpx.Request | ||
| ) -> Generator[httpx.Request, httpx.Response, None]: | ||
| """Attach a cached token, refreshing it proactively and on 401.""" | ||
| request.read() |
There was a problem hiding this comment.
Suggestion: The new unconditional pre-read buffers every request body into memory before the first send, which breaks true streaming semantics and can cause very large uploads to consume excessive RAM (or fail with out-of-memory) even when no 401 retry happens. Restrict buffering to bounded-size bodies or add a safe fallback path for large/non-bufferable streams instead of always materializing the full payload. [performance]
Severity Level: Major ⚠️
- ❌ All sync authenticated requests buffer bodies in memory.
- ⚠️ Large streamed uploads risk high memory consumption.
- ⚠️ Streaming semantics lost for iterator-based request bodies.Steps of Reproduction ✅
1. In tests/unit/auth/test_extension_framework.py:24, construct an HTTPClient with
ExtensionFrameworkAuthentication by calling HTTPClient(base_url=API_URL,
authentication=ExtensionFrameworkAuthentication(SECRET)).
2. In the same test file at lines 26-28, call client.httpx_client.request("POST",
"/orders", content=iter([b"chunk-1", b"chunk-2"])) to send a streamed body backed by an
iterator.
3. This request triggers ExtensionFrameworkAuthentication.sync_auth_flow at
mpt_api_client/auth/extension_framework.py:81-93; at line 85 the code executes
request.read(), which causes httpx to fully buffer the entire request body into memory and
replace the one-shot stream with a ByteStream.
4. If the iterator in step 2 were producing a very large upload instead of two small
chunks, sync_auth_flow would still unconditionally materialize the whole payload before
sending, defeating streaming semantics and potentially consuming excessive memory or
causing out-of-memory errors even when the first request succeeds without a 401 retry.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** mpt_api_client/auth/extension_framework.py
**Line:** 85:85
**Comment:**
*Performance: The new unconditional pre-read buffers every request body into memory before the first send, which breaks true streaming semantics and can cause very large uploads to consume excessive RAM (or fail with out-of-memory) even when no 401 retry happens. Restrict buffering to bounded-size bodies or add a safe fallback path for large/non-bufferable streams instead of always materializing the full payload.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fixThere was a problem hiding this comment.
Thanks for the review — this behaviour is intentional, so we're keeping the unconditional buffering:
-
It is the deliberate trade-off of this fix. The goal of MPT-22959 is that the request body is read into memory before the auth flow so the 401 retry is always replayable. It is also exactly what httpx itself does for auth schemes that declare
requires_request_body = True: the default auth-flow wrapper performs the same unconditionalrequest.read(), with no size bound or fallback. Since this class overrides the flow wrappers, the explicitrequest.read()/await request.aread()just replicates the standard httpx convention. -
Restricting buffering to bounded-size bodies would reintroduce the bug. Large non-seekable streamed uploads are precisely the case where the 401 retry silently corrupts data (truncated multipart parts,
httpx.StreamConsumed). A size-capped or fallback path would trade silent data corruption for memory savings on exactly the requests that matter most — the wrong trade for an API client. -
The practical impact is limited. Typical bodies in this client (
json=, multipart with seekable files) are either already in memory or cheaply replayable; huge non-seekable streams are a marginal case, and the buffering behaviour is now documented indocs/usage.mdso callers can account for it.
There was a problem hiding this comment.
✅ Customized review instruction saved!
Instruction:
Do not flag unconditional request-body buffering in auth-flow wrappers that require replayable requests for 401 retries and intentionally set
requires_request_body = True.
Applied to:
mpt_api_client/auth/**
💡 To manage or update this instruction, visit: CodeAnt AI Settings
| self, request: httpx.Request | ||
| ) -> AsyncGenerator[httpx.Request, httpx.Response]: | ||
| """Attach a cached token, refreshing it proactively and on 401.""" | ||
| await request.aread() |
There was a problem hiding this comment.
Suggestion: The async flow now always drains the full request stream into memory before sending, which can block or exhaust memory for large/long-lived async streams and removes expected streaming behavior for normal successful requests. Add size-aware buffering or skip replay logic for oversized/non-replayable streams to avoid forcing full in-memory reads. [performance]
Severity Level: Major ⚠️
- ❌ Async extension client always buffers request bodies in memory.
- ⚠️ Large async streams can exhaust memory resources.
- ⚠️ Async streaming uploads lose progressive send behavior.Steps of Reproduction ✅
1. In tests/unit/auth/test_extension_framework.py:154-156, construct an AsyncHTTPClient
with ExtensionFrameworkAuthentication by calling AsyncHTTPClient(base_url=API_URL,
authentication=ExtensionFrameworkAuthentication(SECRET)).
2. In the same file at lines 158-162, define an async generator stream_body that yields
multiple byte chunks, simulating a streamed request body produced over time.
3. At lines 163-165, call client.httpx_client.request("POST", "/orders",
content=stream_body()) to send the async streamed content through the authenticated
client.
4. This call triggers ExtensionFrameworkAuthentication.async_auth_flow at
mpt_api_client/auth/extension_framework.py:96-108; at line 100 the code executes await
request.aread(), which drains the entire async stream into memory before the first send,
so for large or long-lived streams the authentication layer forces full in-memory
buffering and removes true streaming behavior, increasing memory usage even when the
initial request does not receive a 401 response.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** mpt_api_client/auth/extension_framework.py
**Line:** 100:100
**Comment:**
*Performance: The async flow now always drains the full request stream into memory before sending, which can block or exhaust memory for large/long-lived async streams and removes expected streaming behavior for normal successful requests. Add size-aware buffering or skip replay logic for oversized/non-replayable streams to avoid forcing full in-memory reads.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
mpt_api_client/auth/extension_framework.py (1)
85-85: 🚀 Performance & Scalability | 🔵 TrivialBuffering placement is correct; note the streaming/memory trade-off.
Reading the full body before the first send is required for retry-safety and is placed correctly (before the first
yield request). Per httpx's own docs, settingrequires_request_body(or, as here, explicitly reading the body) prevents true streaming for that auth scheme — the whole body is loaded into memory on every request, not just on 401 retries. If this auth provider is ever used with large file uploads (e.g.CreateFileMixin/UpdateFileMixin), this removes the memory benefit of streaming uploads. This is an inherent and likely acceptable trade-off for the bug being fixed, worth being aware of.Also applies to: 100-100
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@mpt_api_client/auth/extension_framework.py` at line 85, The buffering in the auth flow is already correctly placed in the extension framework request handler, so no functional change is needed; keep the full-body read in `request.read()` before the first `yield request` to preserve retry-safety. If you touch this area again, retain that ordering in the auth provider logic and be mindful that it intentionally trades away true streaming for large uploads handled by `CreateFileMixin` and `UpdateFileMixin`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@mpt_api_client/auth/extension_framework.py`:
- Line 85: The buffering in the auth flow is already correctly placed in the
extension framework request handler, so no functional change is needed; keep the
full-body read in `request.read()` before the first `yield request` to preserve
retry-safety. If you touch this area again, retain that ordering in the auth
provider logic and be mindful that it intentionally trades away true streaming
for large uploads handled by `CreateFileMixin` and `UpdateFileMixin`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 2a1a6683-18eb-42e2-8719-1bdcc62a9395
📒 Files selected for processing (3)
docs/usage.mdmpt_api_client/auth/extension_framework.pytests/unit/auth/test_extension_framework.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
softwareone-platform/mpt-extension-skills(manual)
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (6)
docs/**/*.md
📄 CodeRabbit inference engine (docs/contributing.md)
When repository behavior changes, update the narrowest relevant document under
docs/
Files:
docs/usage.md
docs/*.md
📄 CodeRabbit inference engine (docs/documentation.md)
docs/*.md: Topic-specific documentation must live in the matching file underdocs/directory
Shared engineering rules must be linked frommpt-extension-skillsinstead of copied into this repository
Files:
docs/usage.md
docs/usage.md
📄 CodeRabbit inference engine (docs/documentation.md)
docs/usage.mdis the source of truth for installation, configuration, examples, and supported command entry points
Files:
docs/usage.md
docs/**
📄 CodeRabbit inference engine (AGENTS.md)
docs/**: Put topic-specific documentation underdocs/directory instead of expandingREADME.md
Link shared engineering rules frommpt-extension-skillsinstead of duplicating them locally in documentation
Files:
docs/usage.md
⚙️ CodeRabbit configuration file
docs/**: # ArchitectureThis document describes the internal architecture of
mpt-api-python-client.Overview
mpt-api-python-clientis a Python API client that provides a typed, fluent interface for the
SoftwareONE Marketplace Platform (MPT) REST API. It supports both synchronous and asynchronous
usage and is built on top of httpx.API Reference: The full upstream API contract is described by the
MPT OpenAPI Spec.
The client mirrors this spec's resource structure.The client exposes every MPT API domain (catalog, commerce, billing, etc.) as a resource group,
where each resource is a service object composed from reusable HTTP operation mixins.Directory Structure
mpt_api_client/ ├── __init__.py # Public API: MPTClient, AsyncMPTClient, RQLQuery ├── mpt_client.py # Client entry points ├── constants.py # Shared constants (content types) ├── exceptions.py # Error hierarchy (MPTError, MPTHttpError, MPTAPIError) │ ├── http/ # HTTP transport layer │ ├── client.py # Sync HTTPClient (httpx.Client) │ ├── async_client.py # Async AsyncHTTPClient (httpx.AsyncClient) │ ├── base_service.py # ServiceBase — shared service logic │ ├── service.py # Service — sync service (extends ServiceBase) │ ├── async_service.py # AsyncService — async service (extends ServiceBase) │ ├── query_state.py # Query parameter accumulation │ ├── client_utils.py # URL validation helpers │ ├── types.py # Type aliases (Response, HeaderTypes, etc.) │ └── mixins/ # Composable HTTP operation mixins │ ├── collection_mixin.py │ ├── create_mixin.py │ ├── create_file_mixin.py │ ├── update_mixin.py │ ├── update_file_mixin.py │ ├── delete_mixin.py │ ├── get_mixin.py │ ├── enable_mixin.py │ ├── disable_mixin...
Files:
docs/usage.md
⚙️ CodeRabbit configuration file
docs/**: Review documentation changes againstdocs/documentation.mdand the linked repository'sstandards/documentation.md.
Use those documents as the source of truth for structure, topic boundaries, navigation updates, and when to link shared rules instead of copying them.
Files:
docs/usage.md
**
⚙️ CodeRabbit configuration file
**: # AGENTS.mdWorking protocol for any task in this repository:
- Identify the task type and select only the local repository files that are relevant to that task.
- Read only those relevant local files before making changes.
- If any selected local file references shared standards or shared operational guidance that are relevant to the same task, read those shared documents too before proceeding.
- Treat repository-local documents as repository-specific additions, restrictions, or overrides to shared guidance.
- If a repository-local rule conflicts with a shared rule, the local repository rule takes precedence.
Python API client for the SoftwareONE Marketplace Platform (MPT) API. Provides synchronous
(MPTClient) and asynchronous (AsyncMPTClient) clients built on httpx, with typed
resource services, mixin-based HTTP operations, and an RQL query builder.Documentation Reading Order
When applicable, read the repository documentation in this order:
README.md— repository overview, quick start, and documentation mapdocs/usage.md— installation, configuration, Python usage examples, and supported Docker-based commandsdocs/architecture.md— layered architecture, directory structure, and key abstractionsdocs/local-development.md— Docker-only setup and execution modeldocs/testing.md— repository-specific testing strategy and command mappingdocs/contributing.md— repository-specific workflow and links to shared standardsdocs/documentation.md— repository-specific documentation rulesdocs/unit_tests.md— unit test structure and guidancedocs/e2e_tests.md— end-to-end test setup and executionThen inspect the code paths relevant to the task:
mpt_api_client/mpt_client.py— public sync and async client entry pointsmpt_api_client/http/— HTTP clients, services, query state, and reusable mixinsmpt_api_client/resources/— domain resource groups such as catalog, commerce, billing, and integrati...
Files:
docs/usage.mdmpt_api_client/auth/extension_framework.pytests/unit/auth/test_extension_framework.py
**/*
⚙️ CodeRabbit configuration file
**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved
Files:
docs/usage.mdmpt_api_client/auth/extension_framework.pytests/unit/auth/test_extension_framework.py
🧠 Learnings (6)
📚 Learning: 2026-02-17T10:04:00.873Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 210
File: mpt_api_client/rql/query_builder.py:18-18
Timestamp: 2026-02-17T10:04:00.873Z
Learning: In this repository, Ruff and flake8 with wemake-python-styleguide are used together. Do not remove WPS* noqa directives (e.g., WPS231) even if Ruff flags them as unknown in RUF102. Keep the directives to satisfy flake8 rules; ensure tooling configuration accounts for both linters to avoid false positives.
Applied to files:
mpt_api_client/auth/extension_framework.py
📚 Learning: 2026-04-16T13:00:41.320Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 303
File: tests/e2e/helpdesk/chats/participants/conftest.py:25-31
Timestamp: 2026-04-16T13:00:41.320Z
Learning: In mpt-api-python-client, do not treat list-wrapped arguments to CreateMixin.create() / AsyncCreateMixin.create() as an error. ResourceData is intentionally typed as Resource | list[Resource] (see mpt_api_client/models/model.py), so create() should accept either a single resource dict or a list of resource dicts (e.g., create([chat_participant_data])) to perform a batch create and return a ModelCollection. Therefore, reviewers should only flag list-wrapped create() arguments when there is evidence they violate the expected API contract beyond this documented batch-create behavior.
Applied to files:
mpt_api_client/auth/extension_framework.pytests/unit/auth/test_extension_framework.py
📚 Learning: 2026-06-23T10:58:23.527Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 354
File: tests/e2e/exchange/currencies/conftest.py:1-20
Timestamp: 2026-06-23T10:58:23.527Z
Learning: When using the `iso4217` library’s `Currency` enum in Python, remember that `Currency.<X>.value` is intentionally equivalent to `Currency.<X>.code` (both return the same 3-letter ISO 4217 code). Prefer `.code` in new code because it’s the library’s documented public accessor and more intention-revealing, but do not flag existing usages of `.value` as a bug or as raising `AttributeError`/nonexistent attribute issues.
Applied to files:
mpt_api_client/auth/extension_framework.pytests/unit/auth/test_extension_framework.py
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/unit/auth/test_extension_framework.py
📚 Learning: 2026-04-02T09:35:03.825Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 269
File: tests/e2e/helpdesk/chats/links/test_sync_links.py:18-18
Timestamp: 2026-04-02T09:35:03.825Z
Learning: In this repository’s test suite, flake8-aaa/flake8-aaa codes use short two-digit suffixes (e.g., `# noqa: AAA01`), not three-digit variants like `AAA001`. If you see `# noqa: AAA01` in a test file (e.g., when the Act step is performed via a pytest fixture rather than inline code), treat it as valid and intentional—do not flag it as an invalid/no-longer-needed noqa, and do not suggest removing it even if Ruff reports `RUF102`, since `AAA` is configured under `tool.ruff.lint.external` and these noqa directives are expected to be preserved.
Applied to files:
tests/unit/auth/test_extension_framework.py
📚 Learning: 2026-02-02T13:05:41.144Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 201
File: tests/unit/resources/accounts/mixins/test_activatable_mixin.py:132-139
Timestamp: 2026-02-02T13:05:41.144Z
Learning: In the mpt-api-python-client repository, tests are configured to use pytest asyncio mode auto, which auto-detects async test functions and runs them without requiring pytest.mark.asyncio. Reviewers should rely on this behavior for all Python test files under tests/, and avoid adding unnecessary asyncio markers in async tests. Ensure test files in tests/ adhere to this convention unless a specific test requires an explicit marker.
Applied to files:
tests/unit/auth/test_extension_framework.py
🔇 Additional comments (3)
mpt_api_client/auth/extension_framework.py (1)
43-48: Correct: matches httpx's own buffering semantics.Confirmed against httpx's source: the default
sync_auth_flow/async_auth_flowwrappers only callrequest.read()/await request.aread()whenrequires_request_bodyis set, and they do so before delegating toauth_flow(). Since this class overridessync_auth_flow/async_auth_flowdirectly, that default check is bypassed, so the explicitread()/aread()calls added here are indeed required — the comment's reasoning is accurate.tests/unit/auth/test_extension_framework.py (1)
4-4: Solid regression coverage for both sync and async retry paths.Both new tests correctly simulate a
401→200retry with a streamed body (iter([...])for sync, an async generator for async) and assert the retried request carries the refreshed token and the fully concatenated body. TheAsyncGenerator[bytes]return-type annotation on the nestedstream_bodyhelper is valid — theSendTypeparameter defaults toNonewhen omitted, per current Python typing docs.Also applies to: 109-136, 239-273
docs/usage.md (1)
49-51: LGTM!
Code Review by Qodo
Context used✅ Compliance rules (platform):
2 rules 1. Unbounded in-memory buffering
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/auth/test_extension_framework.py`:
- Around line 70-71: The test function
`test_extension_framework_refreshes_expired_token` is incorrectly declared as
async even though it uses the sync `extension_http_client` fixture and makes a
synchronous `.request(...)` call with no await. Change this test from `async
def` to a plain `def`, matching the other sync tests in the same module and
keeping it aligned with the `_async` version only where awaits are actually
used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0a5f9cba-0f38-4235-a8f2-4013fadb7090
📒 Files selected for processing (1)
tests/unit/auth/test_extension_framework.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
softwareone-platform/mpt-extension-skills(manual)
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**
⚙️ CodeRabbit configuration file
**: # AGENTS.mdWorking protocol for any task in this repository:
- Identify the task type and select only the local repository files that are relevant to that task.
- Read only those relevant local files before making changes.
- If any selected local file references shared standards or shared operational guidance that are relevant to the same task, read those shared documents too before proceeding.
- Treat repository-local documents as repository-specific additions, restrictions, or overrides to shared guidance.
- If a repository-local rule conflicts with a shared rule, the local repository rule takes precedence.
Python API client for the SoftwareONE Marketplace Platform (MPT) API. Provides synchronous
(MPTClient) and asynchronous (AsyncMPTClient) clients built on httpx, with typed
resource services, mixin-based HTTP operations, and an RQL query builder.Documentation Reading Order
When applicable, read the repository documentation in this order:
README.md— repository overview, quick start, and documentation mapdocs/usage.md— installation, configuration, Python usage examples, and supported Docker-based commandsdocs/architecture.md— layered architecture, directory structure, and key abstractionsdocs/local-development.md— Docker-only setup and execution modeldocs/testing.md— repository-specific testing strategy and command mappingdocs/contributing.md— repository-specific workflow and links to shared standardsdocs/documentation.md— repository-specific documentation rulesdocs/unit_tests.md— unit test structure and guidancedocs/e2e_tests.md— end-to-end test setup and executionThen inspect the code paths relevant to the task:
mpt_api_client/mpt_client.py— public sync and async client entry pointsmpt_api_client/http/— HTTP clients, services, query state, and reusable mixinsmpt_api_client/resources/— domain resource groups such as catalog, commerce, billing, and integrati...
Files:
tests/unit/auth/test_extension_framework.py
**/*
⚙️ CodeRabbit configuration file
**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved
Files:
tests/unit/auth/test_extension_framework.py
🧠 Learnings (5)
📚 Learning: 2025-12-12T15:02:20.732Z
Learnt from: robcsegal
Repo: softwareone-platform/mpt-api-python-client PR: 160
File: tests/e2e/commerce/agreement/attachment/test_async_agreement_attachment.py:55-58
Timestamp: 2025-12-12T15:02:20.732Z
Learning: In pytest with pytest-asyncio, if a test function uses async fixtures but contains no await, declare the test function as def (synchronous) instead of async def. Pytest-asyncio will resolve the async fixtures automatically; this avoids linter complaints about unnecessary async functions. This pattern applies to any test file under the tests/ directory that uses such fixtures.
Applied to files:
tests/unit/auth/test_extension_framework.py
📚 Learning: 2026-04-02T09:35:03.825Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 269
File: tests/e2e/helpdesk/chats/links/test_sync_links.py:18-18
Timestamp: 2026-04-02T09:35:03.825Z
Learning: In this repository’s test suite, flake8-aaa/flake8-aaa codes use short two-digit suffixes (e.g., `# noqa: AAA01`), not three-digit variants like `AAA001`. If you see `# noqa: AAA01` in a test file (e.g., when the Act step is performed via a pytest fixture rather than inline code), treat it as valid and intentional—do not flag it as an invalid/no-longer-needed noqa, and do not suggest removing it even if Ruff reports `RUF102`, since `AAA` is configured under `tool.ruff.lint.external` and these noqa directives are expected to be preserved.
Applied to files:
tests/unit/auth/test_extension_framework.py
📚 Learning: 2026-02-02T13:05:41.144Z
Learnt from: albertsola
Repo: softwareone-platform/mpt-api-python-client PR: 201
File: tests/unit/resources/accounts/mixins/test_activatable_mixin.py:132-139
Timestamp: 2026-02-02T13:05:41.144Z
Learning: In the mpt-api-python-client repository, tests are configured to use pytest asyncio mode auto, which auto-detects async test functions and runs them without requiring pytest.mark.asyncio. Reviewers should rely on this behavior for all Python test files under tests/, and avoid adding unnecessary asyncio markers in async tests. Ensure test files in tests/ adhere to this convention unless a specific test requires an explicit marker.
Applied to files:
tests/unit/auth/test_extension_framework.py
📚 Learning: 2026-04-16T13:00:41.320Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 303
File: tests/e2e/helpdesk/chats/participants/conftest.py:25-31
Timestamp: 2026-04-16T13:00:41.320Z
Learning: In mpt-api-python-client, do not treat list-wrapped arguments to CreateMixin.create() / AsyncCreateMixin.create() as an error. ResourceData is intentionally typed as Resource | list[Resource] (see mpt_api_client/models/model.py), so create() should accept either a single resource dict or a list of resource dicts (e.g., create([chat_participant_data])) to perform a batch create and return a ModelCollection. Therefore, reviewers should only flag list-wrapped create() arguments when there is evidence they violate the expected API contract beyond this documented batch-create behavior.
Applied to files:
tests/unit/auth/test_extension_framework.py
📚 Learning: 2026-06-23T10:58:23.527Z
Learnt from: jentyk
Repo: softwareone-platform/mpt-api-python-client PR: 354
File: tests/e2e/exchange/currencies/conftest.py:1-20
Timestamp: 2026-06-23T10:58:23.527Z
Learning: When using the `iso4217` library’s `Currency` enum in Python, remember that `Currency.<X>.value` is intentionally equivalent to `Currency.<X>.code` (both return the same 3-letter ISO 4217 code). Prefer `.code` in new code because it’s the library’s documented public accessor and more intention-revealing, but do not flag existing usages of `.value` as a bug or as raising `AttributeError`/nonexistent attribute issues.
Applied to files:
tests/unit/auth/test_extension_framework.py
🔇 Additional comments (2)
tests/unit/auth/test_extension_framework.py (2)
42-48: LGTM!Also applies to: 57-67, 94-116, 147-158, 287-292, 304-308, 329-345, 347-359
160-172: LGTM!Also applies to: 174-188, 190-210, 214-236, 272-285
…work auth ExtensionFrameworkAuthentication retries requests on 401 Unauthorized, but one-shot streamed bodies (raw iterators, non-seekable files) were consumed by the first send, so the retry re-sent an empty or broken body: multipart parts backed by non-seekable files were silently truncated and raw iterator bodies raised httpx.StreamConsumed. Buffer the request body at the start of both auth flows (httpx only honours requires_request_body in its default flow wrappers, which this class overrides) so the retried request is always replayable. Add sync and async regression tests that stream a body through a 401/200 sequence over shared extension-client fixtures, and document the buffering behaviour in docs/usage.md. MPT-22959 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
svazquezco
force-pushed
the
bugfix/MPT-22959/extensionframeworkauthentication-401-retry-re-sends-consumed-streamed-request-bodies
branch
from
d0ad4ff to
d08acf3
Compare
|
1 participant
User description
🤖 AI-generated PR — Please review carefully.
What was done
ExtensionFrameworkAuthenticationretries requests on401 Unauthorizedin bothsync_auth_flowandasync_auth_flow, but one-shot streamed request bodies (rawiterators, non-seekable file objects) were consumed by the first send, so the retry
re-sent an already-consumed stream: multipart parts backed by non-seekable files were
silently truncated and raw iterator bodies raised
httpx.StreamConsumed.request.read()/await request.aread()), which makes httpx replace the one-shot stream with anin-memory replayable
ByteStream, so the401retry always re-sends the intact body.requires_request_body = Truefor consistency with the httpx convention(httpx custom authentication docs);
httpx only honours the flag in its default auth-flow wrappers, which this class
overrides, so the explicit buffering above is what enforces it (comment added in code).
401→200sequence and assert the retried request carries the fresh token and the intact body.
docs/usage.md.Same issue as MPT-22954 in the extension SDK (
AccountScopedAuthentication,softwareone-platform/mpt-extension-sdk#246).
Jira: MPT-22959
Testing
test_extension_framework_retry_resends_streamed_body(sync)and
test_extension_framework_retry_resends_streamed_body_async; both fail withoutthe fix (empty retried body) and pass with it.
make check-allgreen: 2312 unit tests, ruff, flake8, mypy,uv lock --check.CodeAnt-AI Description
Keep streamed request bodies intact when a 401 retry happens
What Changed
Impact
✅ Reliable 401 retries for streamed uploads✅ Fewer broken multipart requests✅ Clearer behavior for one-shot request bodies💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Closes MPT-22959
ExtensionFrameworkAuthenticationbefore any token refresh/401 Unauthorizedretry so streamed one-shot bodies can be replayed in both synchronous and asynchronous auth flows.requires_request_body = Trueto align withhttpxauthentication conventions.401retries that (a) refresh the bearer token and (b) preserve/resend streamed request bodies, including retry behavior for a non-idempotent POST.tests/unit/auth/test_extension_framework.pyto use shared sync/asyncHTTPClientfixtures forExtensionFrameworkAuthentication.docs/usage.mdto document request-body buffering for retries and account-scoped token retrieval via?account.id=<id>.