From e76803fa7648a9a7c792a78cada7c35ea0a26a98 Mon Sep 17 00:00:00 2001 From: Jeff Zohrab Date: Sat, 21 Dec 2024 23:12:40 -0600 Subject: [PATCH] Issue 460: Set cascade delete pragma on connect. - add test to ensure stray book stats are deleted - data cleanup migration (one-time only) - remove LanguageRepo.delete, deletes now cascade correctly --- lute/app_factory.py | 1 + ...0241221_clean_up_missing_relationships.sql | 25 +++++++++++++++++++ lute/language/routes.py | 6 ++--- lute/models/repositories.py | 23 ----------------- tests/orm/test_Book.py | 10 +++++++- tests/orm/test_Language.py | 4 +-- 6 files changed, 40 insertions(+), 29 deletions(-) create mode 100644 lute/db/schema/migrations/20241221_clean_up_missing_relationships.sql diff --git a/lute/app_factory.py b/lute/app_factory.py index 8266e25c..eaf10c0b 100644 --- a/lute/app_factory.py +++ b/lute/app_factory.py @@ -314,6 +314,7 @@ def _create_app(app_config, extra_config): @listens_for(Pool, "connect") def _pragmas_on_connect(dbapi_con, con_record): # pylint: disable=unused-argument dbapi_con.execute("pragma recursive_triggers = on;") + dbapi_con.execute("pragma foreign_keys = on;") with app.app_context(): db.create_all() diff --git a/lute/db/schema/migrations/20241221_clean_up_missing_relationships.sql b/lute/db/schema/migrations/20241221_clean_up_missing_relationships.sql new file mode 100644 index 00000000..563e5944 --- /dev/null +++ b/lute/db/schema/migrations/20241221_clean_up_missing_relationships.sql @@ -0,0 +1,25 @@ +-- Clean up bad data, where relationships are invalid. +-- +-- Per issue 460, the pragma foreign_keys was not ON, so it's possible +-- (though unlikely) that some data in the db is bad/unreachable. +-- +-- This is being done as a one-time fix, rather than as a repeatable +-- migration, as it would be very annoying if the data model changed +-- and I forgot to update the script!! + +DELETE FROM languagedicts WHERE LdLgID NOT IN (SELECT LgID FROM languages); +DELETE FROM wordsread WHERE WrLgID NOT IN (SELECT LgID FROM languages); + +DELETE FROM books WHERE BkLgID NOT IN (SELECT LgID FROM languages); +DELETE FROM bookstats WHERE BkID NOT IN (SELECT BkID FROM books); +DELETE FROM booktags WHERE BtBkID NOT IN (SELECT BkID FROM books) OR BtT2ID NOT IN (SELECT T2ID FROM tags2); + +DELETE FROM texts WHERE TxBkID NOT IN (SELECT BkID FROM books); +DELETE FROM textbookmarks WHERE TbTxID NOT IN (SELECT TxID FROM texts); +DELETE FROM sentences WHERE SeTxID NOT IN (SELECT TxID FROM texts); +DELETE FROM wordsread WHERE WrTxID IS NOT NULL AND WrTxID NOT IN (SELECT TxID FROM texts); + +DELETE FROM wordtags WHERE WtWoID NOT IN (SELECT WoID FROM words) OR WtTgID NOT IN (SELECT TgID FROM tags); +DELETE FROM wordimages WHERE WiWoID NOT IN (SELECT WoID FROM words); +DELETE FROM wordflashmessages WHERE WfWoID NOT IN (SELECT WoID FROM words); +DELETE FROM wordparents WHERE WpWoID NOT IN (SELECT WoID FROM words) OR WpParentWoID NOT IN (SELECT WoID FROM words); diff --git a/lute/language/routes.py b/lute/language/routes.py index 294e1816..d664abf0 100644 --- a/lute/language/routes.py +++ b/lute/language/routes.py @@ -6,7 +6,7 @@ from sqlalchemy.exc import IntegrityError from flask import Blueprint, current_app, render_template, redirect, url_for, flash from lute.models.language import Language -from lute.models.repositories import LanguageRepository, UserSettingRepository +from lute.models.repositories import UserSettingRepository from lute.language.service import Service from lute.language.forms import LanguageForm from lute.db import db @@ -156,8 +156,8 @@ def delete(langid): language = db.session.get(Language, langid) if not language: flash(f"Language {langid} not found") - r = LanguageRepository(db.session) - r.delete(language) + db.session.delete(language) + db.session.commit() return redirect(url_for("language.index")) diff --git a/lute/models/repositories.py b/lute/models/repositories.py index be3aa40d..ad075a9a 100644 --- a/lute/models/repositories.py +++ b/lute/models/repositories.py @@ -141,29 +141,6 @@ def find_by_name(self, name): .first() ) - def delete(self, language): - """ - Hacky method to delete language and all terms, books, and dicts - associated with it. - - There is _certainly_ a better way to do this using - Sqlalchemy relationships and cascade deletes, but I - was running into problems with it (things not cascading, - or warnings ("SAWarning: Object of type not in - session, add operation along 'Language.terms' will not - proceed") during test runs. It would be nice to have - a "correct" mapping, but this is good enough for now. - - TODO zzfuture fix: fix Language-Book and -Term mappings. - """ - sqls = [ - "pragma foreign_keys = ON", - f"delete from languages where LgID = {language.id}", - ] - for s in sqls: - self.session.execute(sqltext(s)) - self.session.commit() - def all_dictionaries(self): "All dictionaries for all languages." lang_dicts = {} diff --git a/tests/orm/test_Book.py b/tests/orm/test_Book.py index 8a089be8..11873f8b 100644 --- a/tests/orm/test_Book.py +++ b/tests/orm/test_Book.py @@ -6,6 +6,7 @@ import pytest from lute.models.book import Book, BookTag, TextBookmark, BookStats from lute.read.service import Service +from lute.book.stats import Service as BookStatsService from lute.db import db from tests.dbasserts import assert_sql_result, assert_record_count_equals @@ -56,10 +57,17 @@ def test_delete_book(empty_db, simple_book): service.mark_page_read(b.id, 1, False) service.mark_page_read(b.id, 1, True) + bss = BookStatsService(db.session) + bss.refresh_stats() + + check_tables = ["books", "bookstats", "texts", "sentences", "booktags"] + for t in check_tables: + assert_record_count_equals(t, 1, f"{t} created") + db.session.delete(b) db.session.commit() - for t in ["books", "texts", "sentences", "booktags"]: + for t in check_tables: assert_record_count_equals(t, 0, f"{t} deleted") sql = "select * from tags2" diff --git a/tests/orm/test_Language.py b/tests/orm/test_Language.py index f533a8d3..1b6fcae7 100644 --- a/tests/orm/test_Language.py +++ b/tests/orm/test_Language.py @@ -122,8 +122,8 @@ def test_delete_language_removes_book_and_terms(app_context, spanish): assert_sql_result(sqldict, ["something?[LUTE]"], "dict") assert_record_count_equals("select * from wordsread", 1, "saved") - repo = LanguageRepository(db.session) - repo.delete(spanish) + db.session.delete(spanish) + db.session.commit() assert_sql_result(sqlbook, [], "book deleted") assert_sql_result(sqlterms, [], "terms deleted")