Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
jscheffl commented on PR #54034: URL: https://github.com/apache/airflow/pull/54034#issuecomment-3165993882 Cool! Thanks for the contribution! -- 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]
Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
pierrejeambrun commented on PR #54034: URL: https://github.com/apache/airflow/pull/54034#issuecomment-3164826751 manual backport https://github.com/apache/airflow/pull/54235 cc: @potiuk -- 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]
Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
potiuk merged PR #54034: URL: https://github.com/apache/airflow/pull/54034 -- 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]
Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
bugraoz93 commented on PR #54034:
URL: https://github.com/apache/airflow/pull/54034#issuecomment-3161395336
Yes, I agree with Jens here and with comments from Pierre and Liu Zhe You.
This is exactly what I mentioned: we should set it as either `{}` for the
default empty JSON or None for sure but a unified way of handling it. I jumped
in a bit later stage again but looks good, thanks for the changes and for your
time!
--
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]
Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
Prasanth345 commented on PR #54034: URL: https://github.com/apache/airflow/pull/54034#issuecomment-3160721475 @jason810496 fixed the static check errors -- 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]
Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
Prasanth345 commented on PR #54034:
URL: https://github.com/apache/airflow/pull/54034#issuecomment-3154979238
@pierrejeambrun @jason810496
Addressed backward compatibility for `extra` field by handling empty string
`("")` as `{}` in serializers.
Added validation to ensure `extra` is a valid JSON object and wrote backend
tests to verify acceptance of empty string.
Also tested changes using `airflow connections add `
--
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]
Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
Prasanth345 commented on code in PR #54034:
URL: https://github.com/apache/airflow/pull/54034#discussion_r2254134040
##
airflow-core/src/airflow/api_fastapi/logging/decorators.py:
##
@@ -38,11 +38,14 @@ def _mask_connection_fields(extra_fields):
for k, v in extra_fields.items():
if k == "extra" and v:
try:
-extra = json.loads(v)
-extra = {k: secrets_masker.redact(v, k) for k, v in
extra.items()}
-result[k] = dict(extra)
+parsed_extra = json.loads(v)
+if isinstance(parsed_extra, dict):
+masked_extra = {ek: secrets_masker.redact(ev, ek) for ek,
ev in parsed_extra.items()}
+result[k] = masked_extra
+else:
+result[k] = "Expected JSON object in extra field, got
non-dict JSON"
Review Comment:
updated the text
--
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]
Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
Prasanth345 commented on code in PR #54034:
URL: https://github.com/apache/airflow/pull/54034#discussion_r2254133545
##
airflow-core/src/airflow/api_fastapi/logging/decorators.py:
##
@@ -38,11 +38,14 @@ def _mask_connection_fields(extra_fields):
for k, v in extra_fields.items():
if k == "extra" and v:
try:
-extra = json.loads(v)
-extra = {k: secrets_masker.redact(v, k) for k, v in
extra.items()}
-result[k] = dict(extra)
+parsed_extra = json.loads(v)
+if isinstance(parsed_extra, dict):
+masked_extra = {ek: secrets_masker.redact(ev, ek) for ek,
ev in parsed_extra.items()}
+result[k] = masked_extra
+else:
+result[k] = "Expected JSON object in extra field, got
non-dict JSON"
except json.JSONDecodeError:
-result[k] = "Encountered non-JSON in `extra` field"
+result[k] = "Encountered non-JSON in extra field"
Review Comment:
updated the text
--
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]
Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
Prasanth345 commented on code in PR #54034:
URL: https://github.com/apache/airflow/pull/54034#discussion_r2254132060
##
airflow-core/src/airflow/api_fastapi/core_api/datamodels/connections.py:
##
@@ -136,3 +136,25 @@ class ConnectionBody(StrictBaseModel):
port: int | None = Field(default=None)
password: str | None = Field(default=None)
extra: str | None = Field(default=None)
+
+@field_validator("extra")
+@classmethod
+def validate_extra(cls, v: str | None) -> str | None:
+"""
+Validate that `extra` is a JSON-encoded Python dict.
+
+If `extra` is not a valid JSON, it will be returned as is.
+"""
+if v is None:
+return v
+try:
+extra_dict = json.loads(v)
+if not isinstance(extra_dict, dict):
+raise ValueError("Extra must be a valid JSON object (e.g.,
{'key': 'value'})")
Review Comment:
updated the validation message
##
airflow-core/src/airflow/api_fastapi/core_api/datamodels/connections.py:
##
@@ -136,3 +136,25 @@ class ConnectionBody(StrictBaseModel):
port: int | None = Field(default=None)
password: str | None = Field(default=None)
extra: str | None = Field(default=None)
+
+@field_validator("extra")
+@classmethod
+def validate_extra(cls, v: str | None) -> str | None:
+"""
+Validate that `extra` is a JSON-encoded Python dict.
+
+If `extra` is not a valid JSON, it will be returned as is.
+"""
+if v is None:
+return v
+try:
+extra_dict = json.loads(v)
+if not isinstance(extra_dict, dict):
+raise ValueError("Extra must be a valid JSON object (e.g.,
{'key': 'value'})")
+except json.JSONDecodeError:
+# If it's not a valid JSON, we can't redact it, so return as is
+raise ValueError(
+"Extra must be a valid JSON object (e.g., {'key': 'value'}), "
Review Comment:
updated the validation message
--
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]
Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
Prasanth345 commented on code in PR #54034:
URL: https://github.com/apache/airflow/pull/54034#discussion_r2254130293
##
airflow-core/src/airflow/api_fastapi/core_api/datamodels/connections.py:
##
@@ -136,3 +136,25 @@ class ConnectionBody(StrictBaseModel):
port: int | None = Field(default=None)
password: str | None = Field(default=None)
extra: str | None = Field(default=None)
+
+@field_validator("extra")
+@classmethod
+def validate_extra(cls, v: str | None) -> str | None:
+"""
+Validate that `extra` is a JSON-encoded Python dict.
+
+If `extra` is not a valid JSON, it will be returned as is.
+"""
+if v is None:
+return v
+try:
+extra_dict = json.loads(v)
+if not isinstance(extra_dict, dict):
+raise ValueError("Extra must be a valid JSON object (e.g.,
{'key': 'value'})")
+except json.JSONDecodeError:
+# If it's not a valid JSON, we can't redact it, so return as is
Review Comment:
removed that comment
--
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]
Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
jason810496 commented on code in PR #54034:
URL: https://github.com/apache/airflow/pull/54034#discussion_r2251997801
##
airflow-core/src/airflow/api_fastapi/logging/decorators.py:
##
@@ -38,11 +38,14 @@ def _mask_connection_fields(extra_fields):
for k, v in extra_fields.items():
if k == "extra" and v:
try:
-extra = json.loads(v)
-extra = {k: secrets_masker.redact(v, k) for k, v in
extra.items()}
-result[k] = dict(extra)
+parsed_extra = json.loads(v)
+if isinstance(parsed_extra, dict):
+masked_extra = {ek: secrets_masker.redact(ev, ek) for ek,
ev in parsed_extra.items()}
+result[k] = masked_extra
+else:
+result[k] = "Expected JSON object in extra field, got
non-dict JSON"
Review Comment:
```suggestion
result[k] = "Expected JSON object in `extra` field, got
non-dict JSON"
```
##
airflow-core/src/airflow/api_fastapi/logging/decorators.py:
##
@@ -38,11 +38,14 @@ def _mask_connection_fields(extra_fields):
for k, v in extra_fields.items():
if k == "extra" and v:
try:
-extra = json.loads(v)
-extra = {k: secrets_masker.redact(v, k) for k, v in
extra.items()}
-result[k] = dict(extra)
+parsed_extra = json.loads(v)
+if isinstance(parsed_extra, dict):
+masked_extra = {ek: secrets_masker.redact(ev, ek) for ek,
ev in parsed_extra.items()}
+result[k] = masked_extra
+else:
+result[k] = "Expected JSON object in extra field, got
non-dict JSON"
except json.JSONDecodeError:
-result[k] = "Encountered non-JSON in `extra` field"
+result[k] = "Encountered non-JSON in extra field"
Review Comment:
```suggestion
result[k] = "Encountered non-JSON in `extra` field"
```
##
airflow-core/src/airflow/api_fastapi/core_api/datamodels/connections.py:
##
@@ -136,3 +136,25 @@ class ConnectionBody(StrictBaseModel):
port: int | None = Field(default=None)
password: str | None = Field(default=None)
extra: str | None = Field(default=None)
+
+@field_validator("extra")
+@classmethod
+def validate_extra(cls, v: str | None) -> str | None:
+"""
+Validate that `extra` is a JSON-encoded Python dict.
+
+If `extra` is not a valid JSON, it will be returned as is.
+"""
+if v is None:
+return v
+try:
+extra_dict = json.loads(v)
+if not isinstance(extra_dict, dict):
+raise ValueError("Extra must be a valid JSON object (e.g.,
{'key': 'value'})")
Review Comment:
```suggestion
raise ValueError("The `extra` field must be a valid JSON
object (e.g., {'key': 'value'})")
```
##
airflow-core/src/airflow/api_fastapi/core_api/datamodels/connections.py:
##
@@ -136,3 +136,25 @@ class ConnectionBody(StrictBaseModel):
port: int | None = Field(default=None)
password: str | None = Field(default=None)
extra: str | None = Field(default=None)
+
+@field_validator("extra")
+@classmethod
+def validate_extra(cls, v: str | None) -> str | None:
+"""
+Validate that `extra` is a JSON-encoded Python dict.
+
+If `extra` is not a valid JSON, it will be returned as is.
+"""
+if v is None:
+return v
+try:
+extra_dict = json.loads(v)
+if not isinstance(extra_dict, dict):
+raise ValueError("Extra must be a valid JSON object (e.g.,
{'key': 'value'})")
+except json.JSONDecodeError:
+# If it's not a valid JSON, we can't redact it, so return as is
+raise ValueError(
+"Extra must be a valid JSON object (e.g., {'key': 'value'}), "
Review Comment:
```suggestion
"The `extra` field must be a valid JSON object (e.g.,
{'key': 'value'}), "
```
--
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]
Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
pierrejeambrun commented on code in PR #54034:
URL: https://github.com/apache/airflow/pull/54034#discussion_r2251830613
##
airflow-core/src/airflow/api_fastapi/core_api/datamodels/connections.py:
##
@@ -136,3 +136,25 @@ class ConnectionBody(StrictBaseModel):
port: int | None = Field(default=None)
password: str | None = Field(default=None)
extra: str | None = Field(default=None)
+
+@field_validator("extra")
+@classmethod
+def validate_extra(cls, v: str | None) -> str | None:
+"""
+Validate that `extra` is a JSON-encoded Python dict.
+
+If `extra` is not a valid JSON, it will be returned as is.
+"""
+if v is None:
+return v
+try:
+extra_dict = json.loads(v)
+if not isinstance(extra_dict, dict):
+raise ValueError("Extra must be a valid JSON object (e.g.,
{'key': 'value'})")
+except json.JSONDecodeError:
+# If it's not a valid JSON, we can't redact it, so return as is
Review Comment:
Comment is not related to code.
--
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]
Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
akshayvijayjain commented on PR #54034:
URL: https://github.com/apache/airflow/pull/54034#issuecomment-3149969946
@jscheffl @shubhamraj-git @bugraoz93
Thank you for inputs; what we understood is we have to allow support for ''
in api to support backward compatibility for ctl.
so we should convert '' to {} before storing in DB
while from UI; we could restrict even '' | OR | to be in sync till airflow
4; we can allow '' from UI also; and since api already converting it to {}
Now; we also need to add deprecation log in
1. api call
2. CTL
Please confirm if our approach is okay!
--
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]
Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
jscheffl commented on PR #54034:
URL: https://github.com/apache/airflow/pull/54034#issuecomment-3148564630
Coming along here after being a bit off from PC: I think only valid JSON is
of any use in providers. I am not sure if an empty string is used in the
providers... which would be against the idea of the extras.
To come around the problem of "legacy data" where potentially empty strings
are used... I'd propose to ignore just empty strings and either convert them to
`{}` or `None` implicitly. Then we can keep the validation and maybe just allow
an empty string for historic reasons, post a warning and ignore the content.
Support of this could be dropped in Airflow 4.
--
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]
Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
shubhamraj-git commented on PR #54034: URL: https://github.com/apache/airflow/pull/54034#issuecomment-3146677833 ohh, you meant to transform the empty string to empty json upon migration and not allow creation with empty string via CLI to sync with the behaviour of UI. Yes, you are right, this could potentially cause a problem in automations. -- 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]
Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
bugraoz93 commented on PR #54034: URL: https://github.com/apache/airflow/pull/54034#issuecomment-3146667957 > @bugraoz93 I think it would be hard to drop the functionality to support empty string, since users might have a lot of connection and might end up getting failed imports, which could be a problem. We already had some [issue](https://github.com/apache/airflow/issues/49734) reported in 3.0.0, I think we can give an exception to the empty string, anyways we are supporting to edit an empty string in extra due to the issue mentioned. I haven't meant to drop supporting it but rather storing in the database in a unified way. I support backwards compatibility importing from older versions to newer versions for a smooth transition but this doesn't necessarily mean they will be persisted as the default empty string. This means export from a newer version can work on newer versions. It could be even invisible to users if they aren't reading newsletters before updates or following up on database changes. If there is a lot of automation on it across users, we may be even more relaxed about it, I am not against that. -- 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]
Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
shubhamraj-git commented on PR #54034: URL: https://github.com/apache/airflow/pull/54034#issuecomment-3146656607 @bugraoz93 I think it would be hard to drop the functionality to support empty string, since users might have a lot of connection and might end up getting failed imports, which could be a problem. We already had some [issue](https://github.com/apache/airflow/issues/49734) reported in 3.0.0, I think we can give an exception to the empty string, anyways we are supporting to edit an empty string in extra due to the issue mentioned. -- 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]
Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
bugraoz93 commented on PR #54034: URL: https://github.com/apache/airflow/pull/54034#issuecomment-3146130902 If this won't be supported from UI, we may consider dropping the feature from airflow CLI, since if those are imported from CLI, they can end up with some API/UI problems while updates and people can think, this is allowed. If it isn't supported by the API/UI, this shouldn't be allowed from the CLI either, where we can also add small things there if it wasn't deprecated. While more isolation is ongoing, it can be easier to follow up a single source of truth for these kinds of database activities -- 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]
Re: [PR] Fix: Validate and handle invalid `extra` field in connections UI and API (#53963) [airflow]
bugraoz93 commented on PR #54034: URL: https://github.com/apache/airflow/pull/54034#issuecomment-3146127526 > Thanks for the PR. > > Airflow 2.x supported "" in extra parameter for connection and is still supported from the airflow cli. Since upcoming airflowctl would use APIs, I think problem would come if someone tries to import connections from older version of airflow, breaking compatibility. We might need to handle that. > > cc: @bugraoz93 @pierrejeambrun Thanks for flagging @shubhamraj-git! This could indeed be a problem and needs to be handled. This will highly likely reduce the migration effort too if we have backwards compatibility and allow users to migrate their connections to v3. **TLDR** `airflowctl` We can parse them to JSON before sending them to the API if an empty string is passed, to provide backwards compatibility in CTL. We should warn users that an empty string for extras would end up as JSON. I don't think this has to be included in the first release, as the airflow CLI will still support it. While deprecating duplicate functionalities, we have to implement it at least before deprecating the airflow CLI over airflowctl. It is also a small addition where we can add it. I will follow up with this -- 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]
