Skip to content

Commit ef14caf

Browse files
refactor: Simplify Core connection validation per PR feedback
- Remove engine_name and account_name validation checks from _validate_core_connection - Keep only credentials validation since FireboltCore auth handles other parameters - Update unit tests to reflect new validation behavior - Address GitHub comment from ptiurin requesting simplified validation Co-Authored-By: [email protected] <[email protected]>
1 parent 01b4eb4 commit ef14caf

File tree

2 files changed

+13
-21
lines changed

2 files changed

+13
-21
lines changed

src/firebolt_db/firebolt_dialect.py

Lines changed: 1 addition & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -170,19 +170,12 @@ def create_connect_args(self, url: URL) -> Tuple[List, Dict]:
170170
def _validate_core_connection(self, url: URL, parameters: Dict[str, str]) -> None:
171171
"""Validate that Core connection parameters are correct.
172172
173-
Note: In SQLAlchemy URL structure, url.database maps to engine_name
174-
and url.host maps to database name in Firebolt context.
173+
Only validates credentials since FireboltCore auth handles other parameters.
175174
"""
176175
if url.username or url.password:
177176
raise ArgumentError(
178177
"Core connections do not support username/password authentication"
179178
)
180-
if url.database:
181-
raise ArgumentError("Core connections do not support engine_name parameter")
182-
if "account_name" in parameters:
183-
raise ArgumentError(
184-
"Core connections do not support account_name parameter"
185-
)
186179

187180
def _parse_token_cache_flag(self, parameters: Dict[str, str]) -> bool:
188181
"""Parse and remove token cache flag from parameters."""

tests/unit/test_firebolt_dialect.py

Lines changed: 12 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -352,30 +352,29 @@ def test_create_connect_args_core_with_credentials_error(
352352
):
353353
dialect.create_connect_args(u)
354354

355-
def test_create_connect_args_core_with_engine_error(self, dialect: FireboltDialect):
355+
def test_create_connect_args_core_with_engine_allowed(self, dialect: FireboltDialect):
356+
"""Test that Core connections now allow engine_name parameter."""
356357
connection_url = (
357358
"test_engine://test_db_name/test_engine?url=http://localhost:8080"
358359
)
359360
u = url.make_url(connection_url)
361+
362+
result_list, result_dict = dialect.create_connect_args(u)
363+
assert result_dict["engine_name"] == "test_engine"
364+
assert result_dict["url"] == "http://localhost:8080"
360365

361-
with raises(
362-
ArgumentError, match="Core connections do not support engine_name parameter"
363-
):
364-
dialect.create_connect_args(u)
365-
366-
def test_create_connect_args_core_with_account_error(
366+
def test_create_connect_args_core_with_account_allowed(
367367
self, dialect: FireboltDialect
368368
):
369+
"""Test that Core connections now allow account_name parameter."""
369370
connection_url = (
370371
"test_engine://test_db_name?url=http://localhost:8080&account_name=test"
371372
)
372373
u = url.make_url(connection_url)
373-
374-
with raises(
375-
ArgumentError,
376-
match="Core connections do not support account_name parameter",
377-
):
378-
dialect.create_connect_args(u)
374+
375+
result_list, result_dict = dialect.create_connect_args(u)
376+
assert result_dict["account_name"] == "test"
377+
assert result_dict["url"] == "http://localhost:8080"
379378

380379

381380
def test_get_is_nullable():

0 commit comments

Comments
 (0)