Make the nspek postgres connection host/port/database configurable#389
Open
Odin107 wants to merge 2 commits into
Open
Make the nspek postgres connection host/port/database configurable#389Odin107 wants to merge 2 commits into
Odin107 wants to merge 2 commits into
Conversation
set_nspek_connection hardcoded the connection target to localhost:5432/nspek, so the NaeringsspesifikasjonWindow/Tab modules could only ever reach a database literally named "nspek" on localhost. Deployments whose nspek data lives in a differently named database (or a different host/port) fail at construction with `FATAL: database "nspek" does not exist`. Add host/port/database parameters to set_nspek_connection (defaulting to the previous localhost:5432/nspek values) and thread db_host/db_port/db_name through Naeringsspesifikasjon and its Tab/Window subclasses, mirroring the existing db_user parameter. Extract the conninfo construction into a small, testable _build_nspek_conn_url helper and drop a stray print() of the connection URL. Defaults preserve current behavior; the other internal callers (nspek_controls, the __main__ demo) are unaffected.
|
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
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
Makes the NSPEK module's database connection target configurable.
set_nspek_connectionpreviously hardcodedlocalhost:5432/nspek, soNaeringsspesifikasjonWindow/NaeringsspesifikasjonTabcould only ever reach a database literally namednspekon localhost. Any deployment whose NSPEK data lives in a differently named database (or on a different host/port) crashes at construction withFATAL: database "nspek" does not exist. This PR adds optionalhost/port/databaseparameters (defaulting to the previous values) and threadsdb_host/db_port/db_namethrough the module classes, mirroring the existingdb_userparameter. Fully backward compatible.Changes Made
src/ssb_dash_framework/modules/nspek/nspek_utils.pyset_nspek_connection(...)gainshost="localhost",port=5432,database="nspek"._build_nspek_conn_url(...)helper that builds theconninfoURL (makes the behavior unit-testable without a live DB).print(conn_url)debug statement that leaked the connection URL to stdout.database_urlargument).src/ssb_dash_framework/modules/nspek/nspek.pyNaeringsspesifikasjon.__init__,NaeringsspesifikasjonTab.__init__, andNaeringsspesifikasjonWindow.__init__gaindb_host/db_port/db_name(same defaults) and forward them toset_nspek_connection.tests/test_nspek_connection.py(new)set_nspek_connectionforwards the target toConnectionPool.Why These Changes Were Needed
The NSPEK module opens its own pooled connection, separate from the app's main
set_postgres_connection, and that connection was pinned tolocalhost:5432/nspek. In an environment where the relevant Postgres instance (e.g. behind a CloudSQL proxy onlocalhost:5432) does not contain a database callednspek, instantiatingNaeringsspesifikasjonWindow(...)aborts the whole app build with aPoolTimeoutwhose underlying cause isFATAL: database "nspek" does not exist. There was no way to point the module at the correct database short of monkeypatchingset_nspek_connection. Exposing the connection target makes the module usable across deployments without forking the framework.Implementation Details
localhost/5432/nspek), so existing callers - including the two other internal call sites (nspek_controls.pyand the__main__demo innspek_utils.py) - are unaffected._build_nspek_conn_urlisolates thepostgresql://{user}@{host}:{port}/{database}construction (withquote_plususer encoding) so it can be asserted directly in a unit test, avoiding the live-DB validation thatset_nspek_connectionperforms at the end.db_host/db_port/db_namefollow the exact pattern of the existingdb_userparameter throughNaeringsspesifikasjon→Tab/Window.Code Changes
nspek_utils.py- configurable target + extracted builder, debugprintremoved:nspek.py- threaded through the module classes (window shown; tab/base identical):Example downstream usage - point the NSPEK tab at the deployment's actual database:
Testing / Validation
tests/test_nspek_connection.py(no live DB; the post-construction validation is satisfied with aMagicMock(spec=BaseBackend)):test_build_nspek_conn_url_defaults/test_build_nspek_conn_url_custom_target- URL is built correctly, user isquote_plus-encoded.test_set_nspek_connection_forwards_target_to_pool- host/port/database reachConnectionPool(conninfo=...).test_set_nspek_connection_defaults_unchanged- omitting the args yields the originallocalhost:5432/nspekURL.4 passed.inspect.signatureconfirmsdb_host/db_port/db_nameare present on all three classes with"nspek"/5432/"localhost"defaults.Reviewer Notes
Naeringsspesifikasjonclass family is threaded (the user-facing Window/Tab). The otherset_nspek_connectioncaller innspek_controls.pykeeps the defaults; if you'd like the NSPEK controls to be configurable too, that's a small parallel follow-up.set_postgres_connection. Making the target configurable is the minimal fix; if NSPEK data generally lives in the same database as the rest of the app, an alternative worth considering is letting the module reuse the main connection instead of opening its own. Happy to take this whichever direction you prefer.