Follow-up to #210/#211, which found that connect() and Connection.__init__ disagreed on
whether providing both endpoint and (cloud_provider, cloud_region) was an error -- fixed by
moving that check down into __init__ so both connect() and direct Connection(...)
construction behave identically. Auditing the rest of connect()'s validation for the same
disease turned up two more spots.
1. environment_id is only validated in connect(), not __init__
connect() raises:
if not environment_id:
raise InterfaceError("Environment ID is required")
Connection.__init__ has no equivalent check -- it just does self.environment_id = environment_id and moves on. Confirmed by hand that this constructs without error today:
Connection(
flink_api_key="k", flink_api_secret="s",
environment_id="", organization_id="org-id",
cloud_provider="aws", cloud_region="us-east-1",
endpoint=None,
)
environment_id gets interpolated directly into request paths (e.g.
f"/environments/{self.environment_id}"), so an empty value doesn't fail fast -- it produces a
malformed URL that only surfaces later as a confusing server-side 404 / OperationalError
instead of a clear InterfaceError at construction time.
Unlike organization_id (see below), there's no comment or docstring anywhere suggesting this
asymmetry is intentional. It looks like a plain oversight.
Fix: move the environment_id truthiness check down into Connection.__init__, the same way
the endpoint/cloud-info check was moved in #211, so connect() and direct Connection()
construction both fail fast with the same error.
(Note: organization_id's current connect()-only enforcement is not in scope here -- its
docstring in __init__ explicitly documents that the constructor intentionally leaves it empty
when no global key is present, deferring to connect(), because a global key enables lazy
inference later via the organization_id property. Pushing that down would require porting the
same "unless a global key is provided" conditional into __init__, which is a real design
decision rather than a straightforward bug fix -- worth its own separate discussion if we want to
tackle it.)
2. Unify the cloud_provider/cloud_region "missing" check to live only in __init__
Both connect() and __init__ already raise when endpoint is absent and cloud_provider
and/or cloud_region is also missing, so there's no behavioral gap here -- but the two raise
sites duplicate the same validation with different wording:
connect() raises two distinct, field-specific messages:
"Cloud provider is required when endpoint is not provided"
"Cloud region is required when endpoint is not provided"
Connection.__init__ raises one combined message:
"cloud_provider and cloud_region are required when endpoint is not provided"
Tests currently pin both wordings independently (test_connection_unit.py, around the
test_requires_cloud_provider / test_requires_cloud_region tests using connection_factory,
and the direct-Connection() construction test using the combined message).
Fix: collapse this into a single check living only in __init__ (removing the duplicate from
connect()), so there's one source of truth. Prefer keeping the more specific per-field
wording (nicer error messages), so __init__ would raise
"Cloud provider is required when endpoint is not provided" /
"Cloud region is required when endpoint is not provided" as appropriate. Tests that currently
pin the combined __init__-specific message will need updating to match.
Scope
Both are InterfaceError message/placement changes only, no behavioral change for connect()
callers (who already got these errors) -- the only newly-affected callers are those constructing
Connection directly.
Follow-up to #210/#211, which found that
connect()andConnection.__init__disagreed onwhether providing both
endpointand (cloud_provider,cloud_region) was an error -- fixed bymoving that check down into
__init__so bothconnect()and directConnection(...)construction behave identically. Auditing the rest of
connect()'s validation for the samedisease turned up two more spots.
1.
environment_idis only validated inconnect(), not__init__connect()raises:Connection.__init__has no equivalent check -- it just doesself.environment_id = environment_idand moves on. Confirmed by hand that this constructs without error today:environment_idgets interpolated directly into request paths (e.g.f"/environments/{self.environment_id}"), so an empty value doesn't fail fast -- it produces amalformed URL that only surfaces later as a confusing server-side 404 /
OperationalErrorinstead of a clear
InterfaceErrorat construction time.Unlike
organization_id(see below), there's no comment or docstring anywhere suggesting thisasymmetry is intentional. It looks like a plain oversight.
Fix: move the
environment_idtruthiness check down intoConnection.__init__, the same waythe endpoint/cloud-info check was moved in #211, so
connect()and directConnection()construction both fail fast with the same error.
(Note:
organization_id's current connect()-only enforcement is not in scope here -- itsdocstring in
__init__explicitly documents that the constructor intentionally leaves it emptywhen no global key is present, deferring to
connect(), because a global key enables lazyinference later via the
organization_idproperty. Pushing that down would require porting thesame "unless a global key is provided" conditional into
__init__, which is a real designdecision rather than a straightforward bug fix -- worth its own separate discussion if we want to
tackle it.)
2. Unify the cloud_provider/cloud_region "missing" check to live only in
__init__Both
connect()and__init__already raise whenendpointis absent andcloud_providerand/or
cloud_regionis also missing, so there's no behavioral gap here -- but the two raisesites duplicate the same validation with different wording:
connect()raises two distinct, field-specific messages:"Cloud provider is required when endpoint is not provided""Cloud region is required when endpoint is not provided"Connection.__init__raises one combined message:"cloud_provider and cloud_region are required when endpoint is not provided"Tests currently pin both wordings independently (
test_connection_unit.py, around thetest_requires_cloud_provider/test_requires_cloud_regiontests usingconnection_factory,and the direct-
Connection()construction test using the combined message).Fix: collapse this into a single check living only in
__init__(removing the duplicate fromconnect()), so there's one source of truth. Prefer keeping the more specific per-fieldwording (nicer error messages), so
__init__would raise"Cloud provider is required when endpoint is not provided"/"Cloud region is required when endpoint is not provided"as appropriate. Tests that currentlypin the combined
__init__-specific message will need updating to match.Scope
Both are
InterfaceErrormessage/placement changes only, no behavioral change forconnect()callers (who already got these errors) -- the only newly-affected callers are those constructing
Connectiondirectly.