jason810496 commented on code in PR #59339:
URL: https://github.com/apache/airflow/pull/59339#discussion_r2625560498
##########
airflow-core/tests/unit/models/test_connection.py:
##########
@@ -148,6 +148,17 @@ def clear_fernet_cache(self):
None,
r"Invalid connection string:
type://user:pass@protocol://host:port?param=value.",
),
+ (
+
"type://host?int_param=123&bool_param=true&float_param=1.5&str_param=some_str",
+ "type",
+ "host",
+ None,
+ None,
+ None,
+ "",
+ {"int_param": 123, "bool_param": True, "float_param": 1.5,
"str_param": "some_str"},
+ None,
+ ),
Review Comment:
Could we add another case that include `__extra__` in the URI to check
whether the `parse_from_uri` are still able to parse the new encoding logic?
##########
airflow-core/src/airflow/models/connection.py:
##########
@@ -331,14 +335,23 @@ def get_uri(self) -> str:
if self.extra:
try:
- query: str | None = urlencode(self.extra_dejson)
- except TypeError:
- query = None
- if query and self.extra_dejson == dict(parse_qsl(query,
keep_blank_values=True)):
- uri += ("?" if self.schema else "/?") + query
- else:
+ extra_dict = self.extra_dejson
+ can_flatten = True
+ for _, value in extra_dict.items():
+ if not isinstance(value, str):
+ can_flatten = False
+ break
Review Comment:
How about moving the `can_flatten` logic before the `try` block, as it
_should_ not raise any `TypeError, AttributeError` exception.
##########
airflow-core/src/airflow/models/connection.py:
##########
@@ -457,14 +470,14 @@ def get_extra_dejson(self, nested: bool = False) -> dict:
if self.extra:
try:
+ extra = json.loads(self.extra)
if nested:
- for key, value in json.loads(self.extra).items():
- extra[key] = value
+ for key, value in extra.items():
if isinstance(value, str):
- with suppress(JSONDecodeError):
+ try:
extra[key] = json.loads(value)
- else:
- extra = json.loads(self.extra)
+ except (JSONDecodeError, TypeError):
+ pass
Review Comment:
Sorry for pointing you in the wrong direction in my last review.
After double-checking, we don’t need to change `get_extra_dejson` at all.
##########
airflow-core/src/airflow/models/connection.py:
##########
@@ -331,14 +335,23 @@ def get_uri(self) -> str:
if self.extra:
try:
- query: str | None = urlencode(self.extra_dejson)
- except TypeError:
- query = None
- if query and self.extra_dejson == dict(parse_qsl(query,
keep_blank_values=True)):
- uri += ("?" if self.schema else "/?") + query
- else:
+ extra_dict = self.extra_dejson
+ can_flatten = True
+ for _, value in extra_dict.items():
+ if not isinstance(value, str):
+ can_flatten = False
+ break
+
+ if can_flatten:
+ query = urlencode(extra_dict)
+ if extra_dict == dict(parse_qsl(query,
keep_blank_values=True)):
+ uri += ("?" if self.schema else "/?") + query
+ else:
+ uri += ("?" if self.schema else "/?") +
urlencode({self.EXTRA_KEY: self.extra})
+ else:
+ uri += ("?" if self.schema else "/?") +
urlencode({self.EXTRA_KEY: self.extra})
+ except (TypeError, AttributeError):
Review Comment:
It seems the whole flow could be simplify as:
```python
can_flatten = True
# the for loop to check whether could we flatten
try:
query: str | None = urlencode(self.extra_dejson)
except TypeError:
query = None
# we just need to add can_flatten condition check
if can_flatten and query and self.extra_dejson == dict(parse_qsl(query,
keep_blank_values=True)):
uri += ("?" if self.schema else "/?") + query
else:
# fallback to nested EXTRA_KEY
uri += ("?" if self.schema else "/?") + urlencode({self.EXTRA_KEY:
self.extra})
```
which also minimized the change.
--
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]
