codeant-ai-for-open-source[bot] commented on code in PR #34875:
URL: https://github.com/apache/superset/pull/34875#discussion_r4152204269
##########
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:
**Suggestion:** The OAuth check ignores engine-level OAuth configuration, so
OAuth databases without per-database JSON incorrectly trigger catalog
connections during migration.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Security`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=d8de073e79854f5a917a2a83ba7565f0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=d8de073e79854f5a917a2a83ba7565f0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/migrations/shared/catalogs.py
**Line:** 74:74
**Comment:**
*Security: The OAuth check ignores engine-level OAuth configuration, so
OAuth databases without per-database JSON incorrectly trigger catalog
connections during migration.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34875&comment_hash=f4a829541b15d0e4360e939dc89b59c332ac3795f8a51ea7194f6aad3bbcfa60&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34875&comment_hash=f4a829541b15d0e4360e939dc89b59c332ac3795f8a51ea7194f6aad3bbcfa60&reaction=dislike'>๐</a>
##########
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:
**Suggestion:** `get_inspector` returns an `Inspector`, not a context
manager, so both `with` statements raise `TypeError` and schema or catalog
discovery always fails.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=a47f788135654545837aa2ec285df2e0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=a47f788135654545837aa2ec285df2e0&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/migrations/shared/catalogs.py
**Line:** 93:94
**Comment:**
*Api Mismatch: `get_inspector` returns an `Inspector`, not a context
manager, so both `with` statements raise `TypeError` and schema or catalog
discovery always fails.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34875&comment_hash=71b48fcb041cd4c611746286eeb130630f5e490f757c2f2c9d0b9e9ed148df2b&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34875&comment_hash=71b48fcb041cd4c611746286eeb130630f5e490f757c2f2c9d0b9e9ed148df2b&reaction=dislike'>๐</a>
##########
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:
**Suggestion:** `get_default_catalog` passes the local model to engine specs
that read `database.url_object`, so PostgreSQL and BigQuery migrations raise
`AttributeError`.
**Assessment:** ๐ด `Critical` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=fc3ec06fc92442dda75d334723f27422&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=fc3ec06fc92442dda75d334723f27422&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/migrations/shared/catalogs.py
**Line:** 67:67
**Comment:**
*Api Mismatch: `get_default_catalog` passes the local model to engine
specs that read `database.url_object`, so PostgreSQL and BigQuery migrations
raise `AttributeError`.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34875&comment_hash=c3252e4cbf4c86f8d7319cd61a0c1c50e75b8ef8256658aafe874adfc662ecad&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34875&comment_hash=c3252e4cbf4c86f8d7319cd61a0c1c50e75b8ef8256658aafe874adfc662ecad&reaction=dislike'>๐</a>
##########
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:
**Suggestion:** `encrypted_extra` is read as plaintext JSON, but the
database column stores encrypted data, so normal nonempty values cause JSON
decoding errors during migration.
**Assessment:** ๐ด `Critical` ยท ๐ `Occurrence: Often` ยท ๐ท๏ธ `Type error`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=700216fc06894c0995dbb8ab75fb51ad&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=700216fc06894c0995dbb8ab75fb51ad&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/migrations/shared/catalogs.py
**Line:** 73:73
**Comment:**
*Type Error: `encrypted_extra` is read as plaintext JSON, but the
database column stores encrypted data, so normal nonempty values cause JSON
decoding errors during migration.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34875&comment_hash=5e12a64a1729257b8851f93dd3cecb5177b54e6db7e294fbc6fd735d99dc3b91&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34875&comment_hash=5e12a64a1729257b8851f93dd3cecb5177b54e6db7e294fbc6fd735d99dc3b91&reaction=dislike'>๐</a>
##########
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:
**Suggestion:** The local model passes raw URI and lacks production engine
setup, so encrypted credentials, OAuth tokens, connection parameters, and
catalog adjustments are bypassed during inspection.
**Assessment:** ๐ `Major` ยท ๐ `Occurrence: Sometimes` ยท ๐ท๏ธ `Api mismatch`
[](https://docs.codeant.ai/cli/resolve-pr-comments-skill)
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=62d759c80b79438f8c9b7c3127e8b666&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=62d759c80b79438f8c9b7c3127e8b666&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
<details>
<summary><b>Prompt for AI Agent ๐ค </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/migrations/shared/catalogs.py
**Line:** 80:81
**Comment:**
*Api Mismatch: The local model passes raw URI and lacks production
engine setup, so encrypted credentials, OAuth tokens, connection parameters,
and catalog adjustments are bypassed during inspection.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34875&comment_hash=6bd8cabc885bfe294cc81f6879dd8026e83daec4a1563ea1a5e0b29a33d8e87b&reaction=like'>๐</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F34875&comment_hash=6bd8cabc885bfe294cc81f6879dd8026e83daec4a1563ea1a5e0b29a33d8e87b&reaction=dislike'>๐</a>
--
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]
