[GitHub] [airflow] dstandish commented on a change in pull request #6426: [AIRFLOW-5751] add get_uri method to Connection

[GitBox <gi...@apache.org>]

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 urlparse 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`.
   
   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