confluentinc / confluentinc/confluent-sql

Push `environment_id` validation into `Connection.__init__`; unify cloud_provider/cloud_region validation there too

Open
#213 0 comments 0 reactions 0 assignees View on GitHub
Dominant language
Python
Stars
6
Forks
1
Avg merge
21h 47m
Merged PRs (30d)
26

Description

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:

```python
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:

```python
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.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.