bnspopi commented on code in PR #62064:
URL: https://github.com/apache/airflow/pull/62064#discussion_r2815410024
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -252,11 +252,28 @@ def account_identifier(self) -> str:
return account_identifier
def get_oauth_token(
- self,
- conn_config: dict | None = None,
- token_endpoint: str | None = None,
- grant_type: str = "refresh_token",
- ) -> str:
+ self,
+ conn_config: dict | None = None,
+ token_endpoint: str | None = None,
+ grant_type: str = "refresh_token",
Review Comment:
Thanks for the update.
The refactor improves clarity in OAuth token generation and avoids incorrect
usage of _get_static_conn_params.
Question: Should we also validate that client_id and client_secret are
present before invoking _get_valid_oauth_token to fail fast?
Otherwise, the changes look correct.
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -532,8 +555,13 @@ def _get_valid_oauth_token(
client_secret=conn_config["client_secret"],
)
- token = response.json()["access_token"]
- expires_in = int(response.json()["expires_in"])
+ response_json = response.json()
+
Review Comment:
Good improvement to OAuth validation logic.
The updated error message for unsupported grant_type is clearer and more
actionable.
Adding early validation for client_id and client_secret is a good fail-fast
improvement and prevents obscure downstream errors during token request.
The change improves robustness without affecting backward compatibility.
LGTM.
##########
providers/snowflake/src/airflow/providers/snowflake/hooks/snowflake.py:
##########
@@ -735,7 +763,8 @@ def run(
else:
sql_list = sql
- if sql_list:
+ if sql_list and any(stmt.strip() for stmt in sql_list):
Review Comment:
Good defensive improvement.
The updated condition ensures that we do not attempt execution when sql_list
contains only empty or whitespace statements.
This prevents unnecessary execution attempts and improves robustness.
One question: since split_statements already filters empty SQL strings, do
we need this additional check, or is this meant as extra safety for manually
provided SQL lists?
--
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]
