Skip to content

Allow Connection Edit View to handle entries with NULL 'extra'#12149

Merged
XD-DENG merged 2 commits intoapache:masterfrom
XD-DENG:issue-12148
Nov 7, 2020
Merged

Allow Connection Edit View to handle entries with NULL 'extra'#12149
XD-DENG merged 2 commits intoapache:masterfrom
XD-DENG:issue-12148

Conversation

@XD-DENG
Copy link
Member

@XD-DENG XD-DENG commented Nov 7, 2020

closes: #12148 , a bug I observe in 2.0.0a2.

The root-cause is simple:
if the connection data is {..., "extra": None} (which is common), then form.data.get('extra', '{}') would return None rather than '{}', which causes error given json.loads() does not accept None.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.

@XD-DENG XD-DENG requested review from ashb and kaxil November 7, 2020 08:54
@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Nov 7, 2020
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's cool. If possible to add a unit test, that would be great but it's ok like it is anyway

@potiuk potiuk added this to the Airflow 2.0.0-beta1 milestone Nov 7, 2020
@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Nov 7, 2020
@github-actions
Copy link

github-actions bot commented Nov 7, 2020

The PR should be OK to be merged with just subset of tests as it does not modify Core of Airflow. The committers might merge it or can add a label 'full tests needed' and re-run it to run all tests if they see it is needed!

Copy link
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add tests please?

@XD-DENG
Copy link
Member Author

XD-DENG commented Nov 7, 2020

Thanks @potiuk and @ashb .

Have some arrangement later so will only be able to add the test either tonight or tomorrow. Will ping you then.

@github-actions
Copy link

github-actions bot commented Nov 7, 2020

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@potiuk potiuk added the priority:critical Showstopper bug that should be patched immediately label Nov 7, 2020
@XD-DENG XD-DENG requested review from ashb, kaxil and potiuk November 7, 2020 18:35
@XD-DENG
Copy link
Member Author

XD-DENG commented Nov 7, 2020

hi @potiuk @ashb @kaxil , simple test is added to ensure case where connection's extra is None can be handled.

Earlier I had another test case where connection has no extra field. But eventually I decided to remove it given this case may never happen if I'm not wrong

@XD-DENG
Copy link
Member Author

XD-DENG commented Nov 7, 2020

It's the quarantined tests suite failing, and the main test cases are fine. So merging this now.

@XD-DENG XD-DENG merged commit bedaf53 into apache:master Nov 7, 2020
@XD-DENG XD-DENG deleted the issue-12148 branch November 7, 2020 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:webserver Webserver related Issues okay to merge It's ok to merge this PR as it does not require more tests priority:critical Showstopper bug that should be patched immediately

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2.0.0a2 webserver fails when connection "extra" field is None (NULL in DB)

4 participants