[Refactor] SQLAlchemy outbox model.#66
Merged
vadikko2 merged 7 commits intopypatterns:masterfrom Mar 5, 2026
Merged
Conversation
…greSQL and MySQL compatibility.
Contributor
Merging this PR will not alter performance
Comparing 
Footnotes
|
Contributor
📝 WalkthroughWalkthroughMigrates OutboxModel to SQLAlchemy 2.x declarative mappings with a BinaryUUID TypeDecorator, guards SQLAlchemy imports with a clear ImportError, and updates pyproject.toml to add Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 (2)
src/cqrs/outbox/sqlalchemy.py (2)
128-133:⚠️ Potential issue | 🟡 MinorType mismatch:
default={}for abytescolumn.The
payloadcolumn is typed asMapped[bytes]withLargeBinary, but the default is{}(an empty dict). This should be a bytes value.🐛 Proposed fix
payload: Mapped[bytes] = mapped_column( sqlalchemy.LargeBinary, nullable=False, - default={}, + default=b"{}", comment="Event payload", )Alternatively, since
add()always provides a value, consider removing the default entirely if the column should never be empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cqrs/outbox/sqlalchemy.py` around lines 128 - 133, The payload column is declared as Mapped[bytes] with sqlalchemy.LargeBinary but has a default of {} (a dict) causing a type mismatch; update the mapped_column for payload (symbol: payload, mapped_column, Mapped[bytes], sqlalchemy.LargeBinary) to either remove the default entirely if add() always supplies a value, or change the default to a bytes value (e.g., an empty bytes literal) so the default matches the declared bytes type.
218-227:⚠️ Potential issue | 🔴 Critical
UUID_TO_BINis MySQL-specific and breaks PostgreSQL compatibility.
func.UUID_TO_BIN()is a MySQL-only function. This contradicts the PR's goal of PostgreSQL/MySQL compatibility viaBinaryUUID.Consider converting the UUID to bytes in Python instead:
🐛 Proposed fix
self.session.add( OutboxModel( event_id=event.event_id, - event_id_bin=func.UUID_TO_BIN(event.event_id), + event_id_bin=event.event_id.bytes, event_name=event.event_name, created_at=event.event_timestamp, payload=bytes_payload, topic=event.topic, ), )This works for both PostgreSQL and MySQL since
uuid.UUID.bytesreturns the 16-byte representation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cqrs/outbox/sqlalchemy.py` around lines 218 - 227, The Outbox insertion uses the MySQL-only func.UUID_TO_BIN; change it to store the 16-byte UUID in Python for cross-DB compatibility: in the session.add call that constructs OutboxModel (look for OutboxModel and the event_id_bin field), replace func.UUID_TO_BIN(event.event_id) with the UUID's 16-byte representation (e.g. event.event_id.bytes or uuid.UUID(str(event.event_id)).bytes if event.event_id might be a string) so event_id_bin is assigned raw bytes compatible with BinaryUUID for both PostgreSQL and MySQL.
🧹 Nitpick comments (1)
src/cqrs/outbox/sqlalchemy.py (1)
89-98: Potential redundancy betweenevent_idandevent_id_bin.With
BinaryUUIDnow handling the binary conversion transparently, the separateevent_id_bincolumn storingBINARY(16)appears redundant. Both columns store the same UUID—event_idviaBinaryUUID(which usesBINARY(16)on MySQL) andevent_id_binas raw bytes.Consider whether
event_id_binis still necessary or if it exists solely for the unique constraint index. If it's only for indexing, note that you could indexevent_iddirectly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/cqrs/outbox/sqlalchemy.py` around lines 89 - 98, The model shows that event_id (Mapped[uuid.UUID] using BinaryUUID) and event_id_bin (Mapped[bytes] using sqlalchemy.BINARY(16)) are redundant; remove event_id_bin and any references to it (indexes, constraints, code paths) and instead create the unique/index constraint directly on event_id, or if you must keep a byte representation, make event_id_bin derived (a property) rather than a persisted column. Update ORM mappings and any usages of event_id_bin in functions, queries, or index definitions so they reference event_id (BinaryUUID) and ensure BinaryUUID is configured to use BINARY(16) for MySQL to preserve behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pyproject.toml`:
- Line 24: pyproject lists "sqlalchemy[asyncio]==2.0.*" both in core
dependencies and in optional-dependencies (extras) which conflicts with the
guarded import logic in src/cqrs/saga/storage/sqlalchemy.py and
src/cqrs/outbox/sqlalchemy.py; decide whether SQLAlchemy is required or optional
and remove the duplicate accordingly: if SQLAlchemy is mandatory for the
library, delete the extras entry ("sqlalchemy" under optional-dependencies) and
remove guarded imports in the two sqlalchemy modules; if SQLAlchemy is optional,
remove "sqlalchemy[asyncio]==2.0.*" from core dependencies and keep it only
under the extras (e.g., "sqlalchemy") so the guarded import blocks in
src/cqrs/saga/storage/sqlalchemy.py and src/cqrs/outbox/sqlalchemy.py remain
correct.
In `@src/cqrs/saga/storage/sqlalchemy.py`:
- Around line 14-25: Remove the guarded try/except import block around the
SQLAlchemy imports and replace it with direct imports of sqlalchemy, func,
SQLAlchemyError, AsyncSession, async_sessionmaker and registry if SQLAlchemy is
a required dependency; otherwise, make SQLAlchemy optional in pyproject.toml and
keep the guarded import but suppress exception chaining by raising the
ImportError with "from None" (e.g., raise ImportError(...) from None) so the
traceback is cleaner.
---
Outside diff comments:
In `@src/cqrs/outbox/sqlalchemy.py`:
- Around line 128-133: The payload column is declared as Mapped[bytes] with
sqlalchemy.LargeBinary but has a default of {} (a dict) causing a type mismatch;
update the mapped_column for payload (symbol: payload, mapped_column,
Mapped[bytes], sqlalchemy.LargeBinary) to either remove the default entirely if
add() always supplies a value, or change the default to a bytes value (e.g., an
empty bytes literal) so the default matches the declared bytes type.
- Around line 218-227: The Outbox insertion uses the MySQL-only
func.UUID_TO_BIN; change it to store the 16-byte UUID in Python for cross-DB
compatibility: in the session.add call that constructs OutboxModel (look for
OutboxModel and the event_id_bin field), replace
func.UUID_TO_BIN(event.event_id) with the UUID's 16-byte representation (e.g.
event.event_id.bytes or uuid.UUID(str(event.event_id)).bytes if event.event_id
might be a string) so event_id_bin is assigned raw bytes compatible with
BinaryUUID for both PostgreSQL and MySQL.
---
Duplicate comments:
In `@src/cqrs/outbox/sqlalchemy.py`:
- Around line 12-24: The guarded try/except around the top-level sqlalchemy
imports is inconsistent because SQLAlchemy is declared as a required dependency,
so either remove the try/except entirely and keep the direct imports (i.e.,
delete the try/except and the ImportError raise) or, if you want to keep the
runtime guard, suppress exception chaining by changing the raise to include
"from None" (i.e., modify the ImportError raise in the sqlalchemy import block
to use "raise ImportError(... ) from None"); update the import block that
references "import sqlalchemy", "from sqlalchemy import func", "from
sqlalchemy.orm import Mapped, mapped_column, DeclarativeMeta, registry", and
"from sqlalchemy.ext.asyncio import session as sql_session" accordingly.
---
Nitpick comments:
In `@src/cqrs/outbox/sqlalchemy.py`:
- Around line 89-98: The model shows that event_id (Mapped[uuid.UUID] using
BinaryUUID) and event_id_bin (Mapped[bytes] using sqlalchemy.BINARY(16)) are
redundant; remove event_id_bin and any references to it (indexes, constraints,
code paths) and instead create the unique/index constraint directly on event_id,
or if you must keep a byte representation, make event_id_bin derived (a
property) rather than a persisted column. Update ORM mappings and any usages of
event_id_bin in functions, queries, or index definitions so they reference
event_id (BinaryUUID) and ensure BinaryUUID is configured to use BINARY(16) for
MySQL to preserve behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cqrs/outbox/sqlalchemy.py`:
- Around line 128-132: The mapped_column for payload uses sqlalchemy.LargeBinary
but sets default={} (a dict) which is a type/mutable mismatch; update the
payload mapped_column (symbol: payload) to either remove the default entirely
(if payload is always provided) or change the default to a bytes value such as
b"" so the DB binding type matches LargeBinary and avoids a mutable default;
keep nullable=False as-is or adjust if you remove the default and need nullable
behavior changed.
- Around line 44-68: In process_bind_param, normalize string UUIDs before the
dialect check: first return on None, then if isinstance(value, str) convert to
uuid.UUID(value), then handle dialect-specific returns (if dialect.name ==
"postgresql" return the uuid.UUID, otherwise if isinstance(value, uuid.UUID)
return value.bytes for MySQL). Keep process_result_value behavior as-is; this
ensures PostgreSQL path receives a uuid.UUID instead of an unnormalized string.
Use the existing process_bind_param and process_result_value symbols to locate
the change.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/cqrs/outbox/sqlalchemy.py`:
- Around line 12-25: The except block currently catches all ImportError
instances; change it to catch ModuleNotFoundError as exc and check exc.name ==
"sqlalchemy" so you only convert the missing-package case into the friendly
message; for any other import errors (e.g., internal SQLAlchemy submodule
failures) re-raise the original exception. Update the except clause around the
imports in src/cqrs/outbox/sqlalchemy.py (the try that imports "sqlalchemy",
"func", "Mapped", "mapped_column", "DeclarativeMeta", "registry", "sql_session",
and "postgresql") to implement this conditional handling.
In `@src/cqrs/saga/storage/sqlalchemy.py`:
- Around line 14-25: The current broad except ImportError in the top-level
import block masks unrelated import errors; change it to catch
ModuleNotFoundError as exc and only handle the case where exc.name ==
"sqlalchemy" (re-raise the friendly message), otherwise re-raise the original
exception so nested import failures inside sqlalchemy propagate; update the
try/except around the imports (the block that imports sqlalchemy, func,
SQLAlchemyError, AsyncSession/async_sessionmaker and registry) accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 90e9f570-69bd-4c0e-8e7c-d5d32ecd0f83
📒 Files selected for processing (2)
src/cqrs/outbox/sqlalchemy.pysrc/cqrs/saga/storage/sqlalchemy.py
Comment on lines
+12
to
+25
| try: | ||
| import sqlalchemy | ||
|
|
||
| from sqlalchemy import func | ||
| from sqlalchemy.orm import Mapped, mapped_column, DeclarativeMeta, registry | ||
| from sqlalchemy.ext.asyncio import session as sql_session | ||
| from sqlalchemy.dialects import postgresql | ||
| except ImportError: | ||
| raise ImportError( | ||
| "You are trying to use SQLAlchemy outbox implementation, " | ||
| "but 'sqlalchemy' is not installed. " | ||
| "Please install it using: pip install python-cqrs[sqlalchemy]", | ||
| ) from None | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/cqrs/outbox/sqlalchemy.py | head -30Repository: pypatterns/python-cqrs
Length of output: 1011
🏁 Script executed:
python3 << 'EOF'
# Verify ModuleNotFoundError behavior and attributes
import sys
# Test 1: Check ModuleNotFoundError is subclass of ImportError
print(f"ModuleNotFoundError is subclass of ImportError: {issubclass(ModuleNotFoundError, ImportError)}")
# Test 2: Check what attributes ModuleNotFoundError has
try:
import nonexistent_module_xyz_123
except ModuleNotFoundError as e:
print(f"\nModuleNotFoundError attributes:")
print(f" - name: {getattr(e, 'name', 'NOT FOUND')}")
print(f" - path: {getattr(e, 'path', 'NOT FOUND')}")
print(f" - type(e): {type(e).__name__}")
# Test 3: Check what happens with nested ImportError from within a module
print(f"\n--- Test 3: Nested import failure ---")
# Create a test scenario where a submodule fails to import
try:
# This will raise ModuleNotFoundError because 'sqlalchemy' doesn't exist
import sqlalchemy
from sqlalchemy import nonexistent_submodule
except ModuleNotFoundError as e:
print(f"Error name: {e.name}")
except ImportError as e:
print(f"Got ImportError: {e}")
# Test 4: Verify that nested imports raise ModuleNotFoundError when the top-level module doesn't exist
print(f"\n--- Test 4: Checking if nested missing imports raise ModuleNotFoundError ---")
try:
from nonexistent.nested.module import something
except ModuleNotFoundError as e:
print(f"Caught ModuleNotFoundError with name='{e.name}'")
except Exception as e:
print(f"Caught {type(e).__name__}: {e}")
EOFRepository: pypatterns/python-cqrs
Length of output: 427
🏁 Script executed:
# Check for other ImportError patterns in this file
rg "except.*Import" src/cqrs/outbox/sqlalchemy.py -A 3Repository: pypatterns/python-cqrs
Length of output: 221
🏁 Script executed:
# Also check other files in the outbox directory for similar patterns
find src/cqrs/outbox -name "*.py" -type f | head -20Repository: pypatterns/python-cqrs
Length of output: 200
🏁 Script executed:
# Check for ImportError patterns in all outbox files
rg "except.*Import" src/cqrs/outbox/ -A 5Repository: pypatterns/python-cqrs
Length of output: 489
🏁 Script executed:
# Test what happens when a submodule within an existing package has an import error
python3 << 'EOF'
# Simulate what happens when sqlalchemy exists but one of its submodules fails
# We'll create a mock scenario
# Test 1: What type of error when trying to import from a missing submodule of an existing package?
import sys
import types
# Create a fake sqlalchemy module that exists but one of its imports fails internally
fake_sqlalchemy = types.ModuleType('fake_sqlalchemy_test')
# Create a submodule that will fail on import
def failing_import():
raise ImportError("Internal import failed within sqlalchemy")
# This simulates a scenario where sqlalchemy itself imports but a submodule fails
try:
# Simulate the error that would occur
raise ImportError("Some internal sqlalchemy import issue")
except Exception as e:
print(f"Internal import error type: {type(e).__name__}")
print(f"Has 'name' attribute: {hasattr(e, 'name')}")
if hasattr(e, 'name'):
print(f"name value: {e.name}")
# The key insight: internal import failures may raise ImportError (not ModuleNotFoundError)
# if they're caused by issues within the module, not because the module doesn't exist
print("\n--- Key difference ---")
print("ModuleNotFoundError: raised when a module cannot be found")
print("ImportError: can be raised for other import-related issues (syntax errors, circular imports, etc.)")
print("The proposed fix may NOT catch all internal import failures")
EOFRepository: pypatterns/python-cqrs
Length of output: 390
🏁 Script executed:
# Test nested import scenarios more realistically
python3 << 'EOF'
# Test scenario: what happens with nested submodule imports
# Scenario 1: Top-level module missing
print("=== Scenario 1: Top-level module missing ===")
try:
import nonexistent_package_xyz
except ModuleNotFoundError as e:
print(f"Exception: {type(e).__name__}")
print(f"name: {e.name}")
# Scenario 2: Simulate nested module import attempt
# When you do: from sqlalchemy import func
# If sqlalchemy doesn't exist, what happens?
print("\n=== Scenario 2: What happens with from X import Y when X is missing ===")
try:
from nonexistent_package import something
except ModuleNotFoundError as e:
print(f"Exception: {type(e).__name__}")
print(f"name: {e.name}")
print(f"The 'name' attribute correctly identifies: {e.name}")
# Scenario 3: Multiple imports - what if second one fails?
print("\n=== Scenario 3: Multiple imports where first succeeds, second fails ===")
try:
import sys # this exists
from nonexistent_module import something # this doesn't
except ModuleNotFoundError as e:
print(f"Exception: {type(e).__name__}")
print(f"name: {e.name}")
# The key insight: The proposed fix handles all these correctly
# because ModuleNotFoundError for any missing module will have name set
# and the check `if exc.name != "sqlalchemy"` will re-raise non-sqlalchemy errors
print("\n=== Analysis ===")
print("Proposed fix catches: ModuleNotFoundError with name='sqlalchemy'")
print("Proposed fix re-raises: ModuleNotFoundError with name!='sqlalchemy'")
print("Proposed fix does NOT catch: other ImportError subclasses (AttributeError, etc.)")
EOFRepository: pypatterns/python-cqrs
Length of output: 725
🏁 Script executed:
# Check all outbox files for similar import error handling patterns
for file in src/cqrs/outbox/*.py; do
echo "=== $file ==="
rg "except.*Import" "$file" -A 3 || echo "No ImportError patterns found"
doneRepository: pypatterns/python-cqrs
Length of output: 516
🏁 Script executed:
# Also check for similar patterns elsewhere in the codebase
rg "except ImportError" src/ --include="*.py" | head -20Repository: pypatterns/python-cqrs
Length of output: 507
🏁 Script executed:
# Search for similar ImportError patterns in the codebase
rg "except ImportError" src/ -A 3 | head -30Repository: pypatterns/python-cqrs
Length of output: 1571
🏁 Script executed:
# Examine the sqlalchemy.py and saga storage sqlalchemy.py patterns more closely
echo "=== Outbox sqlalchemy.py ===" && sed -n '12,24p' src/cqrs/outbox/sqlalchemy.py
echo -e "\n=== Saga storage sqlalchemy.py ===" && rg "except ImportError" src/cqrs/saga/storage/sqlalchemy.py -B 10 -A 6Repository: pypatterns/python-cqrs
Length of output: 1274
🏁 Script executed:
# Final verification: test the proposed fix logic more thoroughly
python3 << 'EOF'
# Test the proposed fix logic comprehensively
print("=== Testing proposed fix logic ===\n")
# Simulating the proposed fix behavior
def test_proposed_fix(exception_to_raise):
"""Simulates the proposed fix"""
try:
if exception_to_raise == "missing_sqlalchemy":
raise ModuleNotFoundError("No module named 'sqlalchemy'", name='sqlalchemy')
elif exception_to_raise == "missing_submodule":
raise ModuleNotFoundError("No module named 'sqlalchemy.orm'", name='sqlalchemy')
elif exception_to_raise == "missing_other":
raise ModuleNotFoundError("No module named 'other_package'", name='other_package')
except ModuleNotFoundError as exc:
if exc.name != "sqlalchemy":
print(f"✓ Re-raised: {type(exc).__name__} for {exc.name}")
raise
print(f"✓ Caught: {type(exc).__name__} for {exc.name}")
return "custom message"
test_cases = [
("missing_sqlalchemy", "Should catch and show custom message"),
("missing_submodule", "Should catch (name=sqlalchemy)"),
("missing_other", "Should re-raise"),
]
for case, expected in test_cases:
print(f"Case: {case} - {expected}")
try:
result = test_proposed_fix(case)
if result:
print(f" Result: Custom message shown\n")
except ModuleNotFoundError as e:
print(f" Result: Re-raised ModuleNotFoundError\n")
print("=== Conclusion ===")
print("The proposed fix correctly:")
print("1. Catches sqlalchemy ModuleNotFoundError (name='sqlalchemy')")
print("2. Re-raises ModuleNotFoundError for other packages (name!='sqlalchemy')")
print("3. Narrows exception scope from all ImportError to just ModuleNotFoundError")
EOFRepository: pypatterns/python-cqrs
Length of output: 764
Use a targeted missing-package check instead of broad except ImportError.
This block catches all ImportError instances, including internal SQLAlchemy import failures, and always emits the "not installed" message. Use ModuleNotFoundError with an exc.name check to distinguish between a missing sqlalchemy package and other import-related errors.
Proposed fix
try:
import sqlalchemy
from sqlalchemy import func
from sqlalchemy.orm import Mapped, mapped_column, DeclarativeMeta, registry
from sqlalchemy.ext.asyncio import session as sql_session
from sqlalchemy.dialects import postgresql
-except ImportError:
- raise ImportError(
+except ModuleNotFoundError as exc:
+ if exc.name != "sqlalchemy":
+ raise
+ raise ImportError(
"You are trying to use SQLAlchemy outbox implementation, "
"but 'sqlalchemy' is not installed. "
"Please install it using: pip install python-cqrs[sqlalchemy]",
) from None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cqrs/outbox/sqlalchemy.py` around lines 12 - 25, The except block
currently catches all ImportError instances; change it to catch
ModuleNotFoundError as exc and check exc.name == "sqlalchemy" so you only
convert the missing-package case into the friendly message; for any other import
errors (e.g., internal SQLAlchemy submodule failures) re-raise the original
exception. Update the except clause around the imports in
src/cqrs/outbox/sqlalchemy.py (the try that imports "sqlalchemy", "func",
"Mapped", "mapped_column", "DeclarativeMeta", "registry", "sql_session", and
"postgresql") to implement this conditional handling.
Comment on lines
+14
to
+25
| try: | ||
| import sqlalchemy | ||
| from sqlalchemy import func | ||
| from sqlalchemy.exc import SQLAlchemyError | ||
| from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker | ||
| from sqlalchemy.orm import registry | ||
| except ImportError: | ||
| raise ImportError( | ||
| "You are trying to use SQLAlchemy saga storage implementation, " | ||
| "but 'sqlalchemy' is not installed. " | ||
| "Please install it using: pip install python-cqrs[sqlalchemy]", | ||
| ) from None |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/cqrs/saga/storage/sqlalchemy.py | head -30Repository: pypatterns/python-cqrs
Length of output: 1179
🌐 Web query:
Python ModuleNotFoundError vs ImportError exception handling optional imports
💡 Result:
ImportError vs ModuleNotFoundError
ModuleNotFoundErroris a subclass ofImportErrorand is raised when Python can’t locate the requested module/package. [1][2]ImportErroris broader: it also covers cases where the module was found, but the import still failed (e.g.,from pkg import namewherenamedoesn’t exist, loader issues, etc.). Also, importing can surface any exception raised while executing the module body, not justImportError. [3]
Optional imports: what to catch (and why)
A common pitfall is:
try:
import foo
except ImportError:
foo = NoneThis can accidentally swallow real bugs inside foo (e.g., foo exists, but it raises ImportError while importing something else). This is a well-known problem with catching plain ImportError for optional deps. [4]
Recommended pattern (Python 3.6+): catch ModuleNotFoundError and check which module was missing
try:
import foo
except ModuleNotFoundError as e:
if e.name == "foo":
foo = None # truly optional dependency missing
else:
raise # some dependency *inside* foo is missingThis keeps the “optional dependency” behavior without hiding unrelated import failures. [1][4]
Alternative: probe availability first (then import)
import importlib.util
if importlib.util.find_spec("foo") is None:
foo = None
else:
import foofind_spec() returns None if no module spec is found. [5]
Sources: Python exception hierarchy / ModuleNotFoundError ⟶ ImportError [1], definition of ModuleNotFoundError as “module could not be located” [2], note that import can raise arbitrary exceptions from module execution [3], why catching ImportError can swallow real errors [4], importlib.util.find_spec() behavior [5].
[1] (docs.python.org)
[2] (simuline.eu)
[3] (bugs.python.org)
[4] (pypi.org)
[5] (docs.python.org)
Citations:
- 1: https://docs.python.org/3.11/library/exceptions.html?highlight=exceptions
- 2: https://www.simuline.eu/QualityManagement/literature/Languages/Python/3.10.1/library.pdf?utm_source=openai
- 3: https://bugs.python.org/issue40899?utm_source=openai
- 4: https://pypi.org/project/optional-import/
- 5: https://docs.python.org/3.12/library/importlib.html
Narrow the optional-import exception to avoid masking real failures.
except ImportError (line 20) also catches exceptions from nested import failures within sqlalchemy and rewrites them as "sqlalchemy not installed," which hides the actual cause. Use ModuleNotFoundError with a check on exc.name == "sqlalchemy" to let unrelated import errors propagate.
Proposed fix
try:
import sqlalchemy
from sqlalchemy import func
from sqlalchemy.exc import SQLAlchemyError
from sqlalchemy.ext.asyncio import AsyncSession, async_sessionmaker
from sqlalchemy.orm import registry
-except ImportError:
- raise ImportError(
+except ModuleNotFoundError as exc:
+ if exc.name != "sqlalchemy":
+ raise
+ raise ImportError(
"You are trying to use SQLAlchemy saga storage implementation, "
"but 'sqlalchemy' is not installed. "
"Please install it using: pip install python-cqrs[sqlalchemy]",
) from None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/cqrs/saga/storage/sqlalchemy.py` around lines 14 - 25, The current broad
except ImportError in the top-level import block masks unrelated import errors;
change it to catch ModuleNotFoundError as exc and only handle the case where
exc.name == "sqlalchemy" (re-raise the friendly message), otherwise re-raise the
original exception so nested import failures inside sqlalchemy propagate; update
the try/except around the imports (the block that imports sqlalchemy, func,
SQLAlchemyError, AsyncSession/async_sessionmaker and registry) accordingly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Chores