MPT-16846: implement fake option #21
Conversation
📝 WalkthroughWalkthroughAdds a --fake CLI option and ApplyMigrationUseCase to mark migrations applied without running them; moves detailed errors into manager/use-case modules; introduces FileStateManager and StateJSONEncoder for JSON state; refactors file migration loading to return migration instances; updates tests and exports. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@mpt_tool/cli.py`:
- Around line 74-83: The except block handling ApplyMigrationError should
suppress exception chaining when aborting; update the handler for
ApplyMigrationUseCase().execute (catching ApplyMigrationError) to call raise
typer.Abort from None instead of raise typer.Abort so the chain is not shown
(and keep the existing typer.secho error message before abort).
In `@mpt_tool/use_cases/apply_migration.py`:
- Around line 31-38: Currently the code unconditionally calls state.fake() and
self.state_manager.save_state(state), which overwrites applied_at for migrations
that already exist; change the flow so after retrieving state via
self.state_manager.get_by_id(migration_id) (and after creating it in the
StateNotFoundError branch), you only call state.fake() and
self.state_manager.save_state(state) when the state indicates it is not already
applied (e.g., state.applied_at is None or state.is_applied() is False). Keep
the StateNotFoundError branch that creates the new state via
self.file_migration_manager.load_migration(...) and self.state_manager.new(...),
but ensure existing applied states are treated as a no-op to preserve audit
timestamps.
- Around line 19-36: When creating a new state after StateNotFoundError, wrap
the call to file_migration_manager.load_migration so LoadMigrationError is
caught and translated into the use-case error contract: catch LoadMigrationError
and raise ApplyMigrationError(...) from the caught error (preserving the
original exception), or broaden the except to handle both StateNotFoundError and
LoadMigrationError; reference file_migration_manager.load_migration,
state_manager.new, LoadMigrationError and ApplyMigrationError so the load path
is explicitly wrapped and no LoadMigrationError escapes the ApplyMigration
use-case.
🧹 Nitpick comments (2)
mpt_tool/managers/file_migration.py (2)
44-44: Addisinstancevalidation forBaseCommandinload_migration.The function advertises a
BaseCommandreturn type but doesn't enforce it. While the template ensures new migrations inherit fromBaseCommandviaDataBaseCommandorSchemaBaseCommand, manually crafted or corrupted migration files could bypass this. Add a fast-fail guard to catch such cases immediately.Proposed safeguard
try: migration_instance = migration_module.Command() except (TypeError, AttributeError) as error: raise LoadMigrationError(f"Invalid migration Command: {error!s}") + if not isinstance(migration_instance, BaseCommand): + raise LoadMigrationError("Migration Command must extend BaseCommand.") + return migration_instance
35-42: Consider chaining exceptions to preserve stack traces for debugging.While B904 is already ignored in the project configuration, exception chaining with
from errorremains a best practice. It ensures the root cause is preserved in stack traces, making troubleshooting easier during development and debugging.🔧 Suggested change
try: spec.loader.exec_module(migration_module) except ImportError as error: - raise LoadMigrationError(f"Failed to import migration module: {error!s}") + raise LoadMigrationError( + f"Failed to import migration module: {error!s}" + ) from error try: migration_instance = migration_module.Command() except (TypeError, AttributeError) as error: - raise LoadMigrationError(f"Invalid migration Command: {error!s}") + raise LoadMigrationError( + f"Invalid migration Command: {error!s}" + ) from error
svazquezco
changed the title
svazquezco
changed the title
svazquezco
force-pushed
the
MPT-16846-implement-fake-option
branch
from
5f29430 to
bcb5ede
Compare
@coderabbitai run the pre-merge checks again and update the replied comment |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2256
Pre-merge checks - Updated Status✅ All checks passed (3 passed)
The previously failing check has been resolved! The PR is now ready for merge from a pre-merge checks perspective. 🎉 |
svazquezco
force-pushed
the
MPT-16846-implement-fake-option
branch
from
bcb5ede to
a719211
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@mpt_tool/cli.py`:
- Around line 64-68: The three exception handlers that currently do "raise
typer.Abort" should suppress context to satisfy B904; update each handler (the
one catching RunMigrationError after
RunMigrationsUseCase().execute(migration_type) and the two other similar
handlers in this module) to use "raise typer.Abort from None" instead of "raise
typer.Abort" so the original exception context isn't chained into the Abort
exception.
In `@mpt_tool/managers/file_migration.py`:
- Around line 34-42: The raise statements that convert ImportError and
(TypeError, AttributeError) into LoadMigrationError lose original tracebacks;
update the two raises in this block so they use exception chaining: when
re-raising after spec.loader.exec_module(migration_module) failure, append "from
error" to the LoadMigrationError, and likewise when constructing
migration_instance = migration_module.Command() fails, re-raise the
LoadMigrationError with "from error" (or use "from None" if you intentionally
want to suppress the original traceback) so the original exception context is
preserved or explicitly suppressed; modify the raise expressions referencing
LoadMigrationError, spec.loader.exec_module, and migration_module.Command
accordingly.
In `@mpt_tool/managers/file_state.py`:
- Around line 16-23: The load() method should handle malformed JSON from
cls._state_path: wrap the json.loads(...) call in a try/except that catches
json.JSONDecodeError as e and raise a clearer error (e.g., ValueError or
RuntimeError) that includes the state-file path (cls._state_path) and the
original exception message (use "from e" to preserve the cause), so callers get
a helpful message instead of a raw JSONDecodeError; keep returning {} only when
the file is missing, not when JSON is invalid.
In `@tests/test_cli.py`:
- Around line 132-134: The test function
test_migrate_skip_migration_already_applied declares fixtures applied_migration
and data_migration_file in its signature but does not use them; change this to
use pytest.mark.usefixtures so their side effects are explicit: remove
applied_migration and data_migration_file from the parameter list of
test_migrate_skip_migration_already_applied and add
`@pytest.mark.usefixtures`("applied_migration", "data_migration_file") above the
function (ensure pytest is imported if not already) while leaving other fixtures
like migration_state_file, runner, and log as function args.
♻️ Duplicate comments (2)
mpt_tool/use_cases/apply_migration.py (1)
31-36: Wrapload_migrationto preserve the use‑case error contract.
LoadMigrationErrorcan still leak pastApplyMigrationErrorhere; the TODO indicates this is known but unresolved.🐛 Proposed fix
-from mpt_tool.managers.errors import MigrationFolderError, StateNotFoundError +from mpt_tool.managers.errors import LoadMigrationError, MigrationFolderError, StateNotFoundError @@ - except StateNotFoundError: - # TODO: handle LoadMigrationError exception - migration = self.file_migration_manager.load_migration(migration_file) - state = self.state_manager.new(migration_id, migration.type, migration_file.order_id) + except StateNotFoundError: + try: + migration = self.file_migration_manager.load_migration(migration_file) + except LoadMigrationError as error: + raise ApplyMigrationError(str(error)) from error + state = self.state_manager.new(migration_id, migration.type, migration_file.order_id)mpt_tool/cli.py (1)
76-79: Applyraise typer.Abort from Nonehere as well.This mirrors the previous feedback for the fake‑migration path; the handler still chains exceptions. Based on learnings, suppress the chain after printing the error.
🔧 Proposed fix
except ApplyMigrationError as error: typer.secho(f"Error running migration: {error!s}", fg=typer.colors.RED) - raise typer.Abort + raise typer.Abort from None
🧹 Nitpick comments (2)
mpt_tool/managers/file_state.py (2)
29-32: Add exception chaining withfrom None.When re-raising
StateNotFoundError, usefrom Noneto explicitly suppress the exception chain, which is consistent with the intentional suppression pattern.Suggested fix
try: state = state_data[migration_id] except KeyError: - raise StateNotFoundError("State not found") from None + raise StateNotFoundError(f"State not found: {migration_id}") from NoneIncluding the
migration_idin the error message would help with debugging.
56-61: Consider documenting the non-atomic save behavior.
save_state()performs a load-modify-save operation that is not atomic. While this is likely acceptable for a CLI tool, it could lead to lost updates if multiple processes modify the state file concurrently.
svazquezco
force-pushed
the
MPT-16846-implement-fake-option
branch
2 times, most recently
from
9819ede to
bc8071f
Compare
svazquezco
force-pushed
the
MPT-16846-implement-fake-option
branch
from
bc8071f to
40042b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
mpt_tool/managers/file_migration.py (1)
6-44: Add type guard and cast to fixno-any-returnviolation inload_migration.At line 44, returning
migration_instancefrom a method typed to returnBaseCommandviolates mypy'sno-any-returnrule (strict mode enabled). The dynamically loadedmigration_module.Command()is untyped and mypy infers it asAny.Add an
isinstancecheck for runtime type safety and usecast()to satisfy the type checker:🔧 Proposed fix
-from importlib.util import module_from_spec, spec_from_file_location +from importlib.util import module_from_spec, spec_from_file_location +from typing import cast @@ - try: - migration_instance = migration_module.Command() - except (TypeError, AttributeError) as error: - raise LoadMigrationError(f"Invalid migration Command: {error!s}") from error + try: + migration_instance = migration_module.Command() + except (TypeError, AttributeError) as error: + raise LoadMigrationError(f"Invalid migration Command: {error!s}") from error + + if not isinstance(migration_instance, BaseCommand): + raise LoadMigrationError( + "Invalid migration Command: must inherit BaseCommand" + ) - return migration_instance + return cast(BaseCommand, migration_instance)
🤖 Fix all issues with AI agents
In `@mpt_tool/models.py`:
- Around line 54-57: The fake() method lacks a return type annotation; update
its signature to include "-> None" (matching the adjacent start() method's
style) so it reads like: def fake(self) -> None: and keep the body unchanged;
this satisfies mypy strict mode and linters while preserving behavior of the
fake() method.
In `@mpt_tool/use_cases/apply_migration.py`:
- Line 41: The Migration.fake() method is missing an explicit return type which
triggers the no-untyped-call CI error; open the Migration class in
mpt_tool/models.py and add the return type annotation "-> None" to the
fake(self) method signature (matching applied(), failed(), start()), ensuring
the method signature reads like other methods and that no other behavioral
changes are made so callers such as use_cases/apply_migration.py continue to
type-check.
♻️ Duplicate comments (3)
mpt_tool/use_cases/apply_migration.py (1)
31-36: WrapLoadMigrationErrorto preserve the use‑case error contract.
load_migrationcan raiseLoadMigrationError, which currently bypassesApplyMigrationErrorand the CLI handler.🔧 Proposed fix
-from mpt_tool.managers.errors import MigrationFolderError, StateNotFoundError +from mpt_tool.managers.errors import LoadMigrationError, MigrationFolderError, StateNotFoundError @@ - except StateNotFoundError: - # TODO: handle LoadMigrationError exception - migration = self.file_migration_manager.load_migration(migration_file) - state = self.state_manager.new(migration_id, migration.type, migration_file.order_id) + except StateNotFoundError: + try: + migration = self.file_migration_manager.load_migration(migration_file) + except LoadMigrationError as error: + raise ApplyMigrationError(str(error)) from error + state = self.state_manager.new(migration_id, migration.type, migration_file.order_id)mpt_tool/cli.py (1)
65-69: Useraise typer.Abort from Noneinside except blocks.
This satisfies B904 and keeps the user-facing message clean.🔧 Proposed fix
except RunMigrationError as error: typer.secho(f"Error running migrations: {error!s}", fg=typer.colors.RED) - raise typer.Abort + raise typer.Abort from None @@ except ApplyMigrationError as error: typer.secho(f"Error running migration: {error!s}", fg=typer.colors.RED) - raise typer.Abort + raise typer.Abort from NoneBased on learnings, apply the
from Nonepattern fortyper.Abort.Also applies to: 75-79
tests/test_cli.py (1)
132-134: Use@pytest.mark.usefixturesfor side‑effect‑only fixtures.
These fixtures are not referenced in the test bodies; declare them explicitly to avoid unused-arg warnings.🔧 Proposed fix
`@freeze_time`("2025-04-06 13:00:00") -def test_migrate_skip_migration_already_applied( - applied_migration, migration_state_file, runner, log -): +@pytest.mark.usefixtures("applied_migration") +def test_migrate_skip_migration_already_applied(migration_state_file, runner, log): @@ `@freeze_time`("2025-04-06 10:11:24") -def test_migrate_fake(data_migration_file, migration_state_file, runner): +@pytest.mark.usefixtures("data_migration_file") +def test_migrate_fake(migration_state_file, runner):As per coding guidelines, keep fixtures explicit and avoid unused parameters in tests.
Also applies to: 188-189
svazquezco
force-pushed
the
MPT-16846-implement-fake-option
branch
from
40042b8 to
175b728
Compare
|
Closes MPT-16846