betodealmeida commented on code in PR #30081:
URL: https://github.com/apache/superset/pull/30081#discussion_r1797319594
##########
superset/db_engine_specs/trino.py:
##########
@@ -60,11 +62,26 @@
logger = logging.getLogger(__name__)
+class CustomTrinoAuthErrorMeta(type):
+ def __instancecheck__(cls, instance: object) -> bool:
+ if isinstance(instance, HttpError):
+ return "error 401: b'Invalid credentials'" in str(instance)
+ return False
Review Comment:
Nice, I like this approach!
You can simplify this to:
```python
return isinstance(instance, HttpError) and "error 401: b'Invalid
credentials'" in str(instance)
```
##########
tests/unit_tests/db_engine_specs/test_trino.py:
##########
@@ -784,3 +793,57 @@ def test_where_latest_partition(
)
== f"""SELECT * FROM table \nWHERE partition_key = {expected_value}"""
)
+
+
+@pytest.fixture
+def oauth2_config() -> OAuth2ClientConfig:
+ """
+ Config for GSheets OAuth2.
Review Comment:
```suggestion
Config for Trino OAuth2.
```
##########
superset/utils/oauth2.py:
##########
@@ -192,3 +192,8 @@ class OAuth2ClientConfigSchema(Schema):
)
authorization_request_uri = fields.String(required=True)
token_request_uri = fields.String(required=True)
+ request_content_type = fields.String(
Review Comment:
Right, I think the idea is that DB engine specs can have a default content
type (defined in the `oauth2_token_request_type` class attribute), but it can
be overridden on a per-database basis by setting it in the `encrypted_extra`.
##########
superset/db_engine_specs/base.py:
##########
@@ -552,18 +556,16 @@ def get_oauth2_token(
"""
timeout = current_app.config["DATABASE_OAUTH2_TIMEOUT"].total_seconds()
uri = config["token_request_uri"]
- response = requests.post(
- uri,
- json={
- "code": code,
- "client_id": config["id"],
- "client_secret": config["secret"],
- "redirect_uri": config["redirect_uri"],
- "grant_type": "authorization_code",
- },
- timeout=timeout,
- )
- return response.json()
+ req_body = {
+ "code": code,
+ "client_id": config["id"],
+ "client_secret": config["secret"],
+ "redirect_uri": config["redirect_uri"],
+ "grant_type": "authorization_code",
+ }
+ if config["request_content_type"] == "data":
+ return requests.post(uri, data=req_body, timeout=timeout).json()
+ return requests.post(uri, json=req_body, timeout=timeout).json()
Review Comment:
Nice!
I think that the most common (standard?) workflow is to send form encoded
data, and not JSON. It's just that the first OAuth2 implementation done for
Superset targeted GSheets, and Google uses JSON instead of form encoded (it's
the same for BigQuery, for example). But now changing the default would break
existing databases, so we need to leave JSON as the default.
##########
superset/db_engine_specs/trino.py:
##########
@@ -60,11 +62,26 @@
logger = logging.getLogger(__name__)
+class CustomTrinoAuthErrorMeta(type):
+ def __instancecheck__(cls, instance: object) -> bool:
+ if isinstance(instance, HttpError):
+ return "error 401: b'Invalid credentials'" in str(instance)
+ return False
+
+
+class TrinoAuthError(HttpError, metaclass=CustomTrinoAuthErrorMeta):
+ pass
+
+
class TrinoEngineSpec(PrestoBaseEngineSpec):
engine = "trino"
engine_name = "Trino"
allows_alias_to_source_column = False
+ # OAuth 2.0
+ supports_oauth2 = True
+ oauth2_exception = TrinoAuthError
Review Comment:
Let's also add here:
```python
oauth2_token_request_type = "data"
```
--
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: notifications-unsubscr...@superset.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscr...@superset.apache.org
For additional commands, e-mail: notifications-h...@superset.apache.org
