Skip to content

MPT-21666 stream statement charges as JSONL#333

Merged
albertsola merged 1 commit into
mainfrom
MPT-21666-stream-statement-charges-jsonl
Jun 2, 2026
Merged

MPT-21666 stream statement charges as JSONL#333
albertsola merged 1 commit into
mainfrom
MPT-21666-stream-statement-charges-jsonl

Conversation

@albertsola

@albertsola albertsola commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

🤖 AI-generated PR — Please review carefully.

What was done

Add line-by-line JSONL/NDJSON streaming for statement charges so large charge sets can be consumed without buffering full pages into memory.

  • Add a stream() context-manager primitive to HTTPClient/AsyncHTTPClient that opens a streaming response (Accept: application/jsonl) and follows the redirect to the charge blob automatically.
  • Add StreamJSONLMixin/AsyncStreamJSONLMixin that iterate the response lines and yield one parsed model per line; wire it into StatementChargesService, exposing client.billing.statements.charges(id).stream().
  • Populate the StatementCharge model with the full Charge schema fields from the MPT OpenAPI spec (typed scalars + nested BaseModel refs).
  • Add unit and e2e coverage; ignore .claude/ local artifacts.

Testing

Unit and e2e coverage added for the streaming primitive, the JSONL mixins, and the statement charges service.

Closes MPT-21666

  • Add JSONL/NDJSON streaming for statement charges to stream large charge sets without buffering full pages
  • Introduce HTTPClient.stream and AsyncHTTPClient.stream context managers that open streaming responses with Accept: application/jsonl and follow charge-blob redirects
  • Add StreamJSONLMixin and AsyncStreamJSONLMixin to iterate JSONL response lines and yield one parsed model per line
  • Wire streaming mixins into StatementChargesService and AsyncStatementChargesService so callers can use client.billing.statements.charges(id).stream()
  • Expand StatementCharge model with full Charge schema fields (typed scalars and nested BaseModel references)
  • Add unit and end-to-end tests for the streaming primitives, JSONL mixins, and statement charges service
  • Update .gitignore to ignore local .claude/ artifacts

@albertsola albertsola requested a review from a team as a code owner June 2, 2026 13:42
@coderabbitai

coderabbitai Bot commented Jun 2, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository YAML (base), Organization UI (inherited)

Review profile: CHILL

Plan: Pro

Run ID: 10b9d4db-d032-42c4-8f44-3ce9a989ab7b

📥 Commits

Reviewing files that changed from the base of the PR and between 80c0659 and 21579d3.

⛔ Files ignored due to path filters (1)
  • .gitignore is excluded by !.gitignore
📒 Files selected for processing (12)
  • mpt_api_client/constants.py
  • mpt_api_client/http/async_client.py
  • mpt_api_client/http/client.py
  • mpt_api_client/http/mixins/__init__.py
  • mpt_api_client/http/mixins/stream_jsonl_mixin.py
  • mpt_api_client/resources/billing/statement_charges.py
  • tests/e2e/billing/statement/charge/test_async_statement_charge.py
  • tests/e2e/billing/statement/charge/test_sync_statement_charge.py
  • tests/unit/http/mixins/test_stream_jsonl_mixin.py
  • tests/unit/http/test_async_client.py
  • tests/unit/http/test_client.py
  • tests/unit/resources/billing/test_statement_charges.py
✅ Files skipped from review due to trivial changes (1)
  • mpt_api_client/constants.py
🚧 Files skipped from review as they are similar to previous changes (9)
  • tests/e2e/billing/statement/charge/test_sync_statement_charge.py
  • tests/e2e/billing/statement/charge/test_async_statement_charge.py
  • mpt_api_client/http/client.py
  • mpt_api_client/http/async_client.py
  • mpt_api_client/http/mixins/stream_jsonl_mixin.py
  • mpt_api_client/resources/billing/statement_charges.py
  • tests/unit/http/test_client.py
  • tests/unit/http/test_async_client.py
  • mpt_api_client/http/mixins/init.py
📜 Recent review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: build
🧰 Additional context used
📓 Path-based instructions (1)
**/*

⚙️ CodeRabbit configuration file

**/*: For each subsequent commit in this PR, explicitly verify if previous review comments have been resolved

Files:

  • tests/unit/http/mixins/test_stream_jsonl_mixin.py
  • tests/unit/resources/billing/test_statement_charges.py
🧠 Learnings (4)
📚 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/http/mixins/test_stream_jsonl_mixin.py
  • tests/unit/resources/billing/test_statement_charges.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/http/mixins/test_stream_jsonl_mixin.py
  • tests/unit/resources/billing/test_statement_charges.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/http/mixins/test_stream_jsonl_mixin.py
  • tests/unit/resources/billing/test_statement_charges.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/http/mixins/test_stream_jsonl_mixin.py
  • tests/unit/resources/billing/test_statement_charges.py
🪛 Ruff (0.15.15)
tests/unit/http/mixins/test_stream_jsonl_mixin.py

[warning] 13-13: Invalid rule code in # noqa: WPS215

Add non-Ruff rule codes to the lint.external configuration option

(RUF102)


[warning] 21-21: Invalid rule code in # noqa: WPS215

Add non-Ruff rule codes to the lint.external configuration option

(RUF102)

tests/unit/resources/billing/test_statement_charges.py

[warning] 79-79: Invalid rule code in # noqa: WPS218

Add non-Ruff rule codes to the lint.external configuration option

(RUF102)


[warning] 89-89: Invalid rule code in # noqa: WPS218

Add non-Ruff rule codes to the lint.external configuration option

(RUF102)

🔇 Additional comments (4)
tests/unit/http/mixins/test_stream_jsonl_mixin.py (2)

1-10: LGTM!

Also applies to: 17-19, 29-77


13-16: ⚡ Quick win

Remove the WPS215 noqa configuration concern for this test module — Ruff 0.15.15 (ruff check --config pyproject.toml tests/unit/http/mixins/test_stream_jsonl_mixin.py) passes with the existing # noqa: WPS215 usages, so there’s no RUF102 invalid-noqa issue here.

tests/unit/resources/billing/test_statement_charges.py (2)

3-56: LGTM!

Also applies to: 73-78, 80-105


79-79: ⚡ Quick win

RUF102 / # noqa: WPS218: no config change needed
tool.ruff.lint.external already includes WPS (and tool.ruff.lint.select includes WPS), so # noqa: WPS218 is a valid wemake-python-styleguide rule suppression; dropping the request to add WPS to tool.ruff.lint.external is warranted.

			> Likely an incorrect or invalid review comment.

📝 Walkthrough

Walkthrough

This PR introduces JSONL (newline-delimited JSON) streaming support to the API client. The implementation spans HTTP-level streaming infrastructure, reusable service mixins that wrap HTTP streaming, and integration with the StatementCharges resource.

Changes

JSONL Streaming Support

Layer / File(s) Summary
HTTP Streaming Foundation
mpt_api_client/constants.py, mpt_api_client/http/client.py, mpt_api_client/http/async_client.py, tests/unit/http/test_client.py, tests/unit/http/test_async_client.py
Introduces APPLICATION_JSONL constant. Both sync and async HTTP clients gain stream() context manager methods that open unbuffered httpx streaming requests, consume error response bodies, apply shared error handling (mapping RequestError/HTTPError to MPTMaxRetryError/MPTError), and yield open responses for line-by-line consumption. Unit tests verify streaming line parsing, header propagation, and error handling.
JSONL Streaming Mixins
mpt_api_client/http/mixins/stream_jsonl_mixin.py, mpt_api_client/http/mixins/__init__.py, tests/unit/http/mixins/test_stream_jsonl_mixin.py
Adds StreamJSONLMixin and AsyncStreamJSONLMixin generic classes. Each provides a stream() method that performs an HTTP GET with Accept: application/jsonl header, iterates response lines, filters empty lines, parses JSON, and yields model instances. Mixins are exported from the mixins package. Unit tests mock JSONL responses and verify streaming yields parsed models and applies query filters.
StatementCharge Resource Integration
mpt_api_client/resources/billing/statement_charges.py, tests/unit/resources/billing/test_statement_charges.py, tests/e2e/billing/statement/charge/test_sync_statement_charge.py, tests/e2e/billing/statement/charge/test_async_statement_charge.py
StatementCharge model gains explicit typed attributes and expanded docstring. Both StatementChargesService and AsyncStatementChargesService inherit from the corresponding streaming mixins, enabling stream() calls. Unit tests validate model construction, type annotations on scalar and nested fields, and optional field omission. E2e tests confirm statement_charges.stream() returns non-empty charge collections with populated IDs.

🎯 3 (Moderate) | ⏱️ ~20 minutes


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (1 error)

Check name Status Explanation Resolution
Ai Warning Line In Pr Description ❌ Error PR description does not start with the required warning line. First line is "## PR summary" instead of "🤖 AI-generated PR — Please review carefully." Add the warning line "🤖 AI-generated PR — Please review carefully." as the first line of the PR description.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Pr And Commit Formatting ✅ Passed PR title "MPT-21666 stream statement charges as JSONL" matches format; description explains changes; commit follows Conventional Commits with detailed body; single commit; linear history.

Comment @coderabbitai help to get the list of available commands and usage tips.

🤖 AI-generated PR — Please review carefully.

Add line-by-line JSONL/NDJSON streaming for statement charges so large
charge sets can be consumed without buffering full pages into memory.

- Add a stream() context-manager primitive to HTTPClient/AsyncHTTPClient
  that opens a streaming response (Accept: application/jsonl) and follows
  the redirect to the charge blob automatically.
- Add StreamJSONLMixin/AsyncStreamJSONLMixin that iterate the response
  lines and yield one parsed model per line; wire it into
  StatementChargesService, exposing
  client.billing.statements.charges(id).stream().
- Populate the StatementCharge model with the full Charge schema fields
  from the MPT OpenAPI spec (typed scalars + nested BaseModel refs).
- Add unit and e2e coverage; ignore .claude/ local artifacts.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@albertsola albertsola force-pushed the MPT-21666-stream-statement-charges-jsonl branch from 80c0659 to 21579d3 Compare June 2, 2026 13:56
@sonarqubecloud

sonarqubecloud Bot commented Jun 2, 2026

Copy link
Copy Markdown

@albertsola albertsola merged commit edcf3a2 into main Jun 2, 2026
4 checks passed
@albertsola albertsola deleted the MPT-21666-stream-statement-charges-jsonl branch June 2, 2026 14:05
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