Copilot commented on code in PR #37984:
URL: https://github.com/apache/superset/pull/37984#discussion_r2848895318
##########
tests/unit_tests/db_engine_specs/test_starrocks.py:
##########
@@ -283,3 +284,25 @@ def test_adjust_engine_params_with_catalog(
url, {}, catalog=catalog, schema=schema
)
assert returned_url.database == expected_database
+
+
+@with_feature_flags(IMPERSONATE_WITH_EMAIL_PREFIX=True)
+def test_get_prequeries_with_email_prefix(mocker: MockerFixture) -> None:
+ """Test that get_prequeries uses email prefix when
IMPERSONATE_WITH_EMAIL_PREFIX"""
+ from superset.db_engine_specs.starrocks import StarRocksEngineSpec
+
+ user = mocker.MagicMock()
+ user.email = "[email protected]"
+ mocker.patch(
+ "superset.db_engine_specs.starrocks.security_manager.find_user",
+ return_value=user,
+ )
+
+ database = mocker.MagicMock()
+ database.impersonate_user = True
+ database.url_object = make_url("starrocks://localhost:9030/")
+ database.get_effective_user.return_value = "[email protected]"
+
Review Comment:
This test setup uses `database.get_effective_user.return_value =
"[email protected]"` and `user.email = "[email protected]"`, so the asserted
result only validates splitting an email-like string at '@'. It doesn’t cover
the more important feature behavior where the effective username differs from
the email prefix (e.g. username "alice" with email "[email protected]"
should impersonate as "alice.doe"), which could let regressions slip by.
##########
superset/db_engine_specs/starrocks.py:
##########
@@ -413,6 +415,11 @@ def get_prequeries(
username = database.get_effective_user(database.url_object)
if username:
+ if is_feature_enabled("IMPERSONATE_WITH_EMAIL_PREFIX"):
+ user = security_manager.find_user(username=username)
+ if user and user.email:
+ username = user.email.split("@", 1)[0]
+
Review Comment:
The PR description says the IMPERSONATE_WITH_EMAIL_PREFIX logic is
centralized inside `Database.get_effective_user`, but this change adds the
transformation directly in `StarRocksEngineSpec.get_prequeries`. This
duplicates the existing transformation already performed in
`Database._get_sqla_engine` and risks the two paths drifting; consider moving
the feature-flag/email-prefix logic into `Database.get_effective_user` (or a
shared helper) and letting StarRocks rely on that transformed effective user
instead of re-implementing it here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
