-
Notifications
You must be signed in to change notification settings - Fork 13.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(dao): Add explicit ON DELETE CASCADE when deleting datasets #24488
chore(dao): Add explicit ON DELETE CASCADE when deleting datasets #24488
Conversation
ab461d7
to
81224e0
Compare
Codecov Report
@@ Coverage Diff @@
## master #24488 +/- ##
==========================================
+ Coverage 69.03% 69.04% +0.01%
==========================================
Files 1901 1901
Lines 74006 74020 +14
Branches 8115 8119 +4
==========================================
+ Hits 51088 51107 +19
+ Misses 20807 20805 -2
+ Partials 2111 2108 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
81224e0
to
d8b9191
Compare
@@ -364,17 +364,6 @@ def create_metric( | |||
@staticmethod | |||
def bulk_delete(models: Optional[list[SqlaTable]], commit: bool = True) -> None: | |||
item_ids = [model.id for model in models] if models else [] | |||
# bulk delete, first delete related data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer needed as the database will handle said logic.
try: | ||
# Even though SQLAlchemy should in theory delete rows from the association |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@betodealmeida this should address the problem you were trying to circumvent in #23414.
d8b9191
to
d1b2033
Compare
@@ -189,11 +189,6 @@ def test_extra_cache_keys(self, flask_g): | |||
self.assertTrue(table3.has_extra_cache_key_calls(query_obj)) | |||
assert extra_cache_keys == ["abc"] | |||
|
|||
# Cleanup |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI was failing for PostgreSQL (though not MySQL or SQLite—which is a little perplexing) with SQLAlchemy error which would indicate that we're trying to delete objects which previously weren't committed to the database. Given how these tables are constructed,
table3 = SqlaTable(
table_name="test_has_no_extra_cache_keys_table",
sql=query,
database=get_example_database(),
)
and there's no clear add/commit call it seems that SQLAlchemy's viewpoint is right, i.e., there's no cleanup required. If I'm right then this is a win for SIP-92. I suspect throughout the code and tests we're doing numerous database operations inefficiently with superfluous commits, not rolling back test state, etc.
5600f4e
to
52afb2c
Compare
try: | ||
DatasetDAO.bulk_delete(self._models) | ||
for model in self._models: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is logic taken #24466. Previously a test was failing because the relationship no longer existed in Python after the commit. See here for details. The issue was that:
DatasetDAO.bulk_delete(self._models)
includes a db.session.commit()
in which all the attributes have expired. I'm not really sure why this worked before, but the TL;DR is this logic has been moved inside the DAO and is executed prior to the commit.
dd031bd
to
d138de0
Compare
superset/utils/core.py
Outdated
if branch: | ||
# 'branch' refers to a sub-connection of a connection, | ||
# we don't want to bother pinging on these. | ||
return | ||
|
||
if connection.dialect.name == "sqlite": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See here for why this is needed. I do wonder if we should drop support for SQLite given it's not supported in production. We already skip a bunch of tests, i.e.,
if backend() == "sqlite":
return
possibly because of how the foreign key constraints have no impact on the underlying table.
09a83c8
to
ab9e94a
Compare
@@ -849,6 +851,19 @@ def ping_connection(connection: Connection, branch: bool) -> None: | |||
# restore 'close with result' | |||
connection.should_close_with_result = save_should_close_with_result | |||
|
|||
if some_engine.dialect.name == "sqlite": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this is why a number of tests currently have,
if backend() == "sqlite":
return
986ac75
to
355c177
Compare
355c177
to
bc60158
Compare
@@ -430,7 +425,7 @@ def test_multiple_sql_statements_raises_exception(self): | |||
} | |||
|
|||
table = SqlaTable( | |||
table_name="test_has_extra_cache_keys_table", | |||
table_name="test_multiple_sql_statements", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comment above. SQLite was complaining that said table already existed when I removed the "cleanup", though neither MySQL nor PostgreSQL did. I'm not entirely sure what's going on here, but it does seem like the table name was copypasta from the other test and thus the easiest solution was to rename the table in accordance with the test.
I sense with this refactoring work it's a case of two steps forwards, one step back at time.
if some_engine.dialect.name == "sqlite": | ||
|
||
@event.listens_for(some_engine, "connect") | ||
def set_sqlite_pragma( # pylint: disable=unused-argument |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hopefully (if approved) this will remove the need for this logic in a number of tests.
Totally agree with this philosophy, except for situations where you really want to leave FK data hanging. IMO, the DB should keep things clean wherever possible and should enforce referential integrity (kinda the point of FK's in the first place :D ). Delegating to the DB to deal with cleanup keeps app-tier logic cleaner and less buggy as it's less concerned with cascades and what not. |
mapper = next(iter(cls.model_cls.registry.mappers)) # type: ignore | ||
|
||
for model in models: | ||
security_manager.dataset_after_delete(mapper, connection, model) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we wouldn't reference the SM from the DAO layer as it's more of a mid-tier concern. This belongs in the CMD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@craig-rueda could you help educate me as to why this is? Granted that the DAO is only abstraction from the database (and not the security manager which in turns interfaces with the database), but isn't this the interface we expect commands, APIs, etc. to interface with? Note this logic would be handled by the after_delete
event listener if we were using the ORM, as opposed to bulk queries, to delete the assets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, DAO's should be super light, only concerned with interfacing with the DB. They shouldn't have dependencies upwards (to the mid-tier), which can lead to circular dependencies as basically all mid-tier logic will at some point need a DAO. This can obviously be worked around using some late-binding technique.
From a semantic POV, the command is just asking to "please delete these records". Calling the SM to do additional work here is a side effect, which ultimately makes this code less reusable for lower level operations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@craig-rueda I generally agree, but how does this differ from the DAO asking "please delete these records" which triggers the SQLAlchemy ORM to handle post deletion events (per the after_delete event handler) which in turn invokes the security manager?
Note we typically use these SQLAlchemy events because of short comings with either our data model and/or constructs which can't be modeled by our database schema.
I guess what I’m saying is I would expect the DAO to behave in the same way whether it was deleting a single entity or bulk deleting multiple entities and explicitly calling the affer_delete
callback (which is only invoked for the former) ensures that the behavior is consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, either way. It's more of a stylistic thing IMO. Keeping the DAO layer as decoupled as possible has worked well for me in the past, hence my comments :).
"fk": "fk_%(table_name)s_%(column_0_name)s_%(referred_table_name)s", | ||
} | ||
|
||
for table in ("sql_metrics", "table_columns"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
SUMMARY
Per SIP-92 Proposal for restructuring the Python code base #24331 organized all the DAOs to be housed within a shared folder—the result of which highlighted numerous inconsistencies, repetition, and inefficiencies.
Personally I really struggle at times with the SQLAlchemy model especially with regards to the relationship logic and why the cascade paradigm only impacts the SQLAlchemy ORM, i.e., isn't materialized to the actual table schema. The TL;DR is there always seems to be multiple ways to skin the cat which just adds to the confusion.
Something clearly is quite right with our model as it's often the case when deleting a dataset we first delete the owners, columns, and metrics to avoid foreign key constrain violations. I gather this is because of our bulk delete logic (see this discussion for more detail) bypasses the SQLAlchemy ORM or there are other gremlins at play—there is this issue reported on Stack Overflow regarding cascading for association tables.
I recently added the DAO-Style-Guidelines-and-Best-Practices wiki page, and I hope there is general agreement in item 5 which mentions the "shift left" mentality where the data model should persist in the database schema than rely on Python logic per the SQLAlchemy ORM. The beauty of this approach is it simplifies the codebase and mental model and ensures that the correct logic is adhered to if/when executing SQL outside of the ORM (be that using the SQLAlchemy bulk workflows, executing SQL against the DB-API, using the MySQL/PostgreSQL CLI, etc.).
The logic I employed follows this SQLAlchemy documentation. Per their suggestion I opted for
passive_deletes=True
as we don't require the ORM to load the columns, metrics, and owners (if lazy loaded) when deleting the dataset.The TL;DR is prior to adding the migration CI was failing so it seems like this approach feels somewhat correct.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
CI.
ADDITIONAL INFORMATION