fix(sqllab): default PostgreSQL port to 5432 in the dynamic connection form - #43351
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #43351 +/- ##
==========================================
- Coverage 66.81% 66.81% -0.01%
==========================================
Files 2876 2876
Lines 164454 164472 +18
Branches 37960 37961 +1
==========================================
+ Hits 109873 109885 +12
- Misses 52388 52393 +5
- Partials 2193 2194 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…s from CI CI caught two sibling test-expectation staleness issues missed in the prior local round (a marshmallow==3.26.2/apispec==6.10.0 local mismatch vs. this repo's pinned marshmallow==4.3.0/apispec==6.6.1 masked one of them): - tests/integration_tests/databases/api_tests.py::test_available has its own hardcoded /api/v1/database/available/ snapshot; its PostgreSQL entry still listed port as required and was missing port's nullable:true. - tests/integration_tests/db_engine_specs/postgres_tests.py:: test_base_parameters_mixin asserted a top-level additionalProperties:False that PostgresEngineSpec.parameters_json_schema() does not actually produce under the pinned marshmallow/apispec versions. Verified both fixes against the pinned versions installed locally, plus a full regression pass of the previously-touched integration test files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…nd test Sibling of the two backend integration test mocks fixed previously: DatabaseModal/index.test.tsx's fetch-mocked /api/v1/database/available/ response for PostgreSQL still listed port as required and was missing port's nullable:true, both stale now that PostgresEngineSpec no longer requires port. Verified byte-accurate against PostgresEngineSpec.parameters_json_schema()'s actual output under the repo's pinned marshmallow/apispec versions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
sadpandajoe
added
review:draft
🎪 ⚡ showtime-trigger-start
github-actions
Bot
added
🎪 28db79d 🚦 building
🎪 ⌛ 48h
|
🎪 Showtime deployed environment on GHA for d06fba3 • Environment: http://54.202.103.113:8080 (admin/admin) |
…ecks BasicParametersMixin.validate_parameters's `if not port: return errors` treated an explicit, present port of 0 the same as a missing/blank port, since 0 is falsy in Python -- silently skipping the int/range/is_port_open checks for a value the schema's own Range(min=0, ...) validator accepts as legitimate. Narrow the short-circuit to the two actual blank representations (`None` from an omitted/null port, `""` from a cleared HTML number input), leaving the missing-parameters detection above it untouched (a separate, unrelated falsy-value quirk, out of scope here). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Code Review Agent Run #02a9bdActionable Suggestions - 0Review Details
Bito Usage GuideCommands Type the following command in the pull request comment and save the comment.
Refer to the documentation for additional commands. Configuration This repository uses Documentation & Help AI Code Review powered by |
|
Bito Automatic Review Skipped – PR Already Merged |
2 participants
SUMMARY
The "Connect a database" dynamic form for PostgreSQL required a Port value and blocked
connecting when it was left blank, even though PostgreSQL has a well-known default port
(5432) and the SQLAlchemy-URI connection path already worked fine without specifying one.
PostgresEngineSpecinheritedBasicParametersMixin's validation and URI-building logicunmodified, which requires
portunconditionally in three places: the parameters JSONschema exposed to the frontend form, the progressive
validate_parameterscheck, andbuild_sqlalchemy_uri. None of them consulted the engine's own advertised default port.This PR gives
PostgresEngineSpecits own parameter schema/validation/URI-buildingoverrides (mirroring the existing pattern in
ClickHouseEngineSpec) so a blank portdefaults to 5432 instead of blocking the connection, while all other validation
(host resolution, port format/range/openness when a port is supplied) is unchanged.
This optional-port behavior is inherited by every
PostgresEngineSpecsubclass(CockroachDB, Greenplum, RisingWave, and both Aurora PostgreSQL variants), each
falling back to its own default via
cls.metadata["default_port"].BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
N/A at PR-open time — will be added after manual verification.
TESTING INSTRUCTIONS
being blocked by a "missing parameter" error.
port-range/format validation errors.
Automated coverage:
tests/unit_tests/db_engine_specs/test_postgres.py(13 new casescovering
build_sqlalchemy_uri's port defaulting,validate_parameters's relaxedrequirement, and the relaxed parameters schema) and updated
tests/integration_tests/db_engine_specs/postgres_tests.py/tests/integration_tests/databases/{commands_tests,api_tests}.py.ADDITIONAL INFORMATION