airflow-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <>
Subject [GitHub] [airflow] dstandish commented on a change in pull request #6426: [AIRFLOW-5751] add get_uri method to Connection
Date Fri, 25 Oct 2019 09:01:16 GMT
dstandish commented on a change in pull request #6426: [AIRFLOW-5751] add get_uri method to

 File path: airflow/models/
 @@ -145,6 +145,38 @@ def parse_from_uri(self, uri):
         if uri_parts.query:
             self.extra = json.dumps(dict(parse_qsl(uri_parts.query, keep_blank_values=True)))
+    def get_uri(self) -> str:
+        uri = '{}://'.format(self.conn_type.replace('_', '-'))
+        user_block = ''
+        if self.login is not None:
+            user_block += quote(self.login, safe='')
 Review comment:
   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
   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( or '', safe=''),
               port=self.port or '',
               schema=quote(self.schema, safe=''),
           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`.
   So 3 of the tests fail (the last 3).  The more verbose approach seems to be a more faithful
inverse of the the `parse_from_uri` function.

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.
For queries about this service, please contact Infrastructure at:

With regards,
Apache Git Services

View raw message