Re: [PR] Remove @provide_session from Variable.set in models to support with AF3 [airflow]

["ashb (via GitHub)" <gi...@apache.org>]

ashb commented on code in PR #49107:
URL: https://github.com/apache/airflow/pull/49107#discussion_r2039062998


##########
airflow-core/src/airflow/models/variable.py:
##########
@@ -180,13 +180,11 @@ def get(
                 return var_val
 
     @staticmethod
-    @provide_session
     def set(
         key: str,
         value: Any,
         description: str | None = None,
         serialize_json: bool = False,
-        session: Session = None,

Review Comment:
   Given `Variable.update` has a session, if we are doing to remove `@provide_session` I think this should be
   
   `session: Session | None = None` -- that way we still use the provided session if there is one.



##########
airflow-core/src/airflow/models/variable.py:
##########
@@ -225,18 +219,20 @@ def set(
             )
             return
 
+        # check if the secret exists in the custom secrets' backend.
+        Variable.check_for_write_conflict(key=key)
         if serialize_json:
             stored_value = json.dumps(value, indent=2)
         else:
             stored_value = str(value)
-
-        Variable.delete(key, session=session)
-        session.add(Variable(key=key, val=stored_value, description=description))
-        session.flush()
-        # invalidate key in cache for faster propagation
-        # we cannot save the value set because it's possible that it's shadowed by a custom backend
-        # (see call to check_for_write_conflict above)
-        SecretCache.invalidate_variable(key)
+        with create_session() as session:
+            Variable.delete(key, session=session)
+            session.add(Variable(key=key, val=stored_value, description=description))
+            session.flush()
+            # invalidate key in cache for faster propagation
+            # we cannot save the value set because it's possible that it's shadowed by a custom backend
+            # (see call to check_for_write_conflict above)
+            SecretCache.invalidate_variable(key)
 
     @staticmethod
     @provide_session

Review Comment:
   Want to do this here too I think?



##########
airflow-core/src/airflow/models/variable.py:
##########
@@ -225,18 +219,20 @@ def set(
             )
             return
 
+        # check if the secret exists in the custom secrets' backend.
+        Variable.check_for_write_conflict(key=key)
         if serialize_json:
             stored_value = json.dumps(value, indent=2)
         else:
             stored_value = str(value)
-
-        Variable.delete(key, session=session)
-        session.add(Variable(key=key, val=stored_value, description=description))
-        session.flush()
-        # invalidate key in cache for faster propagation
-        # we cannot save the value set because it's possible that it's shadowed by a custom backend
-        # (see call to check_for_write_conflict above)
-        SecretCache.invalidate_variable(key)
+        with create_session() as session:

Review Comment:
   ```suggestion
           if session is not None:
               ctx = contextlib.nullcontext(session)
           else:
               ctx = create_session()
           with ctx as session:
   ```
   
   That way we can still use the passed in session if there is one?



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