fix(trino): resolve the mapped host port in get_connection_url() - #1112
Open
rusackas wants to merge 1 commit into
Open
fix(trino): resolve the mapped host port in get_connection_url()#1112rusackas wants to merge 1 commit into
rusackas wants to merge 1 commit into
Conversation
get_connection_url() returned the container-internal port (self.port, e.g. 8080) instead of the Docker-mapped host port, so the URL it built could never actually connect -- the existing test bypassed this method entirely, connecting via trino.dbapi.connect() with an explicitly resolved get_exposed_port() instead, so the bug went uncaught. Fixes the same way CockroachDBContainer/CrateDBContainer already do it via _create_connection_url(), and adds a test that actually exercises get_connection_url() via SQLAlchemy, matching the sibling cockroachdb/ cratedb tests' pattern. Fixes testcontainers#1111
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.
Fixes #1111
What's wrong
TrinoContainer.get_connection_url()builds the URL astrino://{user}@{host}:{self.port}, andself.portis the fixed container-internal port set in__init__(8080 by default) — never resolved throughget_exposed_port(). Since Docker maps that to a random host port, the URL this method returns can't actually connect to anything.The existing test (
test_docker_run_trino) didn't catch this because it bypassesget_connection_url()entirely, connecting viatrino.dbapi.connect()with an explicitly resolvedget_exposed_port(trino.port)instead.The fix
One-line change, matching the pattern
CockroachDBContainerandCrateDBContaineralready use via_create_connection_url():Also added
test_get_connection_url, which actually exercisesget_connection_url()via SQLAlchemy (the way a real user would use it), matching the siblingcockroachdb/cratedbtests' pattern. Ran locally against a real container — passes with the fix, and the bug is reproducible by inspection:trino.portandtrino.get_exposed_port(trino.port)return different values whenever Docker doesn't happen to map the container port 1:1 to the host, which is the common case.Where this was found
Building a nightly CI job in
apache/supersetthat tests its Trinodb_engine_specsmodule against a real Trino instance via this library — more context in the linked issue.PR Checklist
fix(trino):), scoped to the community module per the community-modules note in the PR template.src/testcontainers/community/trino/with a corresponding test intests/community/trino/.main.