diegoscarabelli commented on issue #36549:
URL: https://github.com/apache/superset/issues/36549#issuecomment-3647462086
Apologies for the false alarm. I tried again to reproduce the bugs and
realized that they were in both cases artifacts. However, it does point out to
the fact that Superset currently has a potential incompatibility problem if the
full get_bind() function signature is utilized, and if the custom
`sync_role_definitions()` is removed.
Here a detailed debug of what happened.
# The FAB 5.0 Bugs Were Not Reproduced
**TL;DR**: The documented FAB 5.0 compatibility bugs do not exist. Superset
6.0.0rc4 work correctly with Flask-SQLAlchemy 2.5.1 + Flask-AppBuilder 5.0.0.
The bugs I reported were in reality theoretical incompatibilities
misinterpreted as bugs.
## `get_bind()` Error - Why It Doesn't Occur
### The Documented Bug
```
SignallingSession.get_bind() got an unexpected keyword argument 'bind'
```
### Why It Was Expected
- Flask-SQLAlchemy 2.5.1: `get_bind(mapper, clause)`
- SQLAlchemy 1.4: `get_bind(mapper, clause, bind=None)`
- Signature mismatch exists on paper
### Why It Doesn't Happen
SQLAlchemy 1.4 doesn't pass the `bind` parameter in Superset's code paths.
```python
# Flask-SQLAlchemy 2.5.1 implementation
def get_bind(self, mapper=None, clause=None):
# ... handles binding logic
return SessionBase.get_bind(self, mapper, clause) # Calls parent
without bind
```
When SQLAlchemy 1.4 calls `get_bind()` in the operations Superset uses:
- It calls with `(mapper, clause)`
- It does NOT pass the optional `bind` keyword argument
- Flask-SQLAlchemy 2.5.1 handles it correctly
### Why Reported Bug Previously
While building on top of 6.0.0rc4, the call of the Superset use of
get_bind() was modified to include the bind parameter, due to the function
signature in SQLAlchemy 1.4. This caused the error to appear in that specific
context. However, in vanilla 6.0.0rc4, Superset's code paths do not trigger
this issue.
### Conclusions
The theoretical incompatibility doesn't manifest in practice. However, this
is something to keep in mind for future versions.
---
## Missing `sync_role_definitions()` - Why It Doesn't Occur
### The Documented Bug
```python
AttributeError: 'SupersetSecurityManager' object has no attribute
'sync_role_definitions'
```
### Why It Was Expected
- Flask-AppBuilder 5.0.0 removed `SecurityManager.sync_role_definitions()`
- Superset calls `security_manager.sync_role_definitions()` in
`cli/main.py:85`
### Why It Doesn't Happen
**Superset has its own implementation** - not inherited from FAB.
```python
# superset/security/manager.py:1175
class SupersetSecurityManager(SecurityManager):
def sync_role_definitions(self) -> None:
"""Initialize the Superset application with security roles and
such."""
logger.info("Syncing role definition")
self.create_custom_permissions()
pvms = self._get_all_pvms()
self.set_role("Admin", self._is_admin_pvm, pvms)
self.set_role("Alpha", self._is_alpha_pvm, pvms)
self.set_role("Gamma", self._is_gamma_pvm, pvms)
self.set_role("sql_lab", self._is_sql_lab_pvm, pvms)
# ... continues
```
This method:
- Exists in Superset's own `SupersetSecurityManager` class
- Is NOT inherited from FAB's base `SecurityManager`
- Works with both FAB 4.x and FAB 5.x
FAB 5.0's removal of this method doesn't affect Superset.
### Why Reported Bug Previously
In my modification to 6.0.0rc4, Claude found the repetition and removed the
custom function implementation in Superset, leading to the error. However, in
vanilla 6.0.0rc4, the method is present and works correctly.
### Conclusions
The removal of `sync_role_definitions()` from FAB 5.0 does not impact
Superset, as it implements its own version of the method.
---
## Flask-AppBuilder 5.0 Dependency Facts
### What FAB 5.0 Actually Requires
```python
# From Flask-AppBuilder 5.0.0 package metadata
"Flask-SQLAlchemy>=2.4.0,<4"
```
This means FAB 5.0 accepts BOTH:
- Flask-SQLAlchemy 2.x (2.4.0 - 2.5.1)
- Flask-SQLAlchemy 3.x (3.0.0 - 3.1.x)
--
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]
