MPT-20245 Allow for array of objects in the response#301
Conversation
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaced Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
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 | 🟠 MajorWiden the public return types to match
_action()'s actual union return.The
_action()methods (lines 102 and 201) declare return typeResourceModel | ModelCollection[ResourceModel], but the publicget(),post(), andput()methods (lines 66, 76, 86, 165, 175, 185) declare onlyResourceModel. 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 inmpt_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 returnJournal | ModelCollection[Journal], but theReturnssection still describes onlyJournal.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: UpdateReturnsdocstrings to match the new union return type.Both
upload()methods now returnCustomLedger | ModelCollection[CustomLedger], but docstrings still document onlyCustomLedger. 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 returnsAccountsUserGroup | ModelCollection[AccountsUserGroup]in both sync/async services, so theReturnssection 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: Updateget_attachment()docstrings to reflect possible collection responses.The signature now allows
BatchAttachment | ModelCollection[BatchAttachment], but the current docs still imply only a singleBatchAttachment.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
📒 Files selected for processing (12)
mpt_api_client/http/base_service.pympt_api_client/http/mixins/collection_mixin.pympt_api_client/http/resource_accessor.pympt_api_client/models/__init__.pympt_api_client/models/model.pympt_api_client/models/model_collection.pympt_api_client/resources/accounts/accounts_user_groups.pympt_api_client/resources/billing/custom_ledgers.pympt_api_client/resources/billing/journals.pympt_api_client/resources/notifications/batches.pytests/unit/models/collection/conftest.pytests/unit/models/test_model.py
301d5f9 to
cf3b990
Compare
There was a problem hiding this comment.
🧹 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 returnJournal | ModelCollection[Journal], but theReturnssections still document onlyJournal. 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
📒 Files selected for processing (8)
mpt_api_client/http/resource_accessor.pympt_api_client/models/model.pympt_api_client/models/model_collection.pympt_api_client/resources/accounts/accounts_user_groups.pympt_api_client/resources/billing/custom_ledgers.pympt_api_client/resources/billing/journals.pympt_api_client/resources/notifications/batches.pytests/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
There was a problem hiding this comment.
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.pyonly assertresult is not None, so they would not catch a regression in the newModelCollectionbranch. A mocked list response with anisinstance(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 customupdate()path.These methods now advertise
AccountsUserGroup | ModelCollection[AccountsUserGroup], but the current tests intests/unit/resources/accounts/test_accounts_user_groups.pystill 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
📒 Files selected for processing (8)
mpt_api_client/http/resource_accessor.pympt_api_client/models/model.pympt_api_client/models/model_collection.pympt_api_client/resources/accounts/accounts_user_groups.pympt_api_client/resources/billing/custom_ledgers.pympt_api_client/resources/billing/journals.pympt_api_client/resources/notifications/batches.pytests/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
There was a problem hiding this comment.
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 | 🟡 MinorDocstring does not match return type annotation.
The return type on line 68 is
AccountsUserGroup | ModelCollection[AccountsUserGroup], but the docstring on line 75 only mentionsAccountsUserGroup. 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(), andput()methods declare-> ResourceModelbut call_action()which can returnModelCollection[ResourceModel]. The# type: ignore[return-value]comments suppress the mismatch.This is acceptable for backward compatibility, but callers expecting a single
ResourceModelmay receive aModelCollectionat 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 (likeAccountsUserGroupsService.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
📒 Files selected for processing (8)
mpt_api_client/http/resource_accessor.pympt_api_client/models/model.pympt_api_client/models/model_collection.pympt_api_client/resources/accounts/accounts_user_groups.pympt_api_client/resources/billing/custom_ledgers.pympt_api_client/resources/billing/journals.pympt_api_client/resources/notifications/batches.pytests/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
|



Closes MPT-20245