Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added get_extra_dejson method with nested parameter in Connection #39811

Merged
merged 1 commit into from
Jun 14, 2024

Conversation

dabla
Copy link
Contributor

@dabla dabla commented May 24, 2024

This new method allows you to specify if you want the nested json as string to be also deserialized. The already existing extra_dejson property delegates to this method with nested set to False, as this is the default behaviour (nested json strings don't get deserialized by default). This is a PR in preparation of #35591, as it has become to big, we're going to split it up in smaller ones. This is one of them.


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@dabla dabla requested review from kaxil, XD-DENG and ashb as code owners May 24, 2024 14:48
@Taragolis
Copy link
Contributor

This new method allows you to specify if you want the nested json as string to be also deserialized.

I don't think we need to support string-JSON into the native JSON object.

This one more convenient way to define JSON

{
    "foo": "bar",
    "spam": {"egg": 1, "baz": "qux"}
}

Rather this one. Which looks artificial and ugly

{
    "foo": "bar",
    "spam": "{\"egg\": 1, \"baz\": \"qux\"}"
}

@dabla
Copy link
Contributor Author

dabla commented May 24, 2024

This new method allows you to specify if you want the nested json as string to be also deserialized.

I don't think we need to support string-JSON into the native JSON object.

This one more convenient way to define JSON

{
    "foo": "bar",
    "spam": {"egg": 1, "baz": "qux"}
}

Rather this one. Which looks artificial and ugly

{
    "foo": "bar",
    "spam": "{\"egg\": 1, \"baz\": \"qux\"}"
}

We’ll need it for PR 35591, as there we split the values being put in extra into different textareas for the http connection, leading to those ugly nested json values in the extra field (which will be hidden).

@Taragolis
Copy link
Contributor

Maybe we should fix it in the UI side rather than try to create workaround?
Connection UI itself provided by FAB, there is unlikely would be FAB in Airflow 3.0.

@dabla
Copy link
Contributor Author

dabla commented May 27, 2024

Maybe we should fix it in the UI side rather than try to create workaround? Connection UI itself provided by FAB, there is unlikely would be FAB in Airflow 3.0.

Indeed, but I don't know if the issue is related to FAB or the CodeMirror widget for the textarea's. To me this was the "cleanest" approache to solve the issue

@dabla
Copy link
Contributor Author

dabla commented Jun 7, 2024

Maybe we should fix it in the UI side rather than try to create workaround? Connection UI itself provided by FAB, there is unlikely would be FAB in Airflow 3.0.

Waht are we going to do about this one? Fixing the Javascript issue won't be easy I think, I don't know where to begin if it's related to FAB (which will probably be gone in 3.0). To me personally, this solutions isn't that ugly, ok it's a by-passing a Javascript issue that might be gone in the future unless it's related to the CodeMirror widget but there there is also a new main version released which isn't yet used in Airflow. I also prefer to see a generic/re-usable solution than to code a specific work around in the HTTP Connection/Hook class. Without this, I won't be able to continue working on the original PR 35591, which also solves the ability to specify a different HTTP authenticator class.

@dabla
Copy link
Contributor Author

dabla commented Jun 7, 2024

We could put a TODO remark on that method and state that if the javascript issue would be solved, this one could be simplified.

…llows you to specify if you want the nested json as string to be also deserialized. The extra_dejson property uses this method with nested set to False.
@potiuk potiuk force-pushed the feature/nested-extra-dejson-connection branch from bb353f4 to 5b97aed Compare June 14, 2024 15:22
@potiuk
Copy link
Member

potiuk commented Jun 14, 2024

Let me rebase and rerun tests to see if nothing changed since.

@potiuk potiuk merged commit ca73694 into apache:main Jun 14, 2024
51 checks passed
@eladkal eladkal added this to the Airflow 2.10.0 milestone Jun 14, 2024
@eladkal eladkal added the type:improvement Changelog: Improvements label Jun 14, 2024
jannisko pushed a commit to jannisko/airflow that referenced this pull request Jun 15, 2024
…llows you to specify if you want the nested json as string to be also deserialized. The extra_dejson property uses this method with nested set to False. (apache#39811)

Co-authored-by: David Blain <[email protected]>
romsharon98 pushed a commit to romsharon98/airflow that referenced this pull request Jul 26, 2024
…llows you to specify if you want the nested json as string to be also deserialized. The extra_dejson property uses this method with nested set to False. (apache#39811)

Co-authored-by: David Blain <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:improvement Changelog: Improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants