[GitHub] [airflow] dstandish commented on a change in pull request #19857: Enable json serialization for secrets backend

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

dstandish commented on a change in pull request #19857:
URL: https://github.com/apache/airflow/pull/19857#discussion_r810225522



##########
File path: airflow/cli/commands/connection_command.py
##########
@@ -82,25 +83,35 @@ def connections_list(args):
         )
 
 
+def _connection_to_dict(conn: Connection) -> dict:
+    return dict(
+        conn_type=conn.conn_type,
+        description=conn.description,
+        login=conn.login,
+        password=conn.password,
+        host=conn.host,
+        port=conn.port,
+        schema=conn.schema,
+        extra=conn.extra,

Review comment:
       it doesn't matter what is stored in `extra` because it is a string and when we serialize it we don't make any assumptions about what it contains.
   
   when we deserialize it, it is again a string and we pass it to Connection as a kwarg.
   
   So yes suppose we have this connection
   
   ```
   Connection(extra=json.dumps({'hi': 'bye'}))
   ```
   So the CLI exporter will export this like so
   ```
   '{"extra": "{\\"hi\\": \\"bye\\"}"}'
   ```
   So when we try to parse this again we do json.loads, and now we get a dict where `extra` is a plain json string, as desired:
   ```
   {'extra': '{"hi": "bye"}'}
   ```
   So, everything is cool right?
   There's one more detail here though.
   What we _do also_ support here, is serializing airflow conn extra fully as object.
   By that I mean, storing _this_ uri _is permissible_:
   ```
   '{"extra": {"hi": "bye"}}'
   ```
   And the reason we can support this is, if it's stored like this, `extra` will be `dict` when the value is loaded.
   We just can't _serialize_ in this way by default because there is no guarantee that `extra` will be json.
   
   So in sum, we support `[obj, str] > Connection.extra`, but when _generating_ the value in CLI export, we have to only support `Connection.extra -> str`.
   
   I _wish_ this were not the case, but because we don't require extra to be json (maybe until 3.0?) i think that's how it must be.
   
   




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

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org