betodealmeida commented on code in PR #31332:
URL: https://github.com/apache/superset/pull/31332#discussion_r1876190331
##########
superset/migrations/shared/catalogs.py:
##########
@@ -683,7 +691,15 @@ def downgrade_schema_perms(
None,
schema,
)
- pvms_to_rename.append((pvm, new_name))
+ # check to see if the new name already exists
+ if session.query(ViewMenu).filter_by(name=new_name).one_or_none():
+ logger.warning(
+ "New permission %s already exists. Deleting old one",
+ new_name,
+ )
+ pvms_to_delete.append(pvm)
Review Comment:
Same here, I would just leave the existing one, something like:
```python
# check to see if the new name already exists
if not
session.query(ViewMenu).filter_by(name=new_name).one_or_none():
# new perm doesn't exist, rename existing one to the new name
pvms_to_rename.append((pvm, new_name))
##########
superset/migrations/shared/catalogs.py:
##########
@@ -505,7 +505,15 @@ def upgrade_schema_perms(
.filter_by(name=current_perm)
.one_or_none()
):
- existing_pvm.name = new_perm
+ # check that new_perm does not exist
+ if session.query(ViewMenu).filter_by(name=new_perm).one_or_none():
+ logger.warning(
+ "New permission %s already exists. Removing old one",
+ new_perm,
+ )
+ session.delete(existing_pvm)
Review Comment:
@mistercrunch the current code does that, we get the current perm (where the
catalog name is `null`, see how we compute `current_perm` in line 492) and
change it to point to the new perm name `new_perm`, which includes the default
catalog name (line 497). Then we simply rename the existing PVM to have the new
name, so that existing associations like roles are preserved (line 508 on the
left).
In regards to this PR, I don't think we should delete existing PVMs. First,
because they should not exist... the only reason I can imagine why it would
exist is bad upgrade-downgrade that didn't rename the PVMs correctly. If that
happens, I think it's better to just keep the existing PVM, since it already
has the correct name, and might be associated with roles.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
