Skip to content

MPT-20245 Allow for array of objects in the response#301

Merged
jentyk merged 3 commits intomainfrom
fix/MPT-20245
Apr 16, 2026
Merged

MPT-20245 Allow for array of objects in the response#301
jentyk merged 3 commits intomainfrom
fix/MPT-20245

Conversation

@jentyk
Copy link
Copy Markdown
Member

@jentyk jentyk commented Apr 15, 2026

Closes MPT-20245

  • Model.from_response() now returns either a single Model or a ModelCollection when the API response JSON is a list of objects (raises TypeError for other payload types with an updated message).
  • Replaced public/internal uses of Collection with ModelCollection across the package and adjusted exports (mpt_api_client.models.all).
  • ServiceBase.make_collection(), CollectionMixin.fetch_page(), and AsyncCollectionMixin.fetch_page() now construct and return ModelCollection[Model].
  • ResourceAccessor._action() and AsyncResourceAccessor._action() return ResourceModel | ModelCollection[ResourceModel]; public get/post/put keep ResourceModel annotations and use # type: ignore[return-value] on their returns.
  • Widened service method return annotations for endpoints that may return multiple records:
    • AccountsUserGroupsService.update / AsyncAccountsUserGroupsService.update
    • CustomLedgersService.upload / AsyncCustomLedgersService.upload
    • JournalsService.upload / AsyncJournalsService.upload
    • BatchesService.get_attachment / AsyncBatchesService.get_attachment
  • Removed Model.new(); models are now instantiated directly via the class constructor and Model.from_response handles dict/list branching.
  • ModelCollection implementation updated (renamed from Collection, TYPE_CHECKING guards, forward references, ResourceList alias updated).
  • Tests updated/added to cover list responses and the revised TypeError message.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 15, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Replaced Collection with ModelCollection across models, HTTP layers, services, and tests. Model.from_response() now returns either a single model for dict payloads or a ModelCollection for list payloads. Type annotations and imports were updated accordingly.

Changes

Cohort / File(s) Summary
Core model types
mpt_api_client/models/__init__.py, mpt_api_client/models/model.py, mpt_api_client/models/model_collection.py
Renamed exported CollectionModelCollection; Model.from_response() returns `Self
HTTP base & mixins
mpt_api_client/http/base_service.py, mpt_api_client/http/mixins/collection_mixin.py
Changed return annotations and construction to use ModelCollection[Model] instead of Collection[Model]; instantiate models directly when building collections.
Resource accessor(s)
mpt_api_client/http/resource_accessor.py
Switched imports to ModelCollection/ResourceList forward-ref; widened private _action() return to `ResourceModel
Service methods (resources)
mpt_api_client/resources/accounts/accounts_user_groups.py, mpt_api_client/resources/billing/custom_ledgers.py, mpt_api_client/resources/billing/journals.py, mpt_api_client/resources/notifications/batches.py
Widened sync/async method return annotations to `SpecificModel
Tests
tests/unit/models/collection/conftest.py, tests/unit/models/test_model.py
Updated fixtures to construct ModelCollection; added test_from_response_list asserting list payloads produce ModelCollection; adjusted expected TypeError message for incompatible payloads.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Jira Issue Key In Title ✅ Passed The PR title contains exactly one Jira issue key in the correct format: MPT-20245.
Test Coverage Required ✅ Passed The PR modifies code files in mpt_api_client/ directory and includes corresponding test file changes in tests/ folder with new test cases and updated assertions.
Single Commit Required ✅ Passed The PR contains exactly one commit, keeping git history clean and satisfying the single commit requirement.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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

@jentyk jentyk marked this pull request as ready for review April 16, 2026 09:18
@jentyk jentyk requested a review from a team as a code owner April 16, 2026 09:18
@jentyk jentyk requested review from alephsur and robcsegal April 16, 2026 09:18
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
mpt_api_client/http/resource_accessor.py (1)

68-88: ⚠️ Potential issue | 🟠 Major

Widen the public return types to match _action()'s actual union return.

The _action() methods (lines 102 and 201) declare return type ResourceModel | ModelCollection[ResourceModel], but the public get(), post(), and put() methods (lines 66, 76, 86, 165, 175, 185) declare only ResourceModel. The # type: ignore[return-value] suppressions at lines 68, 78, 88, 167, 177, and 187 mask this mismatch rather than resolve it. Update the public method signatures to match the union return type of _action(), or constrain _action() to return a single type. Per coding guidelines, "Mypy is strict … keep type annotations accurate, and avoid new mypy issues in mpt_api_client."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mpt_api_client/http/resource_accessor.py` around lines 68 - 88, The public
HTTP methods get, post, and put currently declare return type ResourceModel but
actually forward to _action which returns ResourceModel |
ModelCollection[ResourceModel]; update the signatures of get, post, and put to
return ResourceModel | ModelCollection[ResourceModel] (or typing.Union[...]) and
remove the corresponding "# type: ignore[return-value]" suppressions so MyPy
sees the correct types; reference the methods get, post, put and the _action
return type along with ModelCollection and ResourceModel when making the change.
🧹 Nitpick comments (4)
mpt_api_client/resources/billing/journals.py (1)

50-52: Keep method docstrings in sync with widened return annotations.

Both upload() methods now return Journal | ModelCollection[Journal], but the Returns section still describes only Journal.

Suggested docstring update
-        Returns:
-            Journal: Created resource.
+        Returns:
+            Journal | ModelCollection[Journal]: Created resource(s).

Also applies to: 110-112

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mpt_api_client/resources/billing/journals.py` around lines 50 - 52, Update
the docstrings for the upload() methods to match the widened return type: change
the Returns section for the upload function(s) (referenced as upload in
journals.py) to indicate the method may return either a Journal or a
ModelCollection[Journal] (i.e., Journal | ModelCollection[Journal]) and describe
the conditions when each type is returned; ensure both occurrences (the one
around the first upload definition and the second at lines referenced in the
review) are updated consistently.
mpt_api_client/resources/billing/custom_ledgers.py (1)

50-52: Update Returns docstrings to match the new union return type.

Both upload() methods now return CustomLedger | ModelCollection[CustomLedger], but docstrings still document only CustomLedger. This can mislead SDK users and generated docs.

Suggested docstring update
-        Returns:
-            CustomLedger: Created resource.
+        Returns:
+            CustomLedger | ModelCollection[CustomLedger]: Created resource(s).

Also applies to: 111-113

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mpt_api_client/resources/billing/custom_ledgers.py` around lines 50 - 52,
Update the Returns section of the upload() docstrings in
mpt_api_client/resources/billing/custom_ledgers.py to reflect the new union
return type CustomLedger | ModelCollection[CustomLedger]; locate both
occurrences of the upload method (the one around the lines with signature "def
upload(self, custom_ledger_id: str, file: FileTypes) -> CustomLedger |
ModelCollection[CustomLedger]") and change the Returns text from only
"CustomLedger" to explicitly state that the method may return either a
CustomLedger instance or a ModelCollection[CustomLedger], including brief
descriptions of each case.
mpt_api_client/resources/accounts/accounts_user_groups.py (1)

39-41: Docstrings still describe the old single-model return contract.

update() now returns AccountsUserGroup | ModelCollection[AccountsUserGroup] in both sync/async services, so the Returns section should be updated accordingly.

Also applies to: 65-67

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mpt_api_client/resources/accounts/accounts_user_groups.py` around lines 39 -
41, The docstring for the update() method still describes a single-model return
but the signature returns AccountsUserGroup |
ModelCollection[AccountsUserGroup]; update the Returns section of the update()
docstring to state it can return either an AccountsUserGroup or a
ModelCollection[AccountsUserGroup] (mention both synchronous and asynchronous
service behaviors as needed), and make the same change to the other docstring
around lines 65-67 that documents the same method/overload so both docstrings
match the function signature (refer to update, AccountsUserGroup,
ModelCollection, and ResourceData to locate the spots).
mpt_api_client/resources/notifications/batches.py (1)

41-43: Update get_attachment() docstrings to reflect possible collection responses.

The signature now allows BatchAttachment | ModelCollection[BatchAttachment], but the current docs still imply only a single BatchAttachment.

Also applies to: 87-89

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mpt_api_client/resources/notifications/batches.py` around lines 41 - 43,
Update the docstrings for get_attachment (and the other method around lines
87–89 that has the same mismatch) to describe that the return type can be either
a single BatchAttachment or a ModelCollection[BatchAttachment]; explicitly state
the conditions under which a collection is returned (e.g., when multiple
attachments are matched or when attachment_id is omitted/represents a query),
include examples of both return shapes and mention the types BatchAttachment and
ModelCollection[BatchAttachment] by name so the docs match the function
signature.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@mpt_api_client/models/__init__.py`:
- Around line 4-6: The public API removed the Collection alias, breaking
consumers; restore a compatibility shim by reintroducing Collection =
ModelCollection in mpt_api_client.models (in the module where ModelCollection is
imported/defined, e.g., the __init__.py that currently imports ModelCollection),
and keep it exported via __all__ (add "Collection" to the list) so existing
imports continue to work for at least one deprecation cycle.

In `@mpt_api_client/models/model.py`:
- Around line 235-248: from_response currently returns a ModelCollection of raw
dicts when response.json() is a list; change it to instantiate each item as a
model before wrapping. In the from_response method, when response_data is a
list, map each dict item to cls(item, None) (or extract per-item Meta if your
API embeds it) and pass that list of model instances into ModelCollection
instead of the raw response_data so callers get Model objects (reference:
from_response, ModelCollection, Meta).

In `@tests/unit/models/test_model.py`:
- Around line 90-96: The test test_from_response_list only asserts the returned
object is a ModelCollection and its length, but doesn't verify items are
converted to Model instances; update the test for Model.from_response to assert
that result[0] is a Model (or use isinstance on each item), and check a field
value like result[0].id == "1" (or compare result.to_list() to the expected
list) so the test ensures ModelCollection contains Model objects rather than raw
dicts.

---

Outside diff comments:
In `@mpt_api_client/http/resource_accessor.py`:
- Around line 68-88: The public HTTP methods get, post, and put currently
declare return type ResourceModel but actually forward to _action which returns
ResourceModel | ModelCollection[ResourceModel]; update the signatures of get,
post, and put to return ResourceModel | ModelCollection[ResourceModel] (or
typing.Union[...]) and remove the corresponding "# type: ignore[return-value]"
suppressions so MyPy sees the correct types; reference the methods get, post,
put and the _action return type along with ModelCollection and ResourceModel
when making the change.

---

Nitpick comments:
In `@mpt_api_client/resources/accounts/accounts_user_groups.py`:
- Around line 39-41: The docstring for the update() method still describes a
single-model return but the signature returns AccountsUserGroup |
ModelCollection[AccountsUserGroup]; update the Returns section of the update()
docstring to state it can return either an AccountsUserGroup or a
ModelCollection[AccountsUserGroup] (mention both synchronous and asynchronous
service behaviors as needed), and make the same change to the other docstring
around lines 65-67 that documents the same method/overload so both docstrings
match the function signature (refer to update, AccountsUserGroup,
ModelCollection, and ResourceData to locate the spots).

In `@mpt_api_client/resources/billing/custom_ledgers.py`:
- Around line 50-52: Update the Returns section of the upload() docstrings in
mpt_api_client/resources/billing/custom_ledgers.py to reflect the new union
return type CustomLedger | ModelCollection[CustomLedger]; locate both
occurrences of the upload method (the one around the lines with signature "def
upload(self, custom_ledger_id: str, file: FileTypes) -> CustomLedger |
ModelCollection[CustomLedger]") and change the Returns text from only
"CustomLedger" to explicitly state that the method may return either a
CustomLedger instance or a ModelCollection[CustomLedger], including brief
descriptions of each case.

In `@mpt_api_client/resources/billing/journals.py`:
- Around line 50-52: Update the docstrings for the upload() methods to match the
widened return type: change the Returns section for the upload function(s)
(referenced as upload in journals.py) to indicate the method may return either a
Journal or a ModelCollection[Journal] (i.e., Journal | ModelCollection[Journal])
and describe the conditions when each type is returned; ensure both occurrences
(the one around the first upload definition and the second at lines referenced
in the review) are updated consistently.

In `@mpt_api_client/resources/notifications/batches.py`:
- Around line 41-43: Update the docstrings for get_attachment (and the other
method around lines 87–89 that has the same mismatch) to describe that the
return type can be either a single BatchAttachment or a
ModelCollection[BatchAttachment]; explicitly state the conditions under which a
collection is returned (e.g., when multiple attachments are matched or when
attachment_id is omitted/represents a query), include examples of both return
shapes and mention the types BatchAttachment and
ModelCollection[BatchAttachment] by name so the docs match the function
signature.
🪄 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: b208b83f-4050-410a-98c6-e86bb0340bfc

📥 Commits

Reviewing files that changed from the base of the PR and between 9f3cf9c and cbe78ff.

📒 Files selected for processing (12)
  • mpt_api_client/http/base_service.py
  • mpt_api_client/http/mixins/collection_mixin.py
  • mpt_api_client/http/resource_accessor.py
  • mpt_api_client/models/__init__.py
  • mpt_api_client/models/model.py
  • mpt_api_client/models/model_collection.py
  • mpt_api_client/resources/accounts/accounts_user_groups.py
  • mpt_api_client/resources/billing/custom_ledgers.py
  • mpt_api_client/resources/billing/journals.py
  • mpt_api_client/resources/notifications/batches.py
  • tests/unit/models/collection/conftest.py
  • tests/unit/models/test_model.py

Comment thread mpt_api_client/models/__init__.py
Comment thread mpt_api_client/models/model.py Outdated
Comment thread tests/unit/models/test_model.py Outdated
@jentyk jentyk force-pushed the fix/MPT-20245 branch 2 times, most recently from 301d5f9 to cf3b990 Compare April 16, 2026 09:39
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
mpt_api_client/resources/billing/journals.py (1)

50-60: Update the public docstrings to match the widened return type.

Both upload() methods now return Journal | ModelCollection[Journal], but the Returns sections still document only Journal. That drift will mislead client users.

📝 Suggested docstring update
         Returns:
-            Journal: Created resource.
+            Journal | ModelCollection[Journal]: Created resource(s).

Also applies to: 110-120

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mpt_api_client/resources/billing/journals.py` around lines 50 - 60, The
docstring for the upload() method(s) incorrectly documents the return type as
only Journal while the signature returns Journal | ModelCollection[Journal];
update the Returns section in the public docstrings for the upload methods
(reference: upload) to describe both possible return types and when each is
returned (e.g., a single Journal on single upload vs a ModelCollection[Journal]
for batch/multipart), and apply the same change to the second upload docstring
around the other occurrence (lines ~110-120) so the docs match the widened
return annotation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@mpt_api_client/resources/billing/journals.py`:
- Around line 50-60: The docstring for the upload() method(s) incorrectly
documents the return type as only Journal while the signature returns Journal |
ModelCollection[Journal]; update the Returns section in the public docstrings
for the upload methods (reference: upload) to describe both possible return
types and when each is returned (e.g., a single Journal on single upload vs a
ModelCollection[Journal] for batch/multipart), and apply the same change to the
second upload docstring around the other occurrence (lines ~110-120) so the docs
match the widened return annotation.

ℹ️ Review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro

Run ID: 2b7f5c60-0f3d-49ee-b993-f25c4ddc88cd

📥 Commits

Reviewing files that changed from the base of the PR and between cbe78ff and 301d5f9.

📒 Files selected for processing (8)
  • mpt_api_client/http/resource_accessor.py
  • mpt_api_client/models/model.py
  • mpt_api_client/models/model_collection.py
  • mpt_api_client/resources/accounts/accounts_user_groups.py
  • mpt_api_client/resources/billing/custom_ledgers.py
  • mpt_api_client/resources/billing/journals.py
  • mpt_api_client/resources/notifications/batches.py
  • tests/unit/models/test_model.py
🚧 Files skipped from review as they are similar to previous changes (6)
  • tests/unit/models/test_model.py
  • mpt_api_client/models/model_collection.py
  • mpt_api_client/http/resource_accessor.py
  • mpt_api_client/resources/accounts/accounts_user_groups.py
  • mpt_api_client/resources/notifications/batches.py
  • mpt_api_client/resources/billing/custom_ledgers.py

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
mpt_api_client/resources/billing/journals.py (1)

50-52: Add an upload test that exercises the new collection return.

The current upload tests in tests/unit/resources/billing/test_journals.py only assert result is not None, so they would not catch a regression in the new ModelCollection branch. A mocked list response with an isinstance(result, ModelCollection) assertion would pin down this widened contract.

Also applies to: 110-112

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mpt_api_client/resources/billing/journals.py` around lines 50 - 52, Add a
unit test in tests/unit/resources/billing/test_journals.py that specifically
exercises the upload method's branch that returns a ModelCollection: mock the
API/client response to return a list payload (not a single object), call
journals.upload(journal_id, file=...), and assert isinstance(result,
ModelCollection) (and optionally check element types are Journal). This ensures
the widened return type in upload (function upload in
mpt_api_client/resources/billing/journals.py) is covered and prevents
regressions from the collection-return branch.
mpt_api_client/resources/accounts/accounts_user_groups.py (1)

39-41: Add a list-response test for this custom update() path.

These methods now advertise AccountsUserGroup | ModelCollection[AccountsUserGroup], but the current tests in tests/unit/resources/accounts/test_accounts_user_groups.py still only cover dict responses and call .to_dict() on the result. A mocked PUT response with a JSON list would lock in the new contract end-to-end for both sync and async services.

Also applies to: 65-67

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mpt_api_client/resources/accounts/accounts_user_groups.py` around lines 39 -
41, Add a unit test that mocks the PUT response to return a JSON array and
asserts the custom update() path returns a ModelCollection[AccountsUserGroup]
(not a single dict) for both sync and async codepaths: update (the method at
lines with def update(self, resource_data: ResourceData) -> AccountsUserGroup |
ModelCollection[AccountsUserGroup]) and the async counterpart (the block noted
at 65-67). In tests/unit/resources/accounts/test_accounts_user_groups.py, create
a test that stubs the HTTP PUT to return a JSON list, calls the service's update
method (and the async equivalent), and verifies the result is a ModelCollection
containing AccountsUserGroup instances and that you do not call .to_dict() on
the result.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@tests/unit/models/test_model.py`:
- Line 11: The test imports ModelCollection from the private module
(mpt_api_client.models.model_collection); update the import to use the public
package export (from mpt_api_client.models import ModelCollection) so the test
exercises the exported API added by the PR and will fail if the package stops
re-exporting ModelCollection.

---

Nitpick comments:
In `@mpt_api_client/resources/accounts/accounts_user_groups.py`:
- Around line 39-41: Add a unit test that mocks the PUT response to return a
JSON array and asserts the custom update() path returns a
ModelCollection[AccountsUserGroup] (not a single dict) for both sync and async
codepaths: update (the method at lines with def update(self, resource_data:
ResourceData) -> AccountsUserGroup | ModelCollection[AccountsUserGroup]) and the
async counterpart (the block noted at 65-67). In
tests/unit/resources/accounts/test_accounts_user_groups.py, create a test that
stubs the HTTP PUT to return a JSON list, calls the service's update method (and
the async equivalent), and verifies the result is a ModelCollection containing
AccountsUserGroup instances and that you do not call .to_dict() on the result.

In `@mpt_api_client/resources/billing/journals.py`:
- Around line 50-52: Add a unit test in
tests/unit/resources/billing/test_journals.py that specifically exercises the
upload method's branch that returns a ModelCollection: mock the API/client
response to return a list payload (not a single object), call
journals.upload(journal_id, file=...), and assert isinstance(result,
ModelCollection) (and optionally check element types are Journal). This ensures
the widened return type in upload (function upload in
mpt_api_client/resources/billing/journals.py) is covered and prevents
regressions from the collection-return branch.
🪄 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: 1915aed1-b0cf-4976-b24d-5eca63c31b32

📥 Commits

Reviewing files that changed from the base of the PR and between 301d5f9 and cf3b990.

📒 Files selected for processing (8)
  • mpt_api_client/http/resource_accessor.py
  • mpt_api_client/models/model.py
  • mpt_api_client/models/model_collection.py
  • mpt_api_client/resources/accounts/accounts_user_groups.py
  • mpt_api_client/resources/billing/custom_ledgers.py
  • mpt_api_client/resources/billing/journals.py
  • mpt_api_client/resources/notifications/batches.py
  • tests/unit/models/test_model.py
✅ Files skipped from review due to trivial changes (1)
  • mpt_api_client/http/resource_accessor.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • mpt_api_client/models/model_collection.py
  • mpt_api_client/resources/billing/custom_ledgers.py
  • mpt_api_client/resources/notifications/batches.py

Comment thread tests/unit/models/test_model.py Outdated
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
mpt_api_client/resources/accounts/accounts_user_groups.py (1)

74-75: ⚠️ Potential issue | 🟡 Minor

Docstring does not match return type annotation.

The return type on line 68 is AccountsUserGroup | ModelCollection[AccountsUserGroup], but the docstring on line 75 only mentions AccountsUserGroup. The sync version's docstring (lines 48-49) was updated accordingly—this one should match.

Proposed fix
         Returns:
-            AccountsUserGroup: Updated Account User Group.
+            AccountsUserGroup | ModelCollection[AccountsUserGroup]: Updated Account User Group,
+            or a ModelCollection[AccountsUserGroup] when the service returns a list response.
         """
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mpt_api_client/resources/accounts/accounts_user_groups.py` around lines 74 -
75, The async method's docstring "Returns:" block does not match its type
annotation (AccountsUserGroup | ModelCollection[AccountsUserGroup]); update the
docstring's Returns section to list both possible return types and mirror the
sync version's wording/formatting so it documents that the method can return an
AccountsUserGroup or a ModelCollection[AccountsUserGroup], ensuring the text
appears under the method in accounts_user_groups.py that declares the return
type AccountsUserGroup | ModelCollection[AccountsUserGroup].
🧹 Nitpick comments (1)
mpt_api_client/http/resource_accessor.py (1)

60-88: Type-ignore comments hide a potential runtime type mismatch.

The public get(), post(), and put() methods declare -> ResourceModel but call _action() which can return ModelCollection[ResourceModel]. The # type: ignore[return-value] comments suppress the mismatch.

This is acceptable for backward compatibility, but callers expecting a single ResourceModel may receive a ModelCollection at runtime if the API returns a list. Consider documenting this behavior in the class docstring or adding a note that callers relying on list responses should use higher-level service methods (like AccountsUserGroupsService.update) that explicitly declare the union return type.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@mpt_api_client/http/resource_accessor.py` around lines 60 - 88, The public
methods get, post, and put declare -> ResourceModel but call _action which may
return ModelCollection[ResourceModel], and the current # type:
ignore[return-value] hides this mismatch; fix by updating the signatures of
get/post/put to return ResourceModel | ModelCollection[ResourceModel] (or the
appropriate union type used elsewhere) and remove the type: ignore comments, or
alternatively add a clear note in the class/docstring for ResourceAccessor
explaining that get/post/put may return either a ResourceModel or
ModelCollection[ResourceModel] and callers expecting lists should use the
higher-level service methods; reference get, post, put, _action, ResourceModel,
and ModelCollection[ResourceModel] when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@mpt_api_client/resources/accounts/accounts_user_groups.py`:
- Around line 74-75: The async method's docstring "Returns:" block does not
match its type annotation (AccountsUserGroup |
ModelCollection[AccountsUserGroup]); update the docstring's Returns section to
list both possible return types and mirror the sync version's wording/formatting
so it documents that the method can return an AccountsUserGroup or a
ModelCollection[AccountsUserGroup], ensuring the text appears under the method
in accounts_user_groups.py that declares the return type AccountsUserGroup |
ModelCollection[AccountsUserGroup].

---

Nitpick comments:
In `@mpt_api_client/http/resource_accessor.py`:
- Around line 60-88: The public methods get, post, and put declare ->
ResourceModel but call _action which may return ModelCollection[ResourceModel],
and the current # type: ignore[return-value] hides this mismatch; fix by
updating the signatures of get/post/put to return ResourceModel |
ModelCollection[ResourceModel] (or the appropriate union type used elsewhere)
and remove the type: ignore comments, or alternatively add a clear note in the
class/docstring for ResourceAccessor explaining that get/post/put may return
either a ResourceModel or ModelCollection[ResourceModel] and callers expecting
lists should use the higher-level service methods; reference get, post, put,
_action, ResourceModel, and ModelCollection[ResourceModel] when making the
change.

ℹ️ Review info
⚙️ Run configuration

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

Review profile: CHILL

Plan: Pro

Run ID: 110da513-c7ae-44d3-8f01-8a70a57143a9

📥 Commits

Reviewing files that changed from the base of the PR and between cf3b990 and a535be8.

📒 Files selected for processing (8)
  • mpt_api_client/http/resource_accessor.py
  • mpt_api_client/models/model.py
  • mpt_api_client/models/model_collection.py
  • mpt_api_client/resources/accounts/accounts_user_groups.py
  • mpt_api_client/resources/billing/custom_ledgers.py
  • mpt_api_client/resources/billing/journals.py
  • mpt_api_client/resources/notifications/batches.py
  • tests/unit/models/test_model.py
✅ Files skipped from review due to trivial changes (1)
  • tests/unit/models/test_model.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • mpt_api_client/models/model_collection.py
  • mpt_api_client/resources/notifications/batches.py
  • mpt_api_client/resources/billing/custom_ledgers.py

@sonarqubecloud
Copy link
Copy Markdown

@jentyk jentyk merged commit 78e264a into main Apr 16, 2026
4 checks passed
@jentyk jentyk deleted the fix/MPT-20245 branch April 16, 2026 11:09
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