codeant-ai-for-open-source[bot] commented on code in PR #34875:
URL: https://github.com/apache/superset/pull/34875#discussion_r4161873074
##########
superset/migrations/shared/catalogs.py:
##########
@@ -24,23 +24,92 @@
import sqlalchemy as sa
from alembic import op
from flask import current_app
-from sqlalchemy.orm import declarative_base, lazyload, Session
+from sqlalchemy.orm import declarative_base, Session
-from superset import db, security_manager
+# Note: Import Database functionality without importing the actual model
+from superset import db, db_engine_specs, security_manager
+from superset.databases.utils import make_url_safe
from superset.db_engine_specs.base import GenericDBException
from superset.migrations.shared.security_converge import (
add_pvms,
Permission,
PermissionView,
ViewMenu,
)
-from superset.models.core import Database
logger = logging.getLogger("alembic.env")
Base: Type[Any] = declarative_base()
+class Database(Base):
+ """Local Database model for migration"""
+
+ __tablename__ = "dbs"
+
+ id = sa.Column(sa.Integer, primary_key=True)
+ sqlalchemy_uri = sa.Column(sa.String(1024))
+ encrypted_extra = sa.Column(sa.Text)
+ database_name = sa.Column(sa.String(250))
+
+ @property
+ def db_engine_spec(self) -> Type[Any]:
+ url = make_url_safe(self.sqlalchemy_uri)
+ backend = url.get_backend_name()
+ try:
+ driver = url.get_driver_name()
+ except Exception:
+ driver = None
+ return db_engine_specs.get_engine_spec(backend, driver)
+
+ def get_default_catalog(self) -> str | None:
+ """Get default catalog using the engine spec."""
+ return self.db_engine_spec.get_default_catalog(self)
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and
marked this thread resolved** as of `5765f0b`.
The catalog permission upgrade is now an intentional no-op, so
`get_default_catalog` is no longer called with the local model.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it
open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/migrations/shared/catalogs.py:
##########
@@ -24,23 +24,92 @@
import sqlalchemy as sa
from alembic import op
from flask import current_app
-from sqlalchemy.orm import declarative_base, lazyload, Session
+from sqlalchemy.orm import declarative_base, Session
-from superset import db, security_manager
+# Note: Import Database functionality without importing the actual model
+from superset import db, db_engine_specs, security_manager
+from superset.databases.utils import make_url_safe
from superset.db_engine_specs.base import GenericDBException
from superset.migrations.shared.security_converge import (
add_pvms,
Permission,
PermissionView,
ViewMenu,
)
-from superset.models.core import Database
logger = logging.getLogger("alembic.env")
Base: Type[Any] = declarative_base()
+class Database(Base):
+ """Local Database model for migration"""
+
+ __tablename__ = "dbs"
+
+ id = sa.Column(sa.Integer, primary_key=True)
+ sqlalchemy_uri = sa.Column(sa.String(1024))
+ encrypted_extra = sa.Column(sa.Text)
+ database_name = sa.Column(sa.String(250))
+
+ @property
+ def db_engine_spec(self) -> Type[Any]:
+ url = make_url_safe(self.sqlalchemy_uri)
+ backend = url.get_backend_name()
+ try:
+ driver = url.get_driver_name()
+ except Exception:
+ driver = None
+ return db_engine_specs.get_engine_spec(backend, driver)
+
+ def get_default_catalog(self) -> str | None:
+ """Get default catalog using the engine spec."""
+ return self.db_engine_spec.get_default_catalog(self)
+
+ def is_oauth2_enabled(self) -> bool:
+ """Check if OAuth2 is enabled for this database."""
+ from superset.utils import json
+
+ encrypted_extra = json.loads(self.encrypted_extra or "{}")
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and
marked this thread resolved** as of `5765f0b`.
The migration no longer reads or JSON-decodes `encrypted_extra`; catalog
permission backfill is skipped.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it
open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/migrations/shared/catalogs.py:
##########
@@ -24,23 +24,92 @@
import sqlalchemy as sa
from alembic import op
from flask import current_app
-from sqlalchemy.orm import declarative_base, lazyload, Session
+from sqlalchemy.orm import declarative_base, Session
-from superset import db, security_manager
+# Note: Import Database functionality without importing the actual model
+from superset import db, db_engine_specs, security_manager
+from superset.databases.utils import make_url_safe
from superset.db_engine_specs.base import GenericDBException
from superset.migrations.shared.security_converge import (
add_pvms,
Permission,
PermissionView,
ViewMenu,
)
-from superset.models.core import Database
logger = logging.getLogger("alembic.env")
Base: Type[Any] = declarative_base()
+class Database(Base):
+ """Local Database model for migration"""
+
+ __tablename__ = "dbs"
+
+ id = sa.Column(sa.Integer, primary_key=True)
+ sqlalchemy_uri = sa.Column(sa.String(1024))
+ encrypted_extra = sa.Column(sa.Text)
+ database_name = sa.Column(sa.String(250))
+
+ @property
+ def db_engine_spec(self) -> Type[Any]:
+ url = make_url_safe(self.sqlalchemy_uri)
+ backend = url.get_backend_name()
+ try:
+ driver = url.get_driver_name()
+ except Exception:
+ driver = None
+ return db_engine_specs.get_engine_spec(backend, driver)
+
+ def get_default_catalog(self) -> str | None:
+ """Get default catalog using the engine spec."""
+ return self.db_engine_spec.get_default_catalog(self)
+
+ def is_oauth2_enabled(self) -> bool:
+ """Check if OAuth2 is enabled for this database."""
+ from superset.utils import json
+
+ encrypted_extra = json.loads(self.encrypted_extra or "{}")
+ return bool(encrypted_extra.get("oauth2_client_info"))
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and
marked this thread resolved** as of `5765f0b`.
The upgrade no longer performs OAuth checks or opens catalog connections
because catalog permission backfill is disabled.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it
open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/migrations/shared/catalogs.py:
##########
@@ -24,23 +24,92 @@
import sqlalchemy as sa
from alembic import op
from flask import current_app
-from sqlalchemy.orm import declarative_base, lazyload, Session
+from sqlalchemy.orm import declarative_base, Session
-from superset import db, security_manager
+# Note: Import Database functionality without importing the actual model
+from superset import db, db_engine_specs, security_manager
+from superset.databases.utils import make_url_safe
from superset.db_engine_specs.base import GenericDBException
from superset.migrations.shared.security_converge import (
add_pvms,
Permission,
PermissionView,
ViewMenu,
)
-from superset.models.core import Database
logger = logging.getLogger("alembic.env")
Base: Type[Any] = declarative_base()
+class Database(Base):
+ """Local Database model for migration"""
+
+ __tablename__ = "dbs"
+
+ id = sa.Column(sa.Integer, primary_key=True)
+ sqlalchemy_uri = sa.Column(sa.String(1024))
+ encrypted_extra = sa.Column(sa.Text)
+ database_name = sa.Column(sa.String(250))
+
+ @property
+ def db_engine_spec(self) -> Type[Any]:
+ url = make_url_safe(self.sqlalchemy_uri)
+ backend = url.get_backend_name()
+ try:
+ driver = url.get_driver_name()
+ except Exception:
+ driver = None
+ return db_engine_specs.get_engine_spec(backend, driver)
+
+ def get_default_catalog(self) -> str | None:
+ """Get default catalog using the engine spec."""
+ return self.db_engine_spec.get_default_catalog(self)
+
+ def is_oauth2_enabled(self) -> bool:
+ """Check if OAuth2 is enabled for this database."""
+ from superset.utils import json
+
+ encrypted_extra = json.loads(self.encrypted_extra or "{}")
+ return bool(encrypted_extra.get("oauth2_client_info"))
+
+ def get_inspector(self, catalog: str | None = None) -> Any:
+ """Get a database inspector for introspection."""
+ from sqlalchemy import create_engine, inspect
+
+ # Create an engine from the URI
+ engine = create_engine(self.sqlalchemy_uri)
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and
marked this thread resolved** as of `5765f0b`.
The local model inspection and related engine setup have been removed; the
upgrade now only logs that catalog permission backfill is skipped.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it
open.</sub>
<!-- codeant-auto-resolve-reply -->
##########
superset/migrations/shared/catalogs.py:
##########
@@ -24,23 +24,92 @@
import sqlalchemy as sa
from alembic import op
from flask import current_app
-from sqlalchemy.orm import declarative_base, lazyload, Session
+from sqlalchemy.orm import declarative_base, Session
-from superset import db, security_manager
+# Note: Import Database functionality without importing the actual model
+from superset import db, db_engine_specs, security_manager
+from superset.databases.utils import make_url_safe
from superset.db_engine_specs.base import GenericDBException
from superset.migrations.shared.security_converge import (
add_pvms,
Permission,
PermissionView,
ViewMenu,
)
-from superset.models.core import Database
logger = logging.getLogger("alembic.env")
Base: Type[Any] = declarative_base()
+class Database(Base):
+ """Local Database model for migration"""
+
+ __tablename__ = "dbs"
+
+ id = sa.Column(sa.Integer, primary_key=True)
+ sqlalchemy_uri = sa.Column(sa.String(1024))
+ encrypted_extra = sa.Column(sa.Text)
+ database_name = sa.Column(sa.String(250))
+
+ @property
+ def db_engine_spec(self) -> Type[Any]:
+ url = make_url_safe(self.sqlalchemy_uri)
+ backend = url.get_backend_name()
+ try:
+ driver = url.get_driver_name()
+ except Exception:
+ driver = None
+ return db_engine_specs.get_engine_spec(backend, driver)
+
+ def get_default_catalog(self) -> str | None:
+ """Get default catalog using the engine spec."""
+ return self.db_engine_spec.get_default_catalog(self)
+
+ def is_oauth2_enabled(self) -> bool:
+ """Check if OAuth2 is enabled for this database."""
+ from superset.utils import json
+
+ encrypted_extra = json.loads(self.encrypted_extra or "{}")
+ return bool(encrypted_extra.get("oauth2_client_info"))
+
+ def get_inspector(self, catalog: str | None = None) -> Any:
+ """Get a database inspector for introspection."""
+ from sqlalchemy import create_engine, inspect
+
+ # Create an engine from the URI
+ engine = create_engine(self.sqlalchemy_uri)
+ if catalog and hasattr(engine, "execution_options"):
+ engine = engine.execution_options(catalog=catalog)
+ return inspect(engine)
+
+ def get_all_schema_names(self, catalog: str | None = None) -> list[str]:
+ """
+ Get all schema names for this database.
+
+ Uses SQLAlchemy inspector to get schema names directly.
+ """
+ try:
+ with self.get_inspector(catalog=catalog) as inspector:
+ return self.db_engine_spec.get_schema_names(inspector)
Review Comment:
✅ **CodeAnt verified this suggestion was addressed in subsequent commits and
marked this thread resolved** as of `5765f0b`.
The schema and catalog discovery logic, including the `get_inspector`
context-manager calls, has been removed.
<sub>If that's not right, unresolve this thread and CodeAnt will leave it
open.</sub>
<!-- codeant-auto-resolve-reply -->
--
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]
