Copilot commented on code in PR #62737:
URL: https://github.com/apache/airflow/pull/62737#discussion_r2874870274
##########
providers/fab/tests/unit/fab/auth_manager/security_manager/test_override.py:
##########
@@ -32,6 +34,45 @@ def __init__(self):
class TestFabAirflowSecurityManagerOverride:
+
@mock.patch("airflow.providers.fab.auth_manager.security_manager.override.log")
+ def
test_add_permission_to_role_ignores_duplicate_from_concurrent_worker(self,
mock_log):
+ sm = EmptySecurityManager()
+ role = Mock(id=1, name="test_admin", permissions=[])
+ permission = Mock(id=2)
+
+ mock_session = Mock()
+ mock_session.merge.side_effect = IntegrityError("stmt", {},
Exception("Duplicate entry"))
+
Review Comment:
These tests simulate the IntegrityError by raising from `session.merge`, but
in typical SQLAlchemy usage the IntegrityError for a duplicate association is
raised during flush/commit. Setting the side effect on `mock_session.commit`
would better match the real failure mode and make the test more representative.
##########
providers/fab/tests/unit/fab/auth_manager/security_manager/test_override.py:
##########
@@ -32,6 +34,45 @@ def __init__(self):
class TestFabAirflowSecurityManagerOverride:
+
@mock.patch("airflow.providers.fab.auth_manager.security_manager.override.log")
+ def
test_add_permission_to_role_ignores_duplicate_from_concurrent_worker(self,
mock_log):
+ sm = EmptySecurityManager()
+ role = Mock(id=1, name="test_admin", permissions=[])
+ permission = Mock(id=2)
+
+ mock_session = Mock()
+ mock_session.merge.side_effect = IntegrityError("stmt", {},
Exception("Duplicate entry"))
Review Comment:
The new tests use several unspecced `Mock()` instances (e.g., `role`,
`permission`, `mock_session`). Using `spec`/`autospec` (especially for the
session and role objects) would prevent the test from passing with invalid
attribute/method usage and aligns better with Airflow's testing expectations.
##########
providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py:
##########
@@ -1739,10 +1740,29 @@ def add_permission_to_role(self, role: Role,
permission: Permission | None) -> N
self.session.merge(role)
self.session.commit()
log.info(const.LOGMSG_INF_SEC_ADD_PERMROLE, permission,
role.name)
+ except IntegrityError:
+ self.session.rollback()
+ if self._is_permission_assigned_to_role(role_id=role.id,
permission_id=permission.id):
+ log.debug("Permission '%s' already assigned to role '%s'",
permission, role.name)
+ else:
+ log.error(const.LOGMSG_ERR_SEC_ADD_PERMROLE, "Failed to
assign permission after rollback")
Review Comment:
In the IntegrityError handler, the original exception details are discarded
(caught without `as e`), and the error log in the `else` branch doesn't include
the underlying IntegrityError. Capturing the exception and including it in the
error log (while still suppressing the duplicate-assignment case) would
preserve useful diagnostics for non-duplicate integrity failures.
```suggestion
except IntegrityError as e:
self.session.rollback()
if self._is_permission_assigned_to_role(role_id=role.id,
permission_id=permission.id):
log.debug("Permission '%s' already assigned to role
'%s'", permission, role.name)
else:
log.error(
const.LOGMSG_ERR_SEC_ADD_PERMROLE,
f"Failed to assign permission after rollback: {e}",
)
```
##########
providers/fab/src/airflow/providers/fab/auth_manager/security_manager/override.py:
##########
@@ -1739,10 +1740,29 @@ def add_permission_to_role(self, role: Role,
permission: Permission | None) -> N
self.session.merge(role)
self.session.commit()
log.info(const.LOGMSG_INF_SEC_ADD_PERMROLE, permission,
role.name)
+ except IntegrityError:
+ self.session.rollback()
+ if self._is_permission_assigned_to_role(role_id=role.id,
permission_id=permission.id):
+ log.debug("Permission '%s' already assigned to role '%s'",
permission, role.name)
+ else:
+ log.error(const.LOGMSG_ERR_SEC_ADD_PERMROLE, "Failed to
assign permission after rollback")
except Exception as e:
log.error(const.LOGMSG_ERR_SEC_ADD_PERMROLE, e)
self.session.rollback()
+ def _is_permission_assigned_to_role(self, role_id: int | None,
permission_id: int | None) -> bool:
+ """Check if the permission is already assigned to the role."""
+ if role_id is None or permission_id is None:
+ return False
+ return bool(
+ self.session.scalar(
+ select(assoc_permission_role.c.id).where(
+ assoc_permission_role.c.role_id == role_id,
+ assoc_permission_role.c.permission_view_id ==
permission_id,
+ )
Review Comment:
`_is_permission_assigned_to_role` takes a `permission_id` argument but
compares it to `assoc_permission_role.c.permission_view_id`. Since the value
passed is `Permission.id` (permission view id), consider renaming the parameter
to `permission_view_id` (and updating call sites) to avoid confusion with
`Action.id`/`Permission.action_id` elsewhere in the FAB models.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
