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`.
   
   So 3 of the tests fail (the last 3).  This 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:
[email protected]


With regards,
Apache Git Services

Reply via email to