airflow-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From GitBox <...@apache.org>
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:02:01 GMT
dstandish commented on a change in pull request #6426: [AIRFLOW-5751] add get_uri method to
Connection
URL: https://github.com/apache/airflow/pull/6426#discussion_r338951342
 
 

 ##########
 File path: airflow/models/connection.py
 ##########
 @@ -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
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?

----------------------------------------------------------------
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:
users@infra.apache.org


With regards,
Apache Git Services

Mime
View raw message