[AIRFLOW-5751] add get_uri method to Connection

[dstandish]

• Add a convenience method get_uri on Connection object to generate the URI for a connection.

Make sure you have checked all steps below.

Jira

• My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-5751] My Airflow PR"
  • https://issues.apache.org/jira/browse/AIRFLOW-5751

Description

• Here are some details about my PR, including screenshots of any UI changes:

Building URI for use in connection env var can be a nuisance. This is a convenience method that will do it for you, given a connection object.

So if you have created one in UI, you could do BaseHook.get_connection(conn_id).get_uri().

Or build using init params on Connection object and call get_uri().

I think it could also be nice if each hook had a get_uri method that would take all relevant params and produce a correctly encoded URI. If that were implemented, it could use this function for that purpose.

I added tests to verify that generated URIs, when parsed again, produce the same connection object. For this I used the same URIs we were already testing. And as part of this there was an efficiency in refactoring the existing from_uri test.

Tests

• My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

• My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
  1. Subject is separated from body by a blank line
  2. Subject is limited to 50 characters (not including Jira issue reference)
  3. Subject does not end with a period
  4. Subject uses the imperative mood ("add", not "adding")
  5. Body wraps at 72 characters
  6. Body explains "what" and "why", not "how"

Documentation

• In case of new functionality, my PR adds documentation that describes how to use it.
  • All the public functions and the classes in the PR contain docstrings that explain what it does
  • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

[codecov-io]

Codecov Report

❗ No coverage uploaded for pull request base (master@0f21e9b). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #6426   +/-   ##
=========================================
  Coverage          ?   84.09%           
=========================================
  Files             ?      672           
  Lines             ?    38176           
  Branches          ?        0           
=========================================
  Hits              ?    32105           
  Misses            ?     6071           
  Partials          ?        0

Impacted Files	Coverage Δ
airflow/models/connection.py	68.96% <100%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f21e9b...6d458a0. Read the comment docs.

[mik-laj]

I personally prefer not to build URLs myself. What do you think about using the https://docs.python.org/2/library/urlparse.html#urlparse.urlunparse function?

[dstandish]

Ok so one reason is that urlunparse did not seem that helpful to me.

I noticed we use urlunparse in CLI connection add (to build a STDOUT message).

But even there, we construct user / password / host / port with format string (because it does not handle that for you). And it does not handle quoting for you either.

It seems more useful if you are starting with something that has been parsed with urlparse.

In any case we have to be very careful to align our quoting with the handling in parse_from_uri.

It does not handle urlencoding of the extra field either. So it did not seem of much use.

One thing I was on the fence about was whether to bother with minifying the formatting. By that i mean if user / password is omitted, should we omit the :@.

For example we could do this:

    def get_uri(self) -> str:
        uri = '{conn_type}://{login}:{password}@{host}:{port}/{schema}?{query}'.format(
            conn_type=self.conn_type.replace('_', '-'),
            login=quote(self.login or '', safe=''),
            password=quote(self.password or ''),
            host=quote(self.host or '', safe=''),
            port=self.port or '',
            schema=quote(self.schema, safe=''),
            query=urlencode(self.extra_dejson),
        )
        return uri

Much more elegant this way. However, if we do this, login will be ''. When login is omitted in URI, the login attribute it is parsed as None. As a result, 3 of the tests fail (the last 3). So, the more verbose approach seems to be a more faithful inverse of the the parse_from_uri function.

WDYT?

[ashb]

I think schema doesn't depend on host.

For instance AIRFLOW__CORE__SQLALCHEMY_CONN=postgresql:///airflow

Should the schema includce the leading / already? i.e. do we end up with a // here?

[dstandish]

ok @ashb I have updated to support (with tests) for schema only, login only, password only, and port only

[dstandish]

ok lastly (one can hope) i have added another set of tests verifying that when we create connection from init attributes (rather than uri) that everything still works consistently -- URIs generated still parse to an equivalent connection object.

[mik-laj]

I would like to point out that some hooks have methods for creating URI.
For example:
https://github.com/apache/airflow/blob/master/airflow/hooks/dbapi_hook.py#L69-L78

[dstandish]

Interesting. Yeah I think it's good for hooks to have this. I'd like it if every hook had a static method that took all valid params and produce correct URI. With a conn.get_uri() method, this would be more convenient -- hook get_uri methods could just create the conn object and call get_uri.

E.g. for GCP, such a method could look like this:

    @staticmethod
    def get_uri(
        project=None,
        key_path=None,
        keyfile_dict=None,
        scope=None,
        num_retries=None,
    ):
        extra_dict = dict(
            project=project,
            key_path=key_path,
            keyfile_dict=keyfile_dict,
            scope=scope,
            num_retries=num_retries,
        )
        extra = json.dumps(
            {"extra__google_cloud_platform__" + k: v for k, v in extra_dict.items() if v is not None}
        )
        conn = Connection(conn_type='google-cloud-platform', extra=extra)
        return conn.get_uri()

Stepping back, do you think this PR is a worthwhile addition (i.e. adding get_uri to Connection)?

It's a small thing, but I think it's good to have a standardized method of generating a uri that is guaranteed to parse correctly.

So a user could build their connection object with init params, and then generate a URI that will work. No fussing with urllib. No digging through to see how things are parsed.

[dstandish]

@mik-laj @ashb any other concerns with this one?

[dstandish]

@ashb @mik-laj friendly nag

this can help people produce URI reliably

[ashb]

@dstandish Could you rebase this on to latest master please? Sorry for leaving this.

If it still passes ther under pytest (it almost certainly should) then we can merge

[dstandish]

rebased; tests pass; thanks :)