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

Fix CLI connections import and migrate logic from secrets to Connection model #15425

Merged
merged 4 commits into from Jun 11, 2021

Conversation

@natanweinberger
Copy link
Contributor

@natanweinberger natanweinberger commented Apr 18, 2021

This is a bug fix for the CLI command connections_import. I originally added this CLI functionality in #15177. The bug was reported by @mudravrik here.

Bug summary

Running airflow connections import <filepath> results in an AttributeError.

Traceback (most recent call last):
...
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/cli/commands/connection_command.py", line 244, in connections_import
    _import_helper(args.file)
  File "/home/airflow/.local/lib/python3.8/site-packages/airflow/cli/commands/connection_command.py", line 272, in _import_helper
    key: value for key, value in conn_values.items() if key in allowed_fields
AttributeError: 'Connection' object has no attribute 'items'

Root cause

I called the existing function load_connections_dict to get the contents of the target file, thinking it returned a list of dictionaries. When I later looped through the list, I called the dictionary method .items() on each connection. However, each connection was actually stored into a Connection model instance rather than a dictionary, so the call I made to .items() resulted in an AttributeError.

Solution

In taking a closer look at this issue, I realized the code block that contained the bug can be removed altogether. Having a list of Connection models to begin with (rather than dictionaries) eliminated the need to even examine the keys and values of the dictionaries. I had only done that to safely load the data into Connection models in the first place.

Preventing future issues

To avoid future issues, I also changed how the tests work for providing sample data. Before, a dictionary of sample data was set as the return value of load_connections_dict, which actually returns an object of type List[Connection]. Now, that dictionary of sample data is set to be the return value of a lower level function, _parse_secrets_file, which does no heavy lifting outside of parsing a JSON, YAML or env file and returns a dictionary.

Bug originally introduced in #15177, which closed #9855.

@natanweinberger natanweinberger force-pushed the master branch 3 times, most recently from 420d632 to c29a372 Apr 19, 2021
@natanweinberger
Copy link
Contributor Author

@natanweinberger natanweinberger commented Apr 19, 2021

I've refactored the functionality that reads data from a file and creates connections from it.

  • the helper functions to parse the contents of JSON, YAML and env files are moved from airflow/secrets/local_filesystem.py to airflow/utils/parse.py, as suggested by @mik-laj above
  • there is a new method Connection.from_dict
  • the connections import CLI function uses the parse utils and Connection.from_dict to import connections from a file, which eliminates dependency on secrets

I was as conservative as possible in refactoring the secrets backend in this PR, but it seems like there's not much that uses it. I'd be open to doing some broader clean up there as a follow-up PR.

@natanweinberger natanweinberger force-pushed the master branch 7 times, most recently from 52afacc to 5bce89d Apr 20, 2021
@natanweinberger natanweinberger requested a review from mik-laj Apr 20, 2021
@natanweinberger natanweinberger force-pushed the master branch 3 times, most recently from 5c675ba to f8c5cdf Apr 26, 2021
@natanweinberger
Copy link
Contributor Author

@natanweinberger natanweinberger commented Apr 26, 2021

@mik-laj I've incorporated your feedback, can you please re-review when you have a chance? 🙏

@natanweinberger natanweinberger force-pushed the master branch 4 times, most recently from 3bfa59b to a64c8f6 May 4, 2021
airflow/models/connection.py Outdated Show resolved Hide resolved
@natanweinberger natanweinberger changed the title Fix CLI connections import Fix CLI connections import and migrate logic from secrets to Connection model May 4, 2021
@natanweinberger natanweinberger force-pushed the master branch 20 times, most recently from 51fd8a2 to 2fb1bd0 Jun 10, 2021
natanweinberger and others added 4 commits Jun 10, 2021
In connections_import, each connection was deserialized and stored into a
Connection model instance rather than a dictionary, so an erroneous call to the
dictionary methods .items() resulted in an AttributeError. With this fix,
connection information is loaded from dictionaries directly into the
Connection constructor and committed to the DB.
@ashb ashb merged commit 002075a into apache:main Jun 11, 2021
46 of 50 checks passed
jhtimmins added a commit to astronomer/airflow that referenced this issue Jun 22, 2021
…on model (apache#15425)

* Add field 'extra' to Connection init

* Fix connections import CLI

In connections_import, each connection was deserialized and stored into a
Connection model instance rather than a dictionary, so an erroneous call to the
dictionary methods .items() resulted in an AttributeError. With this fix,
connection information is loaded from dictionaries directly into the
Connection constructor and committed to the DB.

* Apply suggestions from code review

* Use load_connections_dict in connections import

Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
(cherry picked from commit 002075a)
ashb added a commit that referenced this issue Jun 22, 2021
…on model (#15425)

* Add field 'extra' to Connection init

* Fix connections import CLI

In connections_import, each connection was deserialized and stored into a
Connection model instance rather than a dictionary, so an erroneous call to the
dictionary methods .items() resulted in an AttributeError. With this fix,
connection information is loaded from dictionaries directly into the
Connection constructor and committed to the DB.

* Apply suggestions from code review

* Use load_connections_dict in connections import

Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
(cherry picked from commit 002075a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants