fix(orm): harden ConnectionFactory against bad port and unknown driver#194
Merged
Conversation
build_url()/make() previously surfaced bad connection configs as raw, low-level errors: - mysql/postgres with a missing/empty 'port' produced a URL with a bare trailing colon that crashed at engine creation with ValueError: invalid literal for int() with base 10: ''. - an unsupported driver raised a raw KeyError from DRIVER_URLS[driver]. - make()'s 'Unsupported driver' fallback was unreachable dead code, since create_engine()/build_url() always raised the raw KeyError first. Hardening: - Add per-driver default ports (mysql 3306, postgres 5432); an explicit port still takes precedence, so the happy path is byte-for-byte unchanged. - build_url() and make() now raise the existing framework-level DriverNotFound with a message listing the supported drivers. The check in make() runs before any engine is built, so it is reachable and meaningful; the dead match fallback is removed in favour of a CONNECTIONS lookup. The documented 'url' passthrough still short-circuits field assembly, so supplying a full url bypasses both driver and port validation. Regression tests cover: mysql/postgres missing and empty-string ports, explicit-port precedence, unsupported driver via build_url() and make(), url passthrough, and unchanged valid sqlite/mysql/postgres URLs.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
1 participant
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
Hardens
ConnectionFactory.build_url()/make()so bad database connection configs fail fast with clear, framework-level errors instead of raw low-level exceptions surfaced deep inside SQLAlchemy. Addresses task #1223 (findings from investigation #1226, originating from PR #191 review).Problems fixed
port = config.get("port", "")defaulted to"", producing a URL with a bare trailing colon that crashed at engine creation withValueError: invalid literal for int() with base 10: ''.KeyErroron unknown driver —DRIVER_URLS[driver]raised a bareKeyError, not a friendly error.make()'sraise ValueError("Unsupported driver")was unreachable, becausecreate_engine()→build_url()always raised the rawKeyErrorfirst.Changes
mysql→ 3306,postgres→ 5432). An explicitly configured port still takes precedence, so the common/happy path is byte-for-byte unchanged.DriverNotFound(an existing, previously-unused framework exception) raised from bothbuild_url()andmake(), with a message listing the supported drivers. The check inmake()now runs before any engine is built — reachable and meaningful — replacing the deadmatchfallback with aCONNECTIONSlookup.urlpassthrough still short-circuits field assembly, so supplying a fullurlbypasses both driver and port validation (useful stopgap for anyone hitting this today).Scope note
Finding (d) — upstream config validation in
DatabaseManager/DatabaseProvider— was intentionally left out of this pass. The factory-level friendly errors already make bad configs fail with a clear message at connection resolution; broader manager/provider validation is a larger, separate change and was not in the task's enumerated asks (1–3).Tests
New regression tests in
tests/masoniteorm/config/test_db_url.py:build_url()andmake()→DriverNotFoundwith a helpful messageurlpassthrough bypasses driver validationUpdated
tests/masoniteorm/models/test_model_attributes.py::test_connection_raises_for_missing_driver: it previously mockedcreate_enginespecifically to reach the dead fallback and asserted the rawValueError("Unsupported driver"). It now asserts the friendlyDriverNotFoundthat fires before any engine is built (mock no longer needed).Verification
uv run pytest --cov --ignore=tests/masoniteorm/postgres→ 1981 passed, 7 skipped,Required test coverage of 80.0% reached. Total coverage: 81.22%.