chore: abstract models and daos into superset-core #35259

villebro · 2025-09-24T04:38:03Z

SUMMARY

WIP

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

korbit-ai

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Category	Issue	Status
	Incomplete Abstract Interface Definition ▹ view	🧠 Incorrect
	Empty models module missing required exports ▹ view	🧠 Not in standard
	Missing batching guidance for bulk ID queries ▹ view	🧠 Not in standard

Files scanned

File Path	Reviewed
superset-core/src/superset_core/init.py	✅
superset-core/src/superset_core/commands/init.py	✅
superset-core/src/superset_core/dao/init.py	✅
superset-core/src/superset_core/models/init.py	✅
superset-core/src/superset_core/commands/types.py	✅
superset-core/src/superset_core/models/base.py	✅
superset/core/api/types/models.py	✅
superset-core/src/superset_core/api/types/models.py	✅
superset/commands/importers/v1/init.py	✅
superset-core/src/superset_core/dao/types.py	✅
superset/daos/base.py	✅
superset/models/core.py	✅
superset/connectors/sqla/models.py	✅

Explore our documentation to understand the languages and file types we support and the files we ignore.

Check out our docs on how you can make Korbit work best for you and your team.

Loving Korbit!? Share us on LinkedIn Reddit and X

superset-core/src/superset_core/models/base.py

superset-core/src/superset_core/models/__init__.py

superset-core/src/superset_core/dao/types.py

betodealmeida · 2025-09-24T22:27:17Z

superset-core/src/superset_core/dao/types.py

+
+    # Class attributes that implementations should define
+    model_cls: Optional[type[T_Model]]
+    base_filter: Optional[BaseFilter]


Can we use something more generic here instead of BaseFilter, which is tightly coupled with FAB? Maybe we could rething how base filter works — it could be something passed to the instance, eg:

dao = DatasetDAO() datasets = dao.find_by_ids([1, 2]) # equivalent to `skip_base_filter=True` dao_for_user = dao.filtered(User.id == current_user.id) datasets = dao_for_user.find_by_ids([1, 2]) # equivalent to `skip_base_filter=False`

In general I think the less dependencies superset-core has, the better.

Fluent Interfaces are cool! but like that makes me think why not just use Query from SQLAlchemy?
Have a somewhat static base filter is a plus for security, we enforce that all DAO operations take into account a certain filter that enforces a security constraint

betodealmeida · 2025-09-24T22:29:46Z

superset-core/src/superset_core/dao/types.py

+    @abstractmethod
+    def find_by_ids(
+        self,
+        model_ids: Union[list[str], list[int]],


Can we use Collection instead of list? Here and in other places where order doesn't matter.

Also, can we use this opportunity to just use UUIDs everywhere, instead of having int IDs, string IDs, and UUIDs?

mistercrunch · 2025-09-24T22:54:51Z

A tiny bit concerned about having superset-core treated (explicitely or implicitely) as a wide public API and the change management concerns that come with that. To help manage that, I'd propose that we:

use python convention for private/public (_method= you probably shouldn't use that if care about easy upgrade, this is meant for the internal ecosystem) and __method -> definitely don't want to use that, even in the ecosystem/monorepo
actively reduce the API surface, go through all the methods and "privatize" what's not essential in the public API
make superset-core pre-1.0 and allow ourselves to break API and privatize things until 1.0 is officially out. From that point onwards we do proper semver/change-management on public methods/objects/properties

mistercrunch · 2025-09-24T23:03:58Z

About Commands more specifically, I can see how some are useful here, but thinking most should either live in the app, or be treated as private.

mistercrunch · 2025-09-24T23:06:50Z

Note: another option I mentioned before would be to treat superset-core as 100% private, and have something like a superset-extension-sdk as the thing that's public/semvered, it would import/re-export the bits of superset-core we want to actively expose (and maintain) as a public API.

dpgaspar · 2025-09-25T08:59:32Z

superset-core/src/superset_core/api/types/models.py

    @staticmethod
    @abstractmethod
-    def get_datasets(query: BaseQuery | None = None, **kwargs: Any) -> list[Any]:
+    def get_datasets(query: BaseQuery | None = None, **kwargs: Any) -> list[Dataset]:


nit: We can use Query from SQLAlchemy

dpgaspar · 2025-09-25T09:02:15Z

superset-core/src/superset_core/dao/types.py

+    @abstractmethod
+    def find_by_id(
+        self, model_id: Union[str, int], skip_base_filter: bool = False
+    ) -> Optional[T_Model]:


nit: In the future we could have models with composite keys

dpgaspar · 2025-09-25T09:04:19Z

superset-core/src/superset_core/models/base.py

+    __abstract__ = True
+
+
+class Database(CoreModel):


Should we call this AbstractDatabase or BaseDatabase ?

michael-s-molina · 2025-09-25T16:45:02Z

A tiny bit concerned about having superset-core treated (explicitely or implicitely) as a wide public API and the change management concerns that come with that.

@mistercrunch I share the same concerns as you and this is one of the motivations for exposing only the API definitions in superset-core without any implementation or private methods. In essence, the APIs should only contain public methods that we will support with semver. Any private method will not be part of superset-core and will be defined at the host implementation. For example:

ISomeDAO: (superset-core)
     @abstractmethod
     def public_function():
          ...

SomeDAO(ISomeDAO):  (host)
    def public_function():
         ... implementation logic
    
    def private_function():
         ... implementation logic

make superset-core pre-1.0 and allow ourselves to break API and privatize things until 1.0 is officially out. From that point onwards we do proper semver/change-management on public methods/objects/properties

That's exactly what we defined in the SIP and the reason we publish the packages as 0.0.1-rcx in both NPM and PyPI.

michael-s-molina · 2025-09-25T16:53:15Z

Note: another option I mentioned before would be to treat superset-core as 100% private, and have something like a superset-extension-sdk as the thing that's public/semvered, it would import/re-export the bits of superset-core we want to actively expose (and maintain) as a public API.

@mistercrunch I think we're achieving the same objective but through different means. The superset-core contains only the bits we want to actively expose and the host application contain the private parts. The private parts don't need to be a separate package, they are already part of the apache-superset package.

Correlating with your example, superset-core is what you're calling as superset-extension-sdk but it's not only for extensions. It can be used by embedded and MCP tools.

mistercrunch · 2025-09-25T19:24:04Z

the APIs should only contain public methods

but clearly there are useful bits/methods/objects that we want to use/reuse within superset-core but don't want to or don't need to expose as part of the public API. I say we just put an underscore in their name to signal they're not part of the public api.

One example might be something like _is_uuid_or_int(any_id: int | str), clearly it's something we need to use/reuse, but really no need to make that public...

Python really lacking here, though doing it by convention with _ and __ prefix is fine.

Also restating that I think there are many pieces of logic that logically belong in superset-core/ (especially if we want to keep the very-related logic together, in one place) that are logically private or only meant for the main app.

Two main approaches here:

have those bits of logic live in superset-core, close to the code they are highly related-to and marked as private
crazy inheritance schemes (some highly-related DAOs/Commands live in superset-core, and superset-app inherits-and-augments them) -> not my favorite approach! ... Some commands are in one module/app, others are in another, calling super() or methods from the parent class, .... = inheritance confusion

aminghadersohi · 2025-09-26T01:47:01Z

superset-core/src/superset_core/dao/types.py

+    @abstractmethod
+    def find_by_id(
+        self, model_id: Union[str, int], skip_base_filter: bool = False
+    ) -> Optional[T_Model]:


Regarding Optional[T_Model], consider: many ORMs provide get() (raises) and get_or_none() (returns None) to handle different use cases. Do we want/need both of these or are we good with what is here?

When debugging missing entities, would we rather see AttributeError: 'NoneType' object has no attribute 'name' on line 47, or EntityNotFoundError: Dashboard with id=123 not found on line 23?

villebro requested review from michael-s-molina, geido, eschutho, rusackas and kgabryje as code owners September 24, 2025 04:38

pull-request-size bot added the size/L label Sep 24, 2025

dosubot bot added the change:backend Requires changing the backend label Sep 24, 2025

korbit-ai bot reviewed Sep 24, 2025

View reviewed changes

superset-core/src/superset_core/models/base.py Show resolved Hide resolved

superset-core/src/superset_core/models/__init__.py Show resolved Hide resolved

superset-core/src/superset_core/dao/types.py Show resolved Hide resolved

chore: abstract models and daos into superset-core

87b387a

villebro force-pushed the villebro/core-protocols branch from 754388d to 87b387a Compare September 24, 2025 04:40

villebro marked this pull request as draft September 24, 2025 04:44

rusackas added the review:draft label Sep 24, 2025

betodealmeida reviewed Sep 24, 2025

View reviewed changes

dpgaspar reviewed Sep 25, 2025

View reviewed changes

aminghadersohi reviewed Sep 26, 2025

View reviewed changes

chore: abstract models and daos into superset-core #35259

Are you sure you want to change the base?

chore: abstract models and daos into superset-core #35259

Uh oh!

Conversation

villebro commented Sep 24, 2025

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

korbit-ai bot left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Review by Korbit AI

Korbit automatically attempts to detect when you fix issues in new commits.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

betodealmeida Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dpgaspar Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

betodealmeida Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mistercrunch commented Sep 24, 2025

Uh oh!

mistercrunch commented Sep 24, 2025

Uh oh!

mistercrunch commented Sep 24, 2025

Uh oh!

dpgaspar Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

dpgaspar Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

dpgaspar Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

michael-s-molina commented Sep 25, 2025

Uh oh!

michael-s-molina commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mistercrunch commented Sep 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aminghadersohi Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

korbit-ai bot left a comment •

edited

Loading

betodealmeida Sep 24, 2025 •

edited

Loading

betodealmeida Sep 24, 2025 •

edited

Loading

michael-s-molina commented Sep 25, 2025 •

edited

Loading

mistercrunch commented Sep 25, 2025 •

edited

Loading