fix(demo): make the docker demo datasources reachable - #74
Merged
Conversation
Uh oh!
There was an error while loading. Please reload this page.
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 freeto 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.
Two bugs, both of which had to be fixed for the Docker demo to work at all
Bug 1 —
${env:...}in any non-password field broke adapter validationDatasourceRegistry.resolved_connection()wrapped every resolved${...}placeholder in
SecretStr, not just the passwords:The wrap could not be made selective at that point, because
ConnectionConfigcarries no type information to work from — it declares a single field,
type: str,with
extra: "allow". Everything else (host,port,user,password,driver)arrives as an untyped extra. The real types live in each adapter's own model, e.g.
PostgresConnectionConfig:host: str,user: str,password: SecretStr. Theregistry has no way to tell a password apart from a hostname.
So
user: "${env:DEMO_REF_USER}"reached the adapter as aSecretStrand validationblew up:
Every
DEMO_DOCKER_DATASOURCESentry uses${env:...}foruser, so the Dockerdemo has never been able to register a datasource.
The fix is to stop wrapping. Pydantic already coerces a plain string into
SecretStrfor fields declared that way, so returning the resolved plain stringgives
userastrandpasswordaSecretStr— both correct, with maskingpreserved exactly where it is declared.
Password masking — one consequence, handled
resolved_connectionhas a single caller, which immediately.model_dump()s theresult into
connection_args. Tracing where that goes turned up two places wherethe plaintext password was previously masked by
SecretStr's repr and would nowbe exposed:
DatasourceAPI.get_datasource_details()returnsadapter.connection_argsverbatim to callers. Now masked by name via
mask_connection_args.ValidationErrorrenders the whole input dict when a requiredfield is missing, and
nl2sql doctorprints that message straight to theconsole. Adapter construction is now wrapped so the message is redacted, and
re-raised
from Noneso the traceback does not carry the cause either.The adapter itself still receives the real value — only the rendered views are
masked. The connection URI has always contained the plaintext password; that is
unchanged by this PR.
Bug 2 —
localhostis the wrong host inside the app containerDEMO_DOCKER_DATASOURCEShardcodedhost: "localhost"with the published ports(5433, 5434, 3307, 1434). That is right from the developer's machine and wrong from
inside the
appcontainer, which has to reach the databases by Compose service nameon their internal ports.
Both callers have to keep working, so host and port are now env-driven with
host-appropriate defaults:
nl2sql --env demo index/runlocalhost.env.demoappcontainerenvironment:onapp, which overridesenv_fileThis uses the same
${env:...}convention already in place for user and password —which is exactly why Bug 1 had to be fixed first, or the new host and port
references would have hit the same
SecretStrfailure.Tests
New tests in the existing files:
resolved_connectionreturnsstrfor non-secret fields, and a realPostgresConnectionConfigstill receives aSecretStrpassword.DEMO_DOCKER_DATASOURCES-shaped config with${env:...}inuser,hostand
portregisters and produces the expected URI. This fails againstmainwith the three-error
ValidationErrorquoted above.get_datasource_detailsmasks it.
appservice sets the service-name/internal-portoverrides, the generated
.envdefaults tolocalhost+ published ports, andthe databases keep their published ports for host access.
Verification
nl2sql setup --demo --docker, thendocker compose config --services:manufacturing_ref,manufacturing_supply,manufacturing_ops,app--profile mssql→ the above plusmanufacturing_historyappservice ends up withDEMO_REF_HOST: manufacturing_ref/DEMO_REF_PORT: "5432", overriding thelocalhost:5433in.env.demo.DEMO_*vars set, all four docker-shapeddatasources resolve to plain
strand build sane URIs, e.g.postgresql://ref_admin:********@manufacturing_ref:5432/manufacturing_refandmssql+pyodbc://history_admin:********@manufacturing_history:1433/manufacturing_history?driver=ODBC+Driver+17+for+SQL+Server.No images were pulled and no containers were started. Docs updated in
docs/getting_started/demo.md.