MPT-16304 Ensure correct model is used in services #157
Conversation
albertsola
requested review from
alephsur,
d3rky,
jentyk,
robcsegal,
ruben-sebrango and
svazquezco
as code owners
WalkthroughThis pull request updates generic type parameters across multiple service classes in the mpt_api_client library, replacing the generic Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (3)
seed/accounts/module.py (1)
47-55: Unreachable warning suppression is reasonable; consider structural alternativeUsing
# type: ignore[unreachable]on the warning is consistent with the static type thatmodules[0]is alwaysModule, while still keeping a runtime safeguard if something goes wrong at runtime. If you want to avoid ignores, an alternative would be to replace theif isinstance(first_module, Module): ... else:pattern with anassert isinstance(first_module, Module)before using it, which most type checkers accept without raising unreachable warnings.seed/accounts/licensee.py (1)
81-94: Type-ignore for unreachable warning path is acceptableGiven that
mpt_client.accounts.licensees.createis now generically parameterized withLicensee, theisinstance(created, Licensee)guard is statically redundant and the warning branch is considered unreachable by type checkers. The# type: ignore[unreachable]keeps the defensive log + exception without fighting the type checker, which is fine here. As a minor alternative, anassert isinstance(created, Licensee)would both satisfy the type checker and still fail loudly if the API ever returned an unexpected type.seed/accounts/buyer.py (1)
71-83: Consistent unreachable warning suppression with other seed modulesHere as well, the
createdvalue is statically aBuyer, so the failure branch is unreachable to the type checker. Adding# type: ignore[unreachable]on the warning keeps runtime safety and aligns this file withlicensee.pyandmodule.py. Same optional refinement: anassert isinstance(created, Buyer)could remove the need for the ignore entirely.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
mpt_api_client/resources/accounts/account.py(2 hunks)mpt_api_client/resources/accounts/buyers.py(2 hunks)mpt_api_client/resources/accounts/licensees.py(2 hunks)mpt_api_client/resources/accounts/modules.py(1 hunks)seed/accounts/buyer.py(1 hunks)seed/accounts/licensee.py(1 hunks)seed/accounts/module.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
seed/accounts/licensee.py (1)
tests/e2e/conftest.py (1)
logger(63-64)
mpt_api_client/resources/accounts/modules.py (2)
mpt_api_client/http/mixins.py (2)
AsyncGetMixin(380-395)AsyncCollectionMixin(538-605)mpt_api_client/http/async_service.py (1)
AsyncService(12-80)
seed/accounts/module.py (1)
tests/e2e/conftest.py (1)
logger(63-64)
mpt_api_client/resources/accounts/buyers.py (1)
mpt_api_client/resources/accounts/mixins.py (6)
ActivatableMixin(4-27)EnablableMixin(30-53)ValidateMixin(56-68)AsyncActivatableMixin(134-159)AsyncEnablableMixin(162-185)AsyncValidateMixin(188-200)
mpt_api_client/resources/accounts/licensees.py (2)
mpt_api_client/http/mixins.py (2)
CreateFileMixin(115-144)UpdateFileMixin(147-184)mpt_api_client/resources/accounts/mixins.py (2)
EnablableMixin(30-53)AsyncEnablableMixin(162-185)
⏰ 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 comments (4)
mpt_api_client/resources/accounts/modules.py (1)
29-31: AsyncModulesService now correctly parameterized withModuleSwitching
AsyncCollectionMixinfromModeltoModulebrings the async service in line with the syncModulesServiceand ensures thatiterate,fetch_page, etc. are typed as handlingModuleobjects. This directly supports the PR goal of using the correct model type in services.mpt_api_client/resources/accounts/licensees.py (1)
35-44: Licensees services now fully specialized onLicenseeinstead of baseModelUpdating
CreateFileMixin,UpdateFileMixin, andEnablableMixin(and their async counterparts) to useLicenseetightens typing so all operations in these services consistently returnLicensee. This matches the config’s_model_class = Licenseeand avoids genericModelleakage in public APIs.Also applies to: 48-57
mpt_api_client/resources/accounts/account.py (1)
41-51: Accounts services correctly useAccountfor all mixin genericsChanging
CreateFileMixin,UpdateFileMixin,ActivatableMixin,EnablableMixin, andValidateMixin(and their async variants) to[...] [Account]removes the last uses of the genericModelin these service signatures. This makes the API surface more precise (everything is typed asAccount) without affecting runtime behavior.Also applies to: 61-71
mpt_api_client/resources/accounts/buyers.py (1)
40-51: Buyers services now consistently parameterized withBuyerUsing
Buyeras the type argument for all (async) file and status/validation mixins aligns with_model_class = Buyerand with the explicitBuyerreturn types ofsynchronizeandtransfer. This improves type safety for callers relying on these service classes.Also applies to: 73-84
|
d3rky
deleted the
MPT-16304-Ensure-the-correct-model-is-uses-on-Resource-Services
branch
| context.set_resource("accounts.module", first_module) | ||
| return first_module | ||
| logger.warning("First module is not a Module instance.") | ||
| logger.warning("First module is not a Module instance.") # type: ignore[unreachable] |
There was a problem hiding this comment.
Be careful with this kind of errors, we shouldn't ignore it since it mens the code after this point will never run. Did you check that the typing is correct? did you try using cast method?
I'd be great to improve a bit this logic using guard clauses or early returns, that way we avoid the spaguetti code and it's simpler to read and understand
Something like ->
"""Refresh module in context (always fetch)."""
module = await get_module(context=context, mpt_operations=mpt_operations)
if module:
return module
filtered_modules = mpt_operations.accounts.modules.filter(
RQLQuery(name="Access Management")
)
modules = [mod async for mod in filtered_modules.iterate()]
if not modules:
logger.warning("Module 'Access Management' not found.")
return None
first_module = modules[0]
if not isinstance(first_module, Module):
logger.warning("First module is not a Module instance.") # type: ignore[unreachable]
return None
context["accounts.module.id"] = first_module.id
context.set_resource("accounts.module", first_module)
return first_module
| logger.info("Licensee created: %s", created.id) | ||
| return created | ||
| logger.warning("Licensee creation failed") | ||
| logger.warning("Licensee creation failed") # type: ignore[unreachable] |
There was a problem hiding this comment.
Summary by CodeRabbit
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.