rjgoyln commented on code in PR #72796:
URL: https://github.com/apache/airflow/pull/72796#discussion_r3970618070
##########
providers/fab/src/airflow/providers/fab/auth_manager/cli_commands/role_command.py:
##########
@@ -205,28 +205,17 @@ def roles_import(args):
existing_roles = [role.name for role in appbuilder.sm.get_all_roles()]
roles_to_import = [role_dict for role_dict in role_list if
role_dict["name"] not in existing_roles]
for role_dict in roles_to_import:
- if role_dict["name"] not in appbuilder.sm.get_all_roles():
- if role_dict["action"] == "" or role_dict["resource"] == "":
- appbuilder.sm.add_role(role_dict["name"])
- else:
- appbuilder.sm.add_role(role_dict["name"])
- role_args = Namespace(
- subcommand="add-perms",
- role=[role_dict["name"]],
- resource=[role_dict["resource"]],
- action=role_dict["action"].split(","),
- )
- __roles_add_or_remove_permissions(role_args)
-
- if role_dict["name"] in appbuilder.sm.get_all_roles():
- if role_dict["action"] == "" or role_dict["resource"] == "":
- pass
- else:
- role_args = Namespace(
- subcommand="add-perms",
- role=[role_dict["name"]],
- resource=[role_dict["resource"]],
- action=role_dict["action"].split(","),
- )
- __roles_add_or_remove_permissions(role_args)
+ # ``roles_export`` emits one entry per (role, resource) pair, so a
role name
+ # repeats across entries; ``add_role`` returns the existing role
after the first.
+ appbuilder.sm.add_role(role_dict["name"])
+ if not role_dict["action"] or not role_dict["resource"]:
Review Comment:
Small ordering change for hand-written files: the old code evaluated
`role_dict["action"]` before `add_role`, so an entry missing those keys raised
`KeyError` without touching the DB; now the role is committed first and the
import still dies half-finished — the same shape of partial import this PR
removes. `.get()` closes that and makes a bare `{"name": "X"}` behave like the
empty-string pair `roles_export` emits for a permissionless role.
```suggestion
if not role_dict.get("action") or not role_dict.get("resource"):
```
##########
providers/fab/tests/unit/fab/auth_manager/cli_commands/test_role_command.py:
##########
@@ -226,3 +226,24 @@ def test_cli_import_roles(self, tmp_path):
and permission.action.name == permissions.ACTION_CAN_ACCESS_MENU
for permission in fakeTeamA.permissions
)
+
+ def
test_cli_import_roles_when_a_role_without_permissions_comes_first(self,
tmp_path):
+ fn = tmp_path / "import_roles.json"
+ roles_list = [
+ {"name": "FakeTeamB", "resource": "", "action": ""},
+ {"name": "FakeTeamA", "resource": "Pools", "action":
"can_edit,can_read"},
+ ]
+ fn.write_text(json.dumps(roles_list))
+ role_command.roles_import(self.parser.parse_args(["roles", "import",
str(fn)]))
+
+ fakeTeamA: Role = self.appbuilder.sm.find_role("FakeTeamA")
+ fakeTeamB: Role = self.appbuilder.sm.find_role("FakeTeamB")
+
+ assert fakeTeamB is not None
Review Comment:
`fakeTeamA` is only dereferenced inside the set comprehension, so if the
import ever skips it the failure surfaces as `AttributeError: 'NoneType' object
has no attribute 'permissions'` instead of a clean assertion — worth mirroring
the `fakeTeamB` check.
```suggestion
assert fakeTeamA is not None
assert fakeTeamB is not None
```
--
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]
