michael-s-molina commented on code in PR #35259: URL: https://github.com/apache/superset/pull/35259#discussion_r2528713969
########## superset-core/src/superset_core/__init__.py: ########## @@ -14,3 +14,7 @@ # KIND, either express or implied. See the License for the # specific language governing permissions and limitations # under the License. + +""" +Apache Superset Core - Public API for Extension Development Review Comment: It's not exclusive for extension development as it will be used by MCP tools as well. ```suggestion Apache Superset Core - Public API with core functions of Superset ``` ########## superset-core/src/superset_core/api/daos.py: ########## @@ -0,0 +1,262 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Data Access Object API for superset-core. + +Provides dependency-injected DAO classes that will be replaced by +host implementations during initialization. + +Usage: + from superset_core.api.daos import DatasetDAO, DatabaseDAO + + # Use standard BaseDAO methods + datasets = DatasetDAO.find_all() + dataset = DatasetDAO.find_one_or_none(id=123) + DatasetDAO.create(attributes={"name": "New Dataset"}) +""" + +from abc import ABC, abstractmethod +from typing import Any, ClassVar, Generic, TypeVar + +from flask_appbuilder.models.filters import BaseFilter +from sqlalchemy.orm import Query as SQLAQuery + +from superset_core.api.models import ( + Chart, + CoreModel, + Dashboard, + Database, + Dataset, + KeyValue, + Query, + SavedQuery, + Tag, + User, +) + +# Type variable bound to our CoreModel +T = TypeVar("T", bound=CoreModel) + + +class BaseDAO(Generic[T], ABC): + """ + Abstract base class for DAOs. + + This ABC defines the base that all DAOs should implement, + providing consistent CRUD operations across Superset and extensions. + """ + + # Due to mypy limitations, we can't have `type[T]` here + model_cls: ClassVar[type[Any] | None] + base_filter: ClassVar[BaseFilter | None] + id_column_name: ClassVar[str] + uuid_column_name: ClassVar[str] + + @classmethod + @abstractmethod + def find_all(cls) -> list[T]: + """Get all entities that fit the base_filter.""" + ... + + @classmethod + @abstractmethod + def find_one_or_none(cls, **filter_by: Any) -> T | None: + """Get the first entity that fits the base_filter.""" + ... + + @classmethod + @abstractmethod + def create( Review Comment: @villebro I wonder if we're exposing the correct layer as a public API... For example, when we expose create/update commands of the DAO layer, we allow MCP tools and extensions to modify these objects bypassing the logic defined at the commands layer. Using DELETE as an example, the `DeleteChart` command enforces a restriction where you can't delete a chart if there are associated alerts or reports. By exposing the delete method from the DAO layer, extensions or MCP tools could bypass this restriction. I wonder if the commands layer is the one that should be exposed instead. WDYT? ########## superset-core/src/superset_core/api/models.py: ########## @@ -0,0 +1,295 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +""" +Model API for superset-core. + +Provides model classes that will be replaced by host implementations +during initialization for extension developers to use. + +Usage: + from superset_core.api.models import Dataset, Database, get_session + + # Use as regular model classes + dataset = Dataset(name="My Dataset") + db = Database(database_name="My DB") + session = get_session() +""" + +from datetime import datetime +from typing import Any +from uuid import UUID + +from flask_appbuilder import Model +from sqlalchemy.orm import scoped_session + + +class CoreModel(Model): + """ + Abstract base class that extends Flask-AppBuilder's Model. + + This base class provides the interface contract for all Superset models. + The host package provides concrete implementations. + """ + + __abstract__ = True + + +class Database(CoreModel): + """ + Abstract class for Database models. + + This abstract class defines the contract that database models should implement, + providing consistent database connectivity and metadata operations. + """ + + __abstract__ = True + + id: int + verbose_name: str + database_name: str | None + + @property + def name(self) -> str: + raise NotImplementedError + + @property + def backend(self) -> str: + raise NotImplementedError + + @property + def data(self) -> dict[str, Any]: + raise NotImplementedError + + +class Dataset(CoreModel): + """ + Abstract class for Dataset models. + + This abstract class defines the contract that dataset models should implement, + providing consistent data source operations and metadata. + + It provides the public API for Datasets implemented by the host application. + """ + + __abstract__ = True + + # Type hints for expected attributes (no actual field definitions) + id: int Review Comment: Should we reduce the scope of what's exposed in these models? In other words, should all these attributes be part of the public API initially? -- 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]
