rusackas commented on code in PR #36856:
URL: https://github.com/apache/superset/pull/36856#discussion_r3564844566
##########
superset/db_engine_specs/snowflake.py:
##########
@@ -33,19 +33,45 @@
from sqlalchemy import types
from sqlalchemy.engine.reflection import Inspector
from sqlalchemy.engine.url import URL
+from sqlalchemy.exc import DatabaseError as SqlalchemyDatabaseError
+from superset import is_feature_enabled, security_manager
from superset.constants import TimeGrain
from superset.databases.utils import make_url_safe
from superset.db_engine_specs.base import BaseEngineSpec, BasicPropertiesType
from superset.db_engine_specs.postgres import PostgresBaseEngineSpec
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
from superset.models.sql_lab import Query
+from superset.superset_typing import (
+ OAuth2ClientConfig,
+ OAuth2State,
+)
from superset.utils import json
from superset.utils.core import get_user_agent, QuerySource
+from superset.utils.oauth2 import encode_oauth2_state
if TYPE_CHECKING:
from superset.models.core import Database
+try:
+ from snowflake.connector.errors import DatabaseError
+except ImportError:
+ DatabaseError = Exception
Review Comment:
Already addressed — `CustomSnowflakeAuthErrorMeta` has a class docstring
explaining its purpose and the `__instancecheck__`-only caveat.
##########
superset/db_engine_specs/snowflake.py:
##########
@@ -377,6 +468,18 @@ def update_params_from_encrypted_extra(
database: "Database",
params: dict[str, Any],
) -> None:
+ # To use OAuth authentication, a database connection must first be
created using
+ # another authenticator (typically key-pair authentication)
+ # with “Impersonate logged in user” enabled.
+ # Key-pair authentication is used for connection tests,
+ # while OAuth authentication is used when executing actual queries,
+ # such as in SQL Lab or dashboards.
+ # Therefore, when using OAuth authentication, the key-pair
authentication
+ # settings are not loaded, and the connection is established using
OAuth only.
+ connects_args = params.get("connect_args", {})
+ if connects_args.get("authenticator") == "oauth":
Review Comment:
Already addressed — `CustomSnowflakeAuthError` has a one-line docstring
pointing to the metaclass for details.
##########
superset/db_engine_specs/snowflake.py:
##########
@@ -44,12 +46,43 @@
from superset.db_engine_specs.postgres import PostgresBaseEngineSpec
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
from superset.models.sql_lab import Query
+from superset.superset_typing import (
+ OAuth2ClientConfig,
+ OAuth2State,
+)
from superset.utils import json
from superset.utils.core import get_user_agent, QuerySource
+from superset.utils.oauth2 import encode_oauth2_state
if TYPE_CHECKING:
from superset.models.core import Database
+try:
+ from snowflake.connector.errors import DatabaseError
+except ImportError:
+ # Use a distinct sentinel type when snowflake is not installed to avoid
+ # matching unrelated exception types (using `Exception` would be too
broad).
+ class _SnowflakeDatabaseError(Exception):
+ """Sentinel type to stand in for
snowflake.connector.errors.DatabaseError."""
+
+ pass
+
+ DatabaseError = _SnowflakeDatabaseError
+
+
+class CustomSnowflakeAuthErrorMeta(type):
Review Comment:
Already addressed — `CustomSnowflakeAuthErrorMeta` has a class docstring
(added in a later push) explaining what it matches and the isinstance()-only
caveat.
##########
superset/db_engine_specs/snowflake.py:
##########
@@ -44,12 +46,43 @@
from superset.db_engine_specs.postgres import PostgresBaseEngineSpec
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
from superset.models.sql_lab import Query
+from superset.superset_typing import (
+ OAuth2ClientConfig,
+ OAuth2State,
+)
from superset.utils import json
from superset.utils.core import get_user_agent, QuerySource
+from superset.utils.oauth2 import encode_oauth2_state
if TYPE_CHECKING:
from superset.models.core import Database
+try:
+ from snowflake.connector.errors import DatabaseError
+except ImportError:
+ # Use a distinct sentinel type when snowflake is not installed to avoid
+ # matching unrelated exception types (using `Exception` would be too
broad).
+ class _SnowflakeDatabaseError(Exception):
+ """Sentinel type to stand in for
snowflake.connector.errors.DatabaseError."""
+
+ pass
+
+ DatabaseError = _SnowflakeDatabaseError
+
+
+class CustomSnowflakeAuthErrorMeta(type):
+ def __instancecheck__(cls, instance: object) -> bool:
+ return (
+ isinstance(instance, SqlalchemyDatabaseError)
+ and isinstance(cast(SqlalchemyDatabaseError, instance).orig,
DatabaseError)
+ and "Invalid OAuth access token" in str(instance)
+ )
+
+
+class CustomSnowflakeAuthError(DatabaseError,
metaclass=CustomSnowflakeAuthErrorMeta):
+ pass
Review Comment:
Already addressed — `CustomSnowflakeAuthError` has a one-line docstring
pointing to the metaclass for details.
--
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]
