Copilot commented on code in PR #64591:
URL: https://github.com/apache/airflow/pull/64591#discussion_r3069215894
##########
providers/zendesk/tests/unit/zendesk/hooks/test_zendesk.py:
##########
@@ -83,3 +224,46 @@ def test_delete_tickets(self):
with patch.object(zenpy_client.tickets, "delete") as search_mock:
self.hook.delete_tickets(ticket, extra_parameter="extra_parameter")
search_mock.assert_called_once_with(ticket,
extra_parameter="extra_parameter")
+
+ def test_hook_init_with_token_flag(self):
+ conn_id = "zendesk_token_flag"
+ conn = Connection(
+ conn_id=conn_id,
+ conn_type="zendesk",
+ host="yoursubdomain.zendesk.com",
+ login="[email protected]",
+ password="my_api_token",
+ extra={"use_token": True},
+ )
+ with
patch("airflow.providers.zendesk.hooks.zendesk.ZendeskHook.get_connection",
return_value=conn):
+ hook = ZendeskHook(zendesk_conn_id=conn_id)
+ zenpy_client = hook.get_conn()
+ assert zenpy_client.users.session.auth == ("[email protected]/token",
"my_api_token")
+
+ def test_hook_init_with_direct_token(self):
+ conn_id = "zendesk_direct_token"
+ conn = Connection(
+ conn_id=conn_id,
+ conn_type="zendesk",
+ host="yoursubdomain.zendesk.com",
+ login="[email protected]",
+ extra={"token": "direct_token"},
+ )
+ with
patch("airflow.providers.zendesk.hooks.zendesk.ZendeskHook.get_connection",
return_value=conn):
Review Comment:
Same issue as above: `Connection.extra` should be a JSON string, not a dict.
Passing a dict here will fail the Connection extra validation/parsing path.
##########
providers/zendesk/tests/unit/zendesk/hooks/test_zendesk.py:
##########
@@ -83,3 +224,46 @@ def test_delete_tickets(self):
with patch.object(zenpy_client.tickets, "delete") as search_mock:
self.hook.delete_tickets(ticket, extra_parameter="extra_parameter")
search_mock.assert_called_once_with(ticket,
extra_parameter="extra_parameter")
+
+ def test_hook_init_with_token_flag(self):
+ conn_id = "zendesk_token_flag"
+ conn = Connection(
+ conn_id=conn_id,
+ conn_type="zendesk",
+ host="yoursubdomain.zendesk.com",
+ login="[email protected]",
+ password="my_api_token",
+ extra={"use_token": True},
+ )
Review Comment:
These tests construct `Connection(extra={...})` with a dict, but
`Connection.extra` expects a JSON string (it is encrypted/validated in the
setter and later parsed via `json.loads`). Using a dict here will raise during
construction or when accessing `extra_dejson`. Use `extra=json.dumps({...})`
(or the same pattern used earlier in this file) so the patched
`get_connection()` behaves like a real Connection object.
##########
providers/zendesk/tests/unit/zendesk/hooks/test_zendesk.py:
##########
@@ -83,3 +224,46 @@ def test_delete_tickets(self):
with patch.object(zenpy_client.tickets, "delete") as search_mock:
self.hook.delete_tickets(ticket, extra_parameter="extra_parameter")
search_mock.assert_called_once_with(ticket,
extra_parameter="extra_parameter")
+
+ def test_hook_init_with_token_flag(self):
+ conn_id = "zendesk_token_flag"
+ conn = Connection(
+ conn_id=conn_id,
+ conn_type="zendesk",
+ host="yoursubdomain.zendesk.com",
+ login="[email protected]",
+ password="my_api_token",
+ extra={"use_token": True},
+ )
+ with
patch("airflow.providers.zendesk.hooks.zendesk.ZendeskHook.get_connection",
return_value=conn):
+ hook = ZendeskHook(zendesk_conn_id=conn_id)
+ zenpy_client = hook.get_conn()
+ assert zenpy_client.users.session.auth == ("[email protected]/token",
"my_api_token")
+
+ def test_hook_init_with_direct_token(self):
+ conn_id = "zendesk_direct_token"
+ conn = Connection(
+ conn_id=conn_id,
+ conn_type="zendesk",
+ host="yoursubdomain.zendesk.com",
+ login="[email protected]",
+ extra={"token": "direct_token"},
+ )
+ with
patch("airflow.providers.zendesk.hooks.zendesk.ZendeskHook.get_connection",
return_value=conn):
+ hook = ZendeskHook(zendesk_conn_id=conn_id)
+ zenpy_client = hook.get_conn()
+ assert zenpy_client.users.session.auth == ("[email protected]/token",
"direct_token")
+
+ def test_hook_init_with_oauth_token(self):
+ conn_id = "zendesk_oauth_token"
+ conn = Connection(
+ conn_id=conn_id,
+ conn_type="zendesk",
+ host="yoursubdomain.zendesk.com",
+ login="[email protected]",
+ extra={"oauth_token": "my_oauth_token"},
+ )
+ with
patch("airflow.providers.zendesk.hooks.zendesk.ZendeskHook.get_connection",
return_value=conn):
Review Comment:
Same issue as above: `Connection.extra` should be a JSON string. Use
`extra=json.dumps({"oauth_token": ...})` (or similar) so `extra_dejson` behaves
correctly in the hook.
##########
providers/zendesk/src/airflow/providers/zendesk/hooks/zendesk.py:
##########
@@ -44,53 +60,105 @@ class ZendeskHook(BaseHook):
@classmethod
def get_ui_field_behaviour(cls) -> dict[str, Any]:
return {
- "hidden_fields": ["schema", "port", "extra"],
- "relabeling": {"host": "Zendesk domain", "login": "Zendesk email"},
+ "hidden_fields": ["schema", "port"],
+ "relabeling": {
+ "host": "Zendesk domain",
+ "login": "Zendesk email",
+ "password": "Password / API token",
+ },
}
def __init__(self, zendesk_conn_id: str = default_conn_name) -> None:
super().__init__()
self.zendesk_conn_id = zendesk_conn_id
self.base_api: BaseApi | None = None
- zenpy_client, url = self._init_conn()
- self.zenpy_client = zenpy_client
- self.__url = url
- self.get = self.zenpy_client.users._get
+ self._zenpy_client: Zenpy | None = None
+ self._url: str = ""
def _init_conn(self) -> tuple[Zenpy, str]:
"""
- Create the Zenpy Client for our Zendesk connection.
+ Create the Zenpy Client for the Zendesk connection.
- :return: zenpy.Zenpy client and the url for the API.
+ Parses the host into ``domain`` and (optionally) ``subdomain`` for
Zenpy.
+ For example, ``yoursubdomain.zendesk.com`` produces
+ ``domain="zendesk.com"`` and ``subdomain="yoursubdomain"``.
+
+ Authentication kwargs are resolved from Connection extras according to
+ the precedence documented on the class docstring.
+
+ :return: (zenpy.Zenpy client, base URL string)
+ :raises ValueError: if the host is missing or has an invalid format.
"""
conn = self.get_connection(self.zendesk_conn_id)
- domain = ""
- url = ""
- subdomain: str | None = None
- if conn.host:
- url = "https://"; + conn.host
- domain = conn.host
- if conn.host.count(".") >= 2:
- dot_splitted_string = conn.host.rsplit(".", 2)
- subdomain = dot_splitted_string[0]
- domain = ".".join(dot_splitted_string[1:])
- return Zenpy(domain=domain, subdomain=subdomain, email=conn.login,
password=conn.password), url
+
+ if not conn.host:
+ raise ValueError(
+ f"No host provided for connection '{self.zendesk_conn_id}'. "
+ "Set the host to your Zendesk domain, e.g.
'yoursubdomain.zendesk.com'."
+ )
+
+ # Parse host into subdomain + domain.
+ # Accepted formats: "sub.zendesk.com" → subdomain="sub",
domain="zendesk.com"
+ # "zendesk.com" → subdomain=None,
domain="zendesk.com"
+ # "zendesk" → invalid (needs at least one
dot)
+ parts = conn.host.rsplit(".", 2)
+ if len(parts) < 2:
+ raise ValueError(
+ f"Invalid host format '{conn.host}' for connection
'{self.zendesk_conn_id}'. "
+ "Expected a domain with at least one dot, e.g.
'yoursubdomain.zendesk.com'."
+ )
+ domain = ".".join(parts[-2:])
+ subdomain: str | None = parts[0] if len(parts) == 3 else None
+ url = "https://"; + conn.host
+
+ extra = conn.extra_dejson
+ kwargs: dict[str, Any] = {
+ "domain": domain,
+ "subdomain": subdomain,
+ "email": conn.login,
+ }
+
+ if extra.get("use_token"):
+ # Treat the password field as an API token.
+ kwargs["token"] = conn.password
+ elif extra.get("token"):
+ # API token stored directly in extras.
+ kwargs["token"] = extra["token"]
+ elif extra.get("oauth_token"):
+ # OAuth token stored in extras.
+ kwargs["oauth_token"] = extra["oauth_token"]
+ else:
+ # Legacy password-based auth (deprecated by Zendesk).
Review Comment:
In `_init_conn()`, `email` is always included in `kwargs` (set to
`conn.login`), even for OAuth auth. This contradicts the docstring stating
`login` is not required for OAuth and can also break OAuth connections where
`login` is unset (passing `email=None` into `Zenpy`). Consider only setting
`email` when using password/token auth, and explicitly validating that
`conn.login` is present for those modes (raising a clear `ValueError`).
```suggestion
}
if extra.get("use_token"):
# Treat the password field as an API token.
if not conn.login:
raise ValueError(
f"No login provided for connection
'{self.zendesk_conn_id}'. "
"The login field must be set to your Zendesk email
address when using API token "
"authentication."
)
kwargs["email"] = conn.login
kwargs["token"] = conn.password
elif extra.get("token"):
# API token stored directly in extras.
if not conn.login:
raise ValueError(
f"No login provided for connection
'{self.zendesk_conn_id}'. "
"The login field must be set to your Zendesk email
address when using API token "
"authentication."
)
kwargs["email"] = conn.login
kwargs["token"] = extra["token"]
elif extra.get("oauth_token"):
# OAuth token stored in extras.
kwargs["oauth_token"] = extra["oauth_token"]
else:
# Legacy password-based auth (deprecated by Zendesk).
if not conn.login:
raise ValueError(
f"No login provided for connection
'{self.zendesk_conn_id}'. "
"The login field must be set to your Zendesk email
address when using password "
"authentication."
)
kwargs["email"] = conn.login
```
--
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]
