Fix issue #42: Add GeneralApi, related models, examples, tests. #48
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
WalkthroughAdds General API surface to the client with sub-APIs for accounts, account accesses, billing, and permissions. Introduces corresponding resource wrappers and typed models. Provides example scripts for each area and unit tests covering happy paths and error handling. Updates package exports for new parameter types. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer Code
participant MC as MailtrapClient
participant GA as GeneralApi
participant RA as Resource API (Accounts/Accesses/Billing/Permissions)
participant HC as HttpClient
participant S as Mailtrap General API
Dev->>MC: instantiate(token)
Dev->>MC: general_api
MC-->>Dev: GeneralApi(GA)
Note over GA: Access sub-APIs via properties
Dev->>GA: .accounts /.account_accesses /.billing /.permissions
GA-->>Dev: RA (specific resource API)
rect rgba(200,235,255,0.25)
Note right of RA: Read operations
Dev->>RA: get_list() / get_resources() / get_current_billing_usage()
RA->>HC: GET /api/...
HC->>S: HTTP GET
S-->>HC: 200 JSON or error
HC-->>RA: Response
RA-->>Dev: Typed model(s)
end
rect rgba(220,255,220,0.25)
Note right of RA: Write operations
Dev->>RA: delete(...) / bulk_permissions_update(...)
RA->>HC: DELETE / PUT /api/...
HC->>S: HTTP DELETE/PUT
S-->>HC: 200 JSON or error
HC-->>RA: Response
RA-->>Dev: DeletedObject / UpdatePermissionsResponse
end
alt Error (401/403/404)
S-->>HC: Error JSON
HC-->>RA: Raise APIError
RA-->>Dev: Exception
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
examples/general/billing.py (1)
16-16: Acknowledge: Logging sensitive data in example code.Static analysis correctly identifies that billing usage data may contain sensitive information. In production code, avoid logging or printing billing details directly. For example purposes, this is acceptable with placeholder credentials, but users should be cautioned.
🧹 Nitpick comments (8)
mailtrap/api/resources/billing.py (1)
9-12: Consider validating account_id before making the API request.The method accepts any integer value for
account_id, including negative numbers or zero, which are likely invalid. Adding a validation check could prevent unnecessary API calls with invalid identifiers.Apply this diff to add input validation:
def get_current_billing_usage(self, account_id: int) -> BillingCycleUsage: """Get current billing cycle usage for Email Testing and Email Sending.""" + if account_id <= 0: + raise ValueError(f"account_id must be positive, got {account_id}") response = self._client.get(f"/api/accounts/{account_id}/billing/usage") return BillingCycleUsage(**response)mailtrap/models/billing.py (3)
6-9: Consider adding field validation for MessagesCount.The model allows any integer values for
currentandlimit, including negative numbers or cases wherecurrentexceedslimit. Adding validation could catch data inconsistencies early.Apply this diff to add validation:
+from pydantic import field_validator + @dataclass class MessagesCount: current: int limit: int + + @field_validator('current', 'limit') + @classmethod + def check_non_negative(cls, v: int) -> int: + if v < 0: + raise ValueError('must be non-negative') + return v + + def __post_init__(self): + if self.current > self.limit: + raise ValueError(f'current ({self.current}) exceeds limit ({self.limit})')
40-43: Consider validating billing cycle date ordering.The model allows
cycle_startto be equal to or aftercycle_end, which likely represents invalid billing data. Adding validation could prevent data inconsistencies.Apply this diff to add validation:
@dataclass class Billing: cycle_start: datetime cycle_end: datetime + + def __post_init__(self): + if self.cycle_start >= self.cycle_end: + raise ValueError( + f'cycle_start ({self.cycle_start}) must be before cycle_end ({self.cycle_end})' + )
46-50: Consider making billing models immutable.The dataclasses are mutable by default, which could lead to accidental modifications of API response objects. Since these models represent read-only billing data from the API, making them immutable with
frozen=Truewould better reflect their intended usage.Apply this pattern to all dataclasses in the file:
-@dataclass +@dataclass(frozen=True) class BillingCycleUsage: billing: Billing testing: Testing sending: SendingThis change should be applied to all eight dataclasses:
MessagesCount,UsageTesting,UsageSending,Plan,Testing,Sending,Billing, andBillingCycleUsage.examples/general/accounts.py (1)
1-11: Prefer environment variables for the API token.
Hard-coding a placeholder invites copy/paste of real tokens into source. Even in examples we should model secure habits by reading the value from the environment and failing fast if it is missing.+import os import mailtrap as mt from mailtrap.models.accounts import Account -API_TOKEN = "YOUR_API_TOKEN" - -client = mt.MailtrapClient(token=API_TOKEN) +client = mt.MailtrapClient(token=os.environ["MAILTRAP_API_TOKEN"])tests/unit/api/general/test_permissions.py (1)
185-195: Assert the bulk update payload.
Without inspectingresponses.calls[0].request.body, a regression that drops or renames the"permissions"payload would pass unnoticed. Decode the recorded JSON and compare it to thesample_permission_paramsexpectation to lock in the request contract.result = client.bulk_permissions_update( ACCOUNT_ID, ACCOUNT_ACCESS_ID, sample_permission_params ) + assert len(responses.calls) == 1 + payload = json.loads(responses.calls[0].request.body) + assert payload == { + "permissions": [ + params.api_data for params in sample_permission_params + ] + } + assert isinstance(result, UpdatePermissionsResponse) assert result.message == "Permissions have been updated!"(Don’t forget to add
import jsonat the top of the module.)mailtrap/api/general.py (1)
12-26: Consider caching sub-API instances for efficiency.Each property access creates a new API instance. While this works correctly, repeated access (e.g.,
client.general_api.permissions.get_resources(...)followed byclient.general_api.permissions.bulk_permissions_update(...)) creates multiple identical instances.If you want to optimize, apply this diff to cache the instances:
class GeneralApi: def __init__(self, client: HttpClient) -> None: self._client = client + self._accounts: Optional[AccountsApi] = None + self._account_accesses: Optional[AccountAccessesApi] = None + self._billing: Optional[BillingApi] = None + self._permissions: Optional[PermissionsApi] = None @property def accounts(self) -> AccountsApi: - return AccountsApi(client=self._client) + if self._accounts is None: + self._accounts = AccountsApi(client=self._client) + return self._accounts @property def account_accesses(self) -> AccountAccessesApi: - return AccountAccessesApi(client=self._client) + if self._account_accesses is None: + self._account_accesses = AccountAccessesApi(client=self._client) + return self._account_accesses @property def billing(self) -> BillingApi: - return BillingApi(client=self._client) + if self._billing is None: + self._billing = BillingApi(client=self._client) + return self._billing @property def permissions(self) -> PermissionsApi: - return PermissionsApi(client=self._client) + if self._permissions is None: + self._permissions = PermissionsApi(client=self._client) + return self._permissionsAdd at the top of the file:
from typing import Optionalmailtrap/api/resources/permissions.py (1)
20-39: LGTM! Bulk update implementation is correct.The method properly constructs the URL, serializes the permissions array using
api_data, and returns the typed response.Optional: Consider validating that the
permissionslist is not empty to provide a clearer error message:def bulk_permissions_update( self, account_id: int, account_access_id: int, permissions: list[PermissionResourceParams], ) -> UpdatePermissionsResponse: """ Manage user or token permissions. For this endpoint, you should send an array of objects (in JSON format) as the body of the request. If you send a combination of resource_type and resource_id that already exists, the permission is updated. If the combination doesn't exist, the permission is created. """ + if not permissions: + raise ValueError("permissions list cannot be empty") response = self._client.put( f"/api/accounts/{account_id}" f"/account_accesses/{account_access_id}" "/permissions/bulk", json={"permissions": [resource.api_data for resource in permissions]}, ) return UpdatePermissionsResponse(**response)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
examples/general/account_accesses.py(1 hunks)examples/general/accounts.py(1 hunks)examples/general/billing.py(1 hunks)examples/general/permissions.py(1 hunks)mailtrap/__init__.py(2 hunks)mailtrap/api/general.py(1 hunks)mailtrap/api/resources/account_accesses.py(1 hunks)mailtrap/api/resources/accounts.py(1 hunks)mailtrap/api/resources/billing.py(1 hunks)mailtrap/api/resources/permissions.py(1 hunks)mailtrap/client.py(2 hunks)mailtrap/models/accounts.py(1 hunks)mailtrap/models/billing.py(1 hunks)mailtrap/models/permissions.py(1 hunks)tests/unit/api/general/test_account_accesses.py(1 hunks)tests/unit/api/general/test_accounts.py(1 hunks)tests/unit/api/general/test_billing.py(1 hunks)tests/unit/api/general/test_permissions.py(1 hunks)tests/unit/models/test_permissions.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (19)
tests/unit/api/general/test_billing.py (4)
mailtrap/api/resources/billing.py (2)
BillingApi(5-12)get_current_billing_usage(9-12)mailtrap/exceptions.py (1)
APIError(10-15)mailtrap/http.py (1)
HttpClient(14-106)mailtrap/models/billing.py (1)
BillingCycleUsage(47-50)
mailtrap/__init__.py (2)
mailtrap/models/accounts.py (1)
AccountAccessFilterParams(8-11)mailtrap/models/permissions.py (1)
PermissionResourceParams(26-30)
mailtrap/api/resources/billing.py (3)
mailtrap/http.py (1)
HttpClient(14-106)mailtrap/api/general.py (1)
billing(21-22)mailtrap/models/billing.py (1)
BillingCycleUsage(47-50)
mailtrap/models/accounts.py (2)
mailtrap/models/common.py (1)
RequestParams(13-19)mailtrap/models/permissions.py (1)
Permissions(9-13)
mailtrap/api/general.py (5)
mailtrap/api/resources/account_accesses.py (1)
AccountAccessesApi(10-43)mailtrap/api/resources/accounts.py (1)
AccountsApi(5-12)mailtrap/api/resources/billing.py (1)
BillingApi(5-12)mailtrap/api/resources/permissions.py (1)
PermissionsApi(7-39)mailtrap/http.py (1)
HttpClient(14-106)
examples/general/accounts.py (3)
mailtrap/api/general.py (1)
accounts(13-14)mailtrap/models/accounts.py (1)
Account(15-18)mailtrap/api/resources/accounts.py (1)
get_list(9-12)
mailtrap/api/resources/permissions.py (3)
mailtrap/http.py (2)
HttpClient(14-106)put(36-38)mailtrap/api/general.py (1)
permissions(25-26)mailtrap/models/permissions.py (3)
PermissionResource(17-22)PermissionResourceParams(26-30)UpdatePermissionsResponse(34-35)
examples/general/permissions.py (3)
mailtrap/api/general.py (1)
permissions(25-26)mailtrap/models/permissions.py (3)
PermissionResource(17-22)UpdatePermissionsResponse(34-35)PermissionResourceParams(26-30)mailtrap/api/resources/permissions.py (2)
get_resources(11-18)bulk_permissions_update(20-39)
mailtrap/api/resources/account_accesses.py (4)
mailtrap/http.py (1)
HttpClient(14-106)mailtrap/models/accounts.py (2)
AccountAccess(37-42)AccountAccessFilterParams(8-11)mailtrap/models/common.py (1)
DeletedObject(23-24)mailtrap/api/resources/accounts.py (1)
get_list(9-12)
tests/unit/api/general/test_accounts.py (5)
mailtrap/api/general.py (1)
accounts(13-14)mailtrap/api/resources/accounts.py (2)
AccountsApi(5-12)get_list(9-12)mailtrap/exceptions.py (1)
APIError(10-15)mailtrap/http.py (1)
HttpClient(14-106)mailtrap/models/accounts.py (1)
Account(15-18)
examples/general/account_accesses.py (5)
mailtrap/api/general.py (2)
accounts(13-14)account_accesses(17-18)mailtrap/models/accounts.py (1)
AccountAccess(37-42)mailtrap/models/common.py (1)
DeletedObject(23-24)mailtrap/client.py (2)
MailtrapClient(31-170)general_api(58-61)mailtrap/api/resources/account_accesses.py (2)
get_list(14-27)delete(29-37)
mailtrap/api/resources/accounts.py (4)
mailtrap/http.py (1)
HttpClient(14-106)mailtrap/api/general.py (1)
accounts(13-14)mailtrap/models/accounts.py (1)
Account(15-18)mailtrap/api/resources/account_accesses.py (1)
get_list(14-27)
tests/unit/models/test_permissions.py (2)
mailtrap/api/general.py (1)
permissions(25-26)mailtrap/models/permissions.py (1)
PermissionResourceParams(26-30)
mailtrap/models/permissions.py (1)
mailtrap/models/common.py (1)
RequestParams(13-19)
examples/general/billing.py (4)
mailtrap/api/general.py (1)
billing(21-22)mailtrap/models/billing.py (1)
BillingCycleUsage(47-50)mailtrap/client.py (2)
MailtrapClient(31-170)general_api(58-61)mailtrap/api/resources/billing.py (1)
get_current_billing_usage(9-12)
tests/unit/api/general/test_account_accesses.py (5)
mailtrap/api/resources/account_accesses.py (3)
AccountAccessesApi(10-43)get_list(14-27)delete(29-37)mailtrap/exceptions.py (1)
APIError(10-15)mailtrap/http.py (1)
HttpClient(14-106)mailtrap/models/accounts.py (2)
AccountAccess(37-42)AccountAccessFilterParams(8-11)mailtrap/models/common.py (1)
DeletedObject(23-24)
mailtrap/models/billing.py (1)
mailtrap/api/general.py (1)
billing(21-22)
tests/unit/api/general/test_permissions.py (4)
mailtrap/api/resources/permissions.py (3)
PermissionsApi(7-39)get_resources(11-18)bulk_permissions_update(20-39)mailtrap/exceptions.py (1)
APIError(10-15)mailtrap/http.py (2)
HttpClient(14-106)put(36-38)mailtrap/models/permissions.py (3)
PermissionResource(17-22)PermissionResourceParams(26-30)UpdatePermissionsResponse(34-35)
mailtrap/client.py (2)
mailtrap/api/general.py (1)
GeneralApi(8-26)mailtrap/http.py (1)
HttpClient(14-106)
🪛 GitHub Check: CodeQL
examples/general/billing.py
[failure] 16-16: Clear-text logging of sensitive information
This expression logs sensitive data (private) as clear text.
🪛 Ruff (0.13.1)
examples/general/accounts.py
4-4: Possible hardcoded password assigned to: "API_TOKEN"
(S105)
examples/general/permissions.py
5-5: Possible hardcoded password assigned to: "API_TOKEN"
(S105)
examples/general/account_accesses.py
5-5: Possible hardcoded password assigned to: "API_TOKEN"
(S105)
examples/general/billing.py
4-4: Possible hardcoded password assigned to: "API_TOKEN"
(S105)
🔇 Additional comments (25)
tests/unit/models/test_permissions.py (2)
4-17: LGTM! Test correctly verifies complete dict serialization.The test validates that
api_dataincludes all fields when provided, including the_destroyfield.
19-24: LGTM! Test correctly verifies optional field exclusion.The test confirms that
api_dataexcludes optional fields (access_leveland_destroy) when not provided, returning only required fields.mailtrap/client.py (1)
57-61: LGTM! Property follows established pattern.The
general_apiproperty correctly instantiatesGeneralApiwith a dedicatedHttpClientpointing toGENERAL_HOST, consistent with other API properties in this class.mailtrap/__init__.py (2)
8-8: LGTM! Export extends public API appropriately.Adding
AccountAccessFilterParamsto the public API allows users to construct filter parameters for account access queries.
28-28: LGTM! Export extends public API appropriately.Adding
PermissionResourceParamsto the public API allows users to construct permission resource parameters for permission update operations.mailtrap/api/resources/accounts.py (2)
5-7: LGTM! Constructor follows established pattern.The
__init__method correctly stores theHttpClientinstance for use in API calls, consistent with other resource API classes.
9-12: LGTM! Method correctly retrieves and parses account list.The
get_listmethod properly:
- Calls the accounts endpoint
- Unpacks response items into
Accountinstances- Includes appropriate docstring
tests/unit/api/general/test_billing.py (4)
17-19: LGTM! Fixture correctly instantiates BillingApi.The client fixture creates a
BillingApiinstance withHttpClientconfigured forGENERAL_HOST, appropriate for unit testing.
22-40: LGTM! Fixture provides comprehensive billing data.The
sample_billing_usage_dictfixture includes nested structure covering billing cycle, testing plan/usage, and sending plan/usage, appropriate for validating model parsing.
45-82: LGTM! Parameterized test correctly validates error handling.The test verifies that
get_current_billing_usageproperly propagatesAPIErrorfor unauthorized (401), forbidden (403), and not-found (404) responses.
84-104: LGTM! Test correctly validates successful billing usage retrieval.The test verifies:
- Correct parsing into
BillingCycleUsageinstance- Nested field access for testing and sending usage metrics
- All expected numeric values match the fixture data
tests/unit/api/general/test_account_accesses.py (1)
94-133: Nice coverage on filtered list retrieval.
The assertions aroundresponses.calls[0].request.urlmake it hard for query-param regressions to slip through, which is exactly what we need here.examples/general/permissions.py (3)
1-9: LGTM! Clean import structure and client initialization.The imports are organized well, and the client setup follows the expected pattern for the new GeneralApi surface.
12-13: LGTM! Simple delegation wrapper for demonstration purposes.
16-25: LGTM! Clear demonstration of bulk permissions update.The function signature and delegation are appropriate for an example script.
examples/general/account_accesses.py (3)
1-9: LGTM! Clean structure and initialization.The imports and client setup are correct and follow the established pattern.
12-13: LGTM! Simple wrapper for API demonstration.
16-19: LGTM! Clear demonstration of deletion.mailtrap/api/general.py (1)
1-10: LGTM! Clean aggregator initialization.The imports are well-organized and the constructor correctly stores the shared
HttpClientfor delegation to sub-APIs.mailtrap/models/permissions.py (4)
1-5: LGTM! Imports are clean and appropriate.
16-22: LGTM! Well-structured recursive model.The
PermissionResourcedataclass correctly uses forward references for the recursiveresourcesfield. Pydantic handles this pattern well.
25-30: Verify the_destroyfield name matches the API specification.The
_destroyfield with a leading underscore is unusual for a public parameter. Ensure this matches the actual API endpoint specification. If the API expectsdestroywithout the underscore, add an alias:@dataclass class PermissionResourceParams(RequestParams): resource_id: str resource_type: str access_level: Optional[str] = None - _destroy: Optional[bool] = None + destroy: Optional[bool] = Field(default=None, alias="_destroy")Also note:
access_levelisOptional[str]in params butintinPermissionResource. This appears intentional (string input accepts level names, int output is the resolved level), but verify this aligns with the API contract.
33-35: LGTM! Simple response model.mailtrap/api/resources/permissions.py (2)
1-9: LGTM! Clean initialization.The imports and constructor follow the established pattern for resource API classes.
11-18: LGTM! Correct implementation of resource retrieval.The method correctly constructs the API path and maps the response to typed models.
Ihor-Bilous
requested review from
IgorDobryn,
VladimirTaytor,
andrii-porokhnavets,
i7an and
yanchuk
3 participants
Motivation
In this PR I added GeneralApi, related models, tests, examples
Changes
How to test
Summary by CodeRabbit