Conversation
svazquezco
requested review from
albertsola and
ruben-sebrango
and removed request for
a team
📝 WalkthroughWalkthroughAdds environment config and Docker env loading; introduces a StateManager abstraction with local and Airtable implementations and factory; refactors file-based state manager to the new interface; updates use cases to use the factory; adds config helpers and mixins changes; expands documentation and adds comprehensive CLI tests while removing an old test module. 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 |
svazquezco
force-pushed
the
MPT-16657-refactor
branch
2 times, most recently
from
dc6f786 to
6cb0f3d
Compare
svazquezco
force-pushed
the
MPT-16667-add-airtable-storage
branch
from
b9385e8 to
374190e
Compare
svazquezco
force-pushed
the
MPT-16667-add-airtable-storage
branch
2 times, most recently
from
1ca8386 to
af3bf41
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@mpt_tool/managers/state/airtable.py`:
- Line 4: Import the formula helper and replace any direct f-string construction
of Airtable formulas with pyairtable.formulas.match to safely escape values and
prevent formula injection; specifically add "from pyairtable.formulas import
match" and use match(field_name, value) instead of f-string interpolation in the
places that build filter/formula strings (the formula constructions around the
current f-strings at ~lines 49-51 and 79-81), e.g. replace constructions like
f\"{{Field}}='{value}'\" with match("Field", value) and pass the resulting
formula to the airtable call.
In `@mpt_tool/managers/state/file.py`:
- Around line 42-51: The new() classmethod currently calls Migration.from_dict
with a partial dict missing required keys (started_at, applied_at) and passes
migration_type as an enum rather than a string, which will raise at runtime; fix
by constructing a proper Migration instance (or supply the missing keys) before
calling cls.save_state: use the Migration constructor or ensure the dict
contains "started_at": None, "applied_at": None and "type": migration_type.value
(or str(...) as expected) so Migration.from_dict receives all required fields;
update the new() method to create a valid Migration object and then call
cls.save_state(new_state).
🧹 Nitpick comments (3)
mpt_tool/managers/state/factory.py (2)
27-31: Consider validating invalidSTORAGE_TYPEvalues.If
STORAGE_TYPEis set to an unsupported value (e.g.,"redis"), the code silently falls back toFileStateManager. This could cause confusion during debugging.♻️ Suggested improvement
`@classmethod` def get_instance(cls) -> StateManager: - storage_type = os.getenv("STORAGE_TYPE", StorageTypeEnum.LOCAL) + storage_type = os.getenv("STORAGE_TYPE", StorageTypeEnum.LOCAL).lower() + if storage_type not in (StorageTypeEnum.LOCAL, StorageTypeEnum.AIRTABLE): + raise ValueError( + f"Invalid STORAGE_TYPE '{storage_type}'. " + f"Must be one of: {', '.join(StorageTypeEnum)}" + ) return ( AirtableStateManager() if storage_type == StorageTypeEnum.AIRTABLE else FileStateManager() )
19-31: Design note: Factory returns instances butStateManageruses class methods.The
StateManagerinterface defines all methods as@classmethod, so the returned instances are effectively stateless. The caller would usemanager.load()which calls the classmethod. This works but is unconventional—typically either:
- Use instance methods with state, or
- Return the class type itself (
type[StateManager])The current approach functions correctly, so this is just a design observation for future consideration.
docs/PROJECT_DESCRIPTION.md (1)
38-44: Consider adding cross-reference for Airtable-specific variables.The main Environment Variables section lists
AIRTABLE_API_KEYbut notSTORAGE_AIRTABLE_BASE_IDandSTORAGE_AIRTABLE_TABLE_NAME. While these are documented in the Airtable Storage subsection, a brief mention or cross-reference here would improve discoverability.
svazquezco
force-pushed
the
MPT-16667-add-airtable-storage
branch
from
af3bf41 to
5f67e49
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/PROJECT_DESCRIPTION.md (1)
99-99: Inconsistent class naming:CommandvsMigration.This line mentions "a prefilled
Commandclass" but the code examples at lines 111 and 125 showclass Migration(...). Similarly, line 253 references "Command.run()" but should now be "Migration.run()".Consider updating these references for consistency with the code examples.
📝 Suggested fix
Line 99:
-A new file is created in `migrations/` with a timestamped prefix (e.g., `20260113180013_migration_name.py`) and a prefilled `Command` class. +A new file is created in `migrations/` with a timestamped prefix (e.g., `20260113180013_migration_name.py`) and a prefilled `Migration` class.Line 253:
-- Check your `Command.run()` implementation for syntax errors +- Check your `Migration.run()` implementation for syntax errors
♻️ Duplicate comments (3)
mpt_tool/managers/state/airtable.py (2)
46-59: Use pyairtable formula helpers to prevent injection.Direct f-string interpolation in the formula parameter (
f"migration_id = '{migration_id}'") is unsafe ifmigration_idcontains quotes or special characters. Usepyairtable.formulas.matchwhich properly escapes values.🔒 Proposed fix
-from pyairtable.orm import Model, fields +from pyairtable.orm import Model, fields +from pyairtable.formulas import match # In get_by_id method: - state = MigrationStateModel.first(formula=f"migration_id = '{migration_id}'") + state = MigrationStateModel.first(formula=match({"migration_id": migration_id}))
76-96: Same formula injection issue insave_state.The
formula=f"migration_id = '{state.migration_id}'"on line 80 has the same injection vulnerability asget_by_id. Apply the same fix usingmatch.🔒 Proposed fix
migration_state_model = MigrationStateModel.first( - formula=f"migration_id = '{state.migration_id}'" + formula=match({"migration_id": state.migration_id}) )mpt_tool/managers/state/file.py (1)
44-51: Critical:Migration.from_dictwill fail at runtime.The dict passed to
Migration.from_dictis missing required keys (started_at,applied_at) and passesmigration_typeas an enum instead of a string. Looking atmpt_tool/models.py,from_dictexpects all keys and callsMigrationTypeEnum(migration_data["type"]), which will fail if passed an enum directly.🐛 Proposed fix - use Migration constructor directly
`@override` `@classmethod` def new(cls, migration_id: str, migration_type: MigrationTypeEnum, order_id: int) -> Migration: - new_state = Migration.from_dict({ - "migration_id": migration_id, - "order_id": order_id, - "type": migration_type, - }) + new_state = Migration( + migration_id=migration_id, + order_id=order_id, + type=migration_type, + ) cls.save_state(new_state) return new_state
🧹 Nitpick comments (3)
mpt_tool/managers/state/airtable.py (1)
35-41: Consider extracting a helper to reduce Migration construction duplication.The same
Migration(...)construction pattern appears inload,get_by_id, andnew. A private helper method like_to_migration(state: MigrationStateModel) -> Migrationwould reduce duplication.♻️ Suggested refactor
`@staticmethod` def _to_migration(state: MigrationStateModel) -> Migration: return Migration( migration_id=state.migration_id, order_id=state.order_id, type=MigrationTypeEnum(state.type), started_at=state.started_at, applied_at=state.applied_at, )Also applies to: 53-58, 68-73
mpt_tool/managers/state/factory.py (2)
19-32: Type inconsistency: mixing string and enum in comparison.
os.getenv("STORAGE_TYPE", StorageTypeEnum.LOCAL)returns a string when the env var is set, but an enum when using the default. The comparison on line 30 works due toStrEnum, but this is inconsistent. Consider using a string default for clarity.♻️ Proposed fix
`@classmethod` def get_instance(cls) -> StateManager: - storage_type = os.getenv("STORAGE_TYPE", StorageTypeEnum.LOCAL) + storage_type = os.getenv("STORAGE_TYPE", StorageTypeEnum.LOCAL.value) return ( AirtableStateManager() if storage_type == StorageTypeEnum.AIRTABLE else FileStateManager() )Alternatively, convert the env var to the enum:
storage_type = StorageTypeEnum(os.getenv("STORAGE_TYPE", StorageTypeEnum.LOCAL.value))This would also provide validation that the env var contains a valid storage type.
28-31: Instantiating class-based managers with all@classmethodmethods.Both
AirtableStateManagerandFileStateManageruse@classmethodfor all their methods (as required by theStateManagerABC). Instantiating them (AirtableStateManager(),FileStateManager()) works but is semantically unusual - the instances have no instance state. Consider returning the class itself (type[StateManager]) instead, or refactoring the managers to use instance methods.This is a design observation for future consideration - the current approach works correctly.
svazquezco
force-pushed
the
MPT-16667-add-airtable-storage
branch
2 times, most recently
from
e4e2d06 to
d24b0ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/PROJECT_DESCRIPTION.md (1)
182-191: Started_at semantics look inconsistent with runtime behavior.Docs say
started_atis recorded on failure, but the current behavior intentionally avoids persistingstarted_atbefore/while running to allow clean retries after crashes. Please align this section with actual behavior.Based on learnings, please ensure the docs match the current persistence semantics.📝 Suggested wording update
- If a migration fails during execution: - * The started_at timestamp is recorded - * The applied_at field remains null - * The error is logged - * Later runs will retry the failed migration as applied_at is null, unless `--fake` is used to mark it as applied + If a migration fails during execution: + * The applied_at field remains null + * The error is logged + * Later runs will retry the failed migration as applied_at is null, unless `--fake` is used to mark it as applied + * started_at is not persisted before execution, so crashes may leave it null
🤖 Fix all issues with AI agents
In `@docs/PROJECT_DESCRIPTION.md`:
- Around line 40-82: The docs reference the deprecated AIRTABLE_API_KEY; replace
it with guidance to use an Airtable Personal Access Token (PAT) and update all
env var mentions (e.g., AIRTABLE_API_KEY) to a clear name like AIRTABLE_PAT or
AIRTABLE_PERSONAL_ACCESS_TOKEN, update the env var list and the "Airtable
Storage" section to instruct how to create a PAT in Airtable (granting access to
the target base/table), and keep STORAGE_AIRTABLE_BASE_ID and
STORAGE_AIRTABLE_TABLE_NAME as-is while noting the required PAT scopes/access
needed to read/write the listed columns (order_id, migration_id, started_at,
applied_at, type).
- Around line 5-18: Update the Quick Start section which currently references a
non-existent PyPI package and CLI name: replace the pip install command "pip
install mpt-tool" and all uses of the CLI "mpt-tool" (examples: "mpt-tool
migrate --new-data sync_users" and "mpt-tool migrate --data") with the verified
package name and CLI executable (e.g., "mpt", "mpt-cli", or the actual package
you confirm). Ensure the install line and both example migrate commands use the
same correct package/command name so they are accurate and consistent in the
Quick Start header and the three listed steps.
In `@mpt_tool/managers/state/airtable.py`:
- Around line 22-25: The Meta class currently reads AIRTABLE_* env vars at class
definition time causing import-time failures; change Meta.api_key, Meta.base_id,
and Meta.table_name to be `@staticmethod` callables that return os.getenv(...) so
pyairtable resolves them lazily (locate the Meta class in this module and update
those three attributes). Also replace the f-string formula construction that
uses state.migration_id (the line constructing "migration_id =
'{state.migration_id}'") with the pyairtable match(...) approach used elsewhere
(use match(state.migration_id) or the same match(...) helper call as on line 50)
to avoid formula injection when AirtableStateManager performs the lookup.
In `@tests/cli/test_cli_local_storage.py`:
- Around line 17-30: The write_text call in the applied_migration fixture omits
the encoding parameter; update the migration_state_file.write_text invocation in
the applied_migration fixture to include encoding="utf-8" (same pattern used
elsewhere) so the JSON is written with consistent UTF-8 encoding when writing
applied_state_data for migration_state_file; reference the applied_migration
fixture and migration_state_file.write_text to locate and change the call.
♻️ Duplicate comments (1)
mpt_tool/managers/state/airtable.py (1)
79-81: Usematch()to escape formula inputs and prevent injection.This was flagged in a previous review. Line 50 was updated to use
match(), but line 81 still uses f-string interpolation which is vulnerable to formula injection ifstate.migration_idcontains quotes or special characters.🔒 Proposed fix
`@override` `@classmethod` def save_state(cls, state: Migration) -> None: migration_state_model = MigrationStateModel.first( - formula=f"migration_id = '{state.migration_id}'" + formula=match({"migration_id": state.migration_id}) )
🧹 Nitpick comments (3)
tests/cli/test_cli_airtable_storage.py (1)
118-129: Consider usingpytest.mark.usefixturesconsistently for clarity.The test uses
@pytest.mark.usefixtures("applied_migration", "schema_migration_file")which correctly documents the fixture dependencies. However, the assertion at line 127-128 relies on truncated output (e.g.,fake_data_file…). If the table formatting changes, this test could become brittle.Consider extracting key fields to verify rather than matching the entire formatted row:
assert "fake_data_file_name" in result.output assert "20250406020202" in result.outputmpt_tool/managers/state/factory.py (1)
27-31: Consider validating the storage type and using consistent types.
os.getenvreturns a string when the environment variable is set, but the default is an enum member. While StrEnum comparison works, an invalidSTORAGE_TYPEvalue (e.g.,"invalid") silently falls through toFileStateManagerwithout warning.♻️ Suggested improvement with validation
`@classmethod` def get_instance(cls) -> StateManager: - storage_type = os.getenv("STORAGE_TYPE", StorageTypeEnum.LOCAL) - return ( - AirtableStateManager() - if storage_type == StorageTypeEnum.AIRTABLE - else FileStateManager() - ) + storage_type_value = os.getenv("STORAGE_TYPE", StorageTypeEnum.LOCAL.value) + try: + storage_type = StorageTypeEnum(storage_type_value) + except ValueError: + raise ValueError( + f"Invalid STORAGE_TYPE '{storage_type_value}'. " + f"Must be one of: {', '.join(e.value for e in StorageTypeEnum)}" + ) + if storage_type == StorageTypeEnum.AIRTABLE: + return AirtableStateManager() + return FileStateManager()README.md (1)
20-23: Add a brief note to keep.envout of version control.📝 Suggested doc tweak
- - Copy .env.sample to .env + - Copy .env.sample to .env + - Keep .env out of version control (it contains secrets)
svazquezco
force-pushed
the
MPT-16667-add-airtable-storage
branch
2 times, most recently
from
38de970 to
477c3e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/cli/test_cli_local_storage.py`:
- Around line 33-36: The test function test_migrate_data_migration currently
declares fixtures data_migration_file, schema_migration_file, and
migration_state_file as parameters but only uses them for side effects, which
triggers Ruff ARG001; remove those fixtures from the function signature and
instead annotate the test with pytest.mark.usefixtures("data_migration_file",
"schema_migration_file", "migration_state_file") (preserve the existing
`@freeze_time` decorator), and apply the same replacement for the other tests
flagged (the tests at the other indicated ranges) so side-effect-only fixtures
are declared via pytest.mark.usefixtures rather than as unused parameters.
♻️ Duplicate comments (4)
mpt_tool/managers/state/airtable.py (1)
91-93: Formula injection vulnerability: usematch()consistently.Line 92 uses f-string interpolation for the formula, which is vulnerable to injection if
state.migration_idcontains quotes or special characters. Line 61 correctly usesmatch()- apply the same pattern here.🐛 Proposed fix
def save_state(cls, state: Migration) -> None: migration_state_model = MigrationStateModel.first( - formula=f"migration_id = '{state.migration_id}'" + formula=match({"migration_id": state.migration_id}) )docs/PROJECT_DESCRIPTION.md (2)
5-18: Verify the PyPI package/CLI name in Quick Start (still showsmpt-tool).This was flagged previously and still appears unchanged. Please confirm the correct package name and CLI executable, then update Quick Start (and the Installation section for consistency) to match.
What is the correct PyPI package name and CLI executable for the SoftwareOne MPT migration tool? Check whether “mpt-tool” exists on PyPI and what CLI name it installs.
40-65: Confirm Airtable auth variable name (AIRTABLE_API_KEY) is still valid.This was flagged previously and remains in both the env var list and Airtable storage config. Please verify current Airtable auth guidance and update to the correct credential type/name (and required scopes) if needed.
Airtable authentication current guidance: Are API keys still supported, or should docs use Personal Access Tokens (PATs)? What are the recommended env var naming and required scopes?tests/cli/test_cli_local_storage.py (1)
17-28: Add encoding when writing the JSON state file.This was flagged previously and is still missing. Keep write_text calls consistent with other fixtures.
🔧 Proposed fix
- migration_state_file.write_text(data=json.dumps(applied_state_data)) + migration_state_file.write_text( + data=json.dumps(applied_state_data), + encoding="utf-8", + )
🧹 Nitpick comments (3)
compose.yaml (1)
6-7: Consider making the .env file optional to avoid startup failures.If
.envdoesn't exist, Docker Compose will fail. You can make it optional with therequired: falseattribute to improve DX for local development without Airtable.💡 Suggested improvement
env_file: - - .env + - path: .env + required: falsempt_tool/config.py (1)
4-11: Consider caching the config dict or validating required keys.The config dict is rebuilt on every call. While minor, this could be optimized. More importantly, returning
Nonefor missing required config (api_key,base_id) may mask configuration errors until Airtable operations fail at runtime.💡 Optional: Add validation for required config
_REQUIRED_KEYS = {"api_key", "base_id"} def get_airtable_config(config_key: str) -> str | None: """Get Airtable configuration.""" config = { "api_key": os.getenv("AIRTABLE_API_KEY"), "base_id": os.getenv("STORAGE_AIRTABLE_BASE_ID"), "table_name": os.getenv("STORAGE_AIRTABLE_TABLE_NAME", "Migrations"), } value = config.get(config_key) if value is None and config_key in _REQUIRED_KEYS: # Could log a warning here for debugging pass return valuempt_tool/managers/state/airtable.py (1)
44-56: Consider extracting a helper to reduce duplication.The
Migrationconstruction fromMigrationStateModelis repeated inload,get_by_id, andnew. A private helper would reduce duplication.♻️ Optional refactor
`@classmethod` def _to_migration(cls, state: MigrationStateModel) -> Migration: return Migration( migration_id=state.migration_id, order_id=state.order_id, type=MigrationTypeEnum(state.type), started_at=state.started_at, applied_at=state.applied_at, )Then use
cls._to_migration(state)inload,get_by_id, andnew.Also applies to: 60-71
svazquezco
force-pushed
the
MPT-16667-add-airtable-storage
branch
from
477c3e7 to
49b441b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/PROJECT_DESCRIPTION.md (1)
182-190: Alignstarted_atsemantics with intended behavior.
Docs statestarted_atis recorded on both success and failure, but the intended behavior is to avoid persistingstarted_atbefore or during execution so failed runs are retried from scratch. Please clarify/update this section to reflect the actual state tracking behavior.Based on learnings, ...
🤖 Fix all issues with AI agents
In `@tests/cli/test_cli_local_storage.py`:
- Around line 75-77: The test test_migrate_data_run_script_fail currently
accepts the data_migration_file_error fixture as a parameter only for its side
effect, causing a Ruff ARG001; change the test to use
pytest.mark.usefixtures("data_migration_file_error") (keep the existing
`@freeze_time` decorator) and remove data_migration_file_error from the
test_migrate_data_run_script_fail function signature so only
migration_state_file, runner, log remain as parameters.
♻️ Duplicate comments (5)
docs/PROJECT_DESCRIPTION.md (2)
5-18: Quick Start still referencesmpt-toolpackage/CLI — verify actual PyPI name and executable.
This appears unchanged from the prior review. Please confirm the correct package/CLI name and update the install and example commands accordingly.What is the correct PyPI package name and CLI executable for the SoftwareOne Marketplace Platform migration tool (mpt-tool)? Please confirm the install command and CLI name from official sources.Also applies to: 24-30
40-65: Airtable auth still documents deprecated API keys — switch to PAT guidance.
This is the same issue noted previously. Update docs to recommend Personal Access Tokens and revise env var naming accordingly.What does the current Airtable API documentation recommend for authentication (PAT vs API key), and what are the required scopes for read/write access to base/table records?mpt_tool/managers/state/airtable.py (1)
90-93: Usematch()for thesave_statelookup as well.
The f-string formula bypasses escaping and can break on quotes or allow formula injection.🔒 Proposed fix
- migration_state_model = MigrationStateModel.first( - formula=f"migration_id = '{state.migration_id}'" - ) + migration_state_model = MigrationStateModel.first( + formula=match({"migration_id": state.migration_id}) + )tests/cli/test_cli_local_storage.py (2)
122-124: Use@pytest.mark.usefixturesfor side-effect fixture.The
migration_state_fileparameter is only used for its side effect and triggers Ruff ARG001.🔧 Proposed fix
`@pytest.mark.usefixtures`("applied_migration", "schema_migration_file") -def test_migrate_list(migration_state_file, runner, log): +@pytest.mark.usefixtures("applied_migration", "schema_migration_file", "migration_state_file") +def test_migrate_list(runner, log):
93-95: Use@pytest.mark.usefixturesfor side-effect fixture.The
data_migration_fileparameter is only used for its side effect and triggers Ruff ARG001.🔧 Proposed fix
`@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):
🧹 Nitpick comments (3)
tests/cli/test_cli_airtable_storage.py (1)
16-19: Prefer@pytest.mark.usefixturesfor setup-only fixtures.
Several fixtures are injected but not referenced, which obscures intent and can trigger unused-arg linting. Consider switching tousefixtures(or dropping the args) where the fixture is only needed for setup.♻️ Suggested cleanup
`@pytest.fixture` -def mock_airtable(monkeypatch): +def mock_airtable(): with MockAirtable() as mock: yield mock `@freeze_time`("2025-04-06 13:00:00") `@pytest.mark.usefixtures`("applied_migration") -def test_migrate_skip_migration_already_applied(mock_airtable, runner, log): +def test_migrate_skip_migration_already_applied(runner, log): `@freeze_time`("2025-04-06 13:00:00") -def test_migrate_data_run_script_fail(data_migration_file_error, mock_airtable, runner, log): +@pytest.mark.usefixtures("data_migration_file_error") +def test_migrate_data_run_script_fail(mock_airtable, runner, log): `@freeze_time`("2025-04-06 10:11:24") -def test_migrate_fake(data_migration_file, mock_airtable, runner): +@pytest.mark.usefixtures("data_migration_file") +def test_migrate_fake(mock_airtable, runner): `@pytest.mark.usefixtures`("applied_migration") -def test_migrate_fake_migration_already_applied(mock_airtable, runner): +def test_migrate_fake_migration_already_applied(runner):Also applies to: 63-74, 91-93, 109-111
mpt_tool/config.py (1)
4-17: Consider exposing config values directly or as typed dataclasses.The current pattern builds a dictionary on every call just to retrieve a single value. This works but has minor inefficiency and lacks type safety at call sites. Consider either:
- Exposing individual getter functions (
get_airtable_api_key(),get_airtable_base_id(), etc.)- Using a dataclass/TypedDict for structured config
This would also provide better IDE autocompletion for callers instead of magic string keys.
mpt_tool/managers/state/factory.py (1)
27-32: Invalid storage type silently falls back to local storage.If
STORAGE_TYPEis set to an invalid value (e.g., typo like"airtabel"), the factory silently defaults toFileStateManager. This could lead to confusion when users expect Airtable storage but get local storage instead.Consider adding validation or logging:
♻️ Proposed approach
`@classmethod` def get_instance(cls) -> StateManager: storage_type = get_storage_type() - return ( - AirtableStateManager() - if storage_type == StorageTypeEnum.AIRTABLE - else FileStateManager() - ) + if storage_type == StorageTypeEnum.AIRTABLE: + return AirtableStateManager() + if storage_type == StorageTypeEnum.LOCAL: + return FileStateManager() + raise ValueError( + f"Invalid STORAGE_TYPE '{storage_type}'. " + f"Valid options: {', '.join(e.value for e in StorageTypeEnum)}" + )
svazquezco
force-pushed
the
MPT-16667-add-airtable-storage
branch
2 times, most recently
from
8d5f42b to
b9b3d5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/PROJECT_DESCRIPTION.md`:
- Around line 253-254: The troubleshooting text incorrectly references
Command.run() instead of the actual generated class/method name Migration.run();
update the docs to replace the Command.run() mention with Migration.run() so the
guidance matches the codebase and generated migration class structure (ensure
any surrounding text still makes sense after the rename).
In `@tests/cli/test_cli_local_storage.py`:
- Around line 94-111: The test_migrate_fake function currently takes the
data_migration_file fixture only for its side effects but doesn't use it; remove
data_migration_file from the test_migrate_fake signature and annotate the test
with `@pytest.mark.usefixtures`("data_migration_file") so the fixture still runs
for setup; keep other fixtures (migration_state_file, runner) as parameters and
leave assertions and freeze_time decorator unchanged.
- Around line 123-134: The test function test_migrate_list has an unused
parameter migration_state_file; remove migration_state_file from the test
signature so the function becomes def test_migrate_list(runner, log) while
keeping the existing fixtures marker
`@pytest.mark.usefixtures`("applied_migration", "schema_migration_file") intact;
update any references if present elsewhere (none expected) and run tests to
confirm behavior remains unchanged.
♻️ Duplicate comments (2)
docs/PROJECT_DESCRIPTION.md (1)
44-44: Consider clarifying that AIRTABLE_API_KEY should be a Personal Access Token (PAT).Since Airtable deprecated user API keys (disabled February 2024), the credential stored in
AIRTABLE_API_KEYmust be a Personal Access Token. Consider adding a note or renaming the variable toAIRTABLE_PATto avoid confusion.mpt_tool/managers/state/airtable.py (1)
91-93: Formula injection vulnerability remains insave_state.Line 92 still uses an f-string to construct the Airtable formula, which is vulnerable to injection if
state.migration_idcontains quotes or special characters. Usematch()consistently, as done inget_by_idon line 61.🔒 Proposed fix
`@classmethod` def save_state(cls, state: Migration) -> None: migration_state_model = MigrationStateModel.first( - formula=f"migration_id = '{state.migration_id}'" + formula=match({"migration_id": state.migration_id}) )
🧹 Nitpick comments (4)
mpt_tool/managers/state/airtable.py (1)
23-36: Remove unusednoqa: WPS602directives.Ruff does not recognize WPS602 (a wemake-python-styleguide rule). These comments have no effect and can be removed.
♻️ Proposed cleanup
`@staticmethod` - def api_key() -> str | None: # noqa: WPS602 + def api_key() -> str | None: """Airtable API key.""" return get_airtable_config("api_key") `@staticmethod` - def base_id() -> str | None: # noqa: WPS602 + def base_id() -> str | None: """Airtable base ID.""" return get_airtable_config("base_id") `@staticmethod` - def table_name() -> str | None: # noqa: WPS602 + def table_name() -> str | None: """Airtable table name.""" return get_airtable_config("table_name")tests/cli/test_cli_airtable_storage.py (1)
16-19: Remove unusedmonkeypatchparameter from fixture.The
mock_airtablefixture doesn't use themonkeypatchparameter. Remove it to silence the linter warning.♻️ Proposed fix
`@pytest.fixture` -def mock_airtable(monkeypatch): +def mock_airtable(): with MockAirtable() as mock: yield mockcompose.yaml (1)
6-7: Therequired: falsesyntax requires Docker Compose v2.24.0 or later.The suggested refactor is technically correct—Docker Compose supports
required: falseto gracefully handle missing env files. However, this feature was introduced in v2.24.0, and the project doesn't document a minimum Docker Compose version requirement. If you want to add this enhancement, ensure the project specifies this version constraint in its documentation or.dockerignore/setup guide.If not enforcing a version, the current
- .envapproach is acceptable since.env.exampleexists and setup documentation can instruct developers to copy it.mpt_tool/managers/state/factory.py (1)
1-32: Normalize storage type to avoid case-sensitive misconfig.Lowercasing the env value prevents accidental fallback to local when users set
AIRTABLE.♻️ Suggested tweak
- storage_type = get_storage_type() + storage_type = get_storage_type().lower() return ( AirtableStateManager() - if storage_type == StorageTypeEnum.AIRTABLE + if storage_type == StorageTypeEnum.AIRTABLE.value else FileStateManager() )
svazquezco
force-pushed
the
MPT-16667-add-airtable-storage
branch
2 times, most recently
from
ce3d989 to
e0f0a5c
Compare
svazquezco
force-pushed
the
MPT-16667-add-airtable-storage
branch
from
e0f0a5c to
3c38690
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/PROJECT_DESCRIPTION.md (1)
99-100: Docs still mentionCommandinstead ofMigration.
This line contradicts the updated generated file snippet below.✏️ Proposed fix
-A new file is created in `migrations/` with a timestamped prefix (e.g., `20260113180013_migration_name.py`) and a prefilled `Command` class. +A new file is created in `migrations/` with a timestamped prefix (e.g., `20260113180013_migration_name.py`) and a prefilled `Migration` class.
🤖 Fix all issues with AI agents
In `@mpt_tool/managers/state/factory.py`:
- Around line 3-6: The module has a circular import because factory.py imports
StateManager from mpt_tool.managers while mpt_tool.managers imports
StateManagerFactory from this file; remove the top-level import of StateManager
and either import it lazily inside the factory function that constructs managers
or guard the import with typing.TYPE_CHECKING (e.g., add "from typing import
TYPE_CHECKING" and under if TYPE_CHECKING: from mpt_tool.managers import
StateManager) so runtime import order is not affected; keep the existing imports
of get_storage_type, AirtableStateManager and FileStateManager and update any
type annotations in functions like the factory constructor or
create_state_manager to use the lazy/local import or string annotations to avoid
importing mpt_tool.managers at module import time.
♻️ Duplicate comments (3)
docs/PROJECT_DESCRIPTION.md (2)
5-18: Confirm the correct PyPI package/CLI name.
The docs still instructpip install mpt-toolandmpt-tool .... Please verify the actual published package/command and update both the Quick Start and Installation snippets to match.Is there a PyPI package named "mpt-tool", and what is the correct package/CLI name for this project?Also applies to: 24-30
40-65: Validate Airtable credential guidance.
The docs still refer toAIRTABLE_API_KEY. Please confirm the current Airtable authentication method and update variable names/instructions if API keys are deprecated in favor of PATs.What is Airtable's current recommended API authentication method, and are legacy API keys deprecated?mpt_tool/managers/state/airtable.py (1)
88-108: Usematch()for formula construction to prevent injection.Line 92 uses f-string interpolation for the formula, which is inconsistent with line 61 and vulnerable to formula injection if
state.migration_idcontains quotes or special characters.🔒 Proposed fix
`@override` `@classmethod` def save_state(cls, state: Migration) -> None: migration_state_model = MigrationStateModel.first( - formula=f"migration_id = '{state.migration_id}'" + formula=match({"migration_id": state.migration_id}) )
🧹 Nitpick comments (4)
mpt_tool/managers/state/factory.py (1)
27-31: Normalize/validateSTORAGE_TYPEto avoid silent fallbacks.
Right now any unexpected value silently falls back to local storage. Consider normalizing and validating to fail fast on misconfiguration.♻️ Proposed refactor
- storage_type = get_storage_type() + try: + storage_type = StorageTypeEnum(get_storage_type().lower()) + except ValueError as exc: + raise ValueError("Unsupported STORAGE_TYPE; use 'local' or 'airtable'.") from excmpt_tool/use_cases/apply_migration.py (1)
31-36: Address the TODO forLoadMigrationErrorhandling.The TODO at line 34 indicates that
LoadMigrationErrorexceptions fromload_migrationare not being caught. Ifload_migrationfails, the exception will propagate unhandled rather than being wrapped inApplyMigrationErrorlike other errors in this method.Would you like me to help implement the exception handling, or should I open an issue to track this task?
mpt_tool/managers/state/airtable.py (1)
22-36: Remove unusednoqa: WPS602directives.The static analysis tool indicates these
noqadirectives reference an unknown ruleWPS602. Since Ruff doesn't recognize this rule, the directives are ineffective and should be removed for cleaner code.♻️ Proposed fix
class Meta: `@staticmethod` - def api_key() -> str | None: # noqa: WPS602 + def api_key() -> str | None: """Airtable API key.""" return get_airtable_config("api_key") `@staticmethod` - def base_id() -> str | None: # noqa: WPS602 + def base_id() -> str | None: """Airtable base ID.""" return get_airtable_config("base_id") `@staticmethod` - def table_name() -> str | None: # noqa: WPS602 + def table_name() -> str | None: """Airtable table name.""" return get_airtable_config("table_name")tests/cli/test_cli_airtable_storage.py (1)
121-132: Consider extracting repeated assertion patterns.The whitespace-stripped output parsing (line 127) and partial string matching for table rows is somewhat fragile. If the table formatting changes slightly, these assertions will break.
Consider using a helper or more robust parsing if the table format is expected to be stable, or document the expected output format more explicitly.
2 participants
Closes MPT-16667