Index: doc/changelog.md ================================================================== --- doc/changelog.md +++ doc/changelog.md @@ -1,8 +1,7 @@ # Changelog - # Next version (in development) ## New features - Proper, full support for oversized cards, like Archenemy schemes or Planechase plane cards. Regular cards and larger @@ -12,10 +11,12 @@ When the image is downloaded, it will be treated as a regular card, even if the deck import wizard warns about it being potentially oversized. ## Fixed issues +- Significantly optimized card database size and import speed. + (The database now takes roughly 25% less time to update and uses about 30% less disk space) - Fixed the “Remove selected” cards button in the deck list importer unexpectedly staying active when clicked while multiple cells of the same row in the card table were selected. # Version 0.17.0 (2022-06-13) Index: mtg_proxy_printer/card_info_downloader.py ================================================================== --- mtg_proxy_printer/card_info_downloader.py +++ mtg_proxy_printer/card_info_downloader.py @@ -288,10 +288,14 @@ set_wackiness_score_cache: typing.Dict[str, SetWackinessScore] = {} skipped_cards = 0 index = 0 face_ids: IntTuples = [] db: sqlite3.Connection = self.model.db + # Will be re-populated while iterating over the card data. Axing the previous data is far cheaper than trying + # to update it in-place by removing up to number-of-available-filters entries per each individual card, + # just to make sure that rare un-banned cards are updated properly. + db.execute("DELETE FROM PrintingDisplayFilter\n") for index, card in enumerate(card_data, start=1): if not self.should_run: logger.info(f"Aborting card import after {index} cards due to user request.") self.download_finished.emit() return index @@ -574,16 +578,13 @@ def _insert_card_filters( model: CardDatabase, printing_id: int, filter_data: typing.Dict[str, bool], printing_filter_ids: typing.Dict[str, int]): model.db.executemany( - cached_dedent("""\ - INSERT OR REPLACE INTO PrintingDisplayFilter (printing_id, filter_id, filter_applies) - VALUES (?, ?, ?) - """), - ((printing_id, printing_filter_ids[filter_name], filter_applies) - for filter_name, filter_applies in filter_data.items()) + "INSERT INTO PrintingDisplayFilter (printing_id, filter_id) VALUES (?, ?)\n", + ((printing_id, printing_filter_ids[filter_name]) + for filter_name, filter_applies in filter_data.items() if filter_applies) ) def _should_skip_card(card: JSONType) -> bool: # Cards without images. These have no "image_uris" item can’t be printed at all. Unconditionally skip these Index: mtg_proxy_printer/model/carddb.py ================================================================== --- mtg_proxy_printer/model/carddb.py +++ mtg_proxy_printer/model/carddb.py @@ -749,11 +749,11 @@ in self.db.execute("SELECT filter_name, filter_active FROM DisplayFilters").fetchall() } filters_in_settings: typing.Dict[str, bool] = {key: section.getboolean(key) for key in section.keys()} return filters_in_settings != filters_in_db - @profile + @profile # TODO: This decorator is unnecessary def _remove_old_printing_filters(self, section) -> bool: stored_filters = { filter_name for filter_name, in self.db.execute("SELECT filter_name FROM DisplayFilters").fetchall() } known_filters = set(section.keys()) @@ -769,14 +769,16 @@ @profile def _update_cached_data(self, progress_signal: typing.Callable[[int], None]): logger.debug("Update the Printing.is_hidden column") self.db.execute(cached_dedent("""\ UPDATE Printing -- _update_cached_data() - SET is_hidden = HiddenPrintings.should_be_hidden - FROM HiddenPrintings - WHERE Printing.printing_id = HiddenPrintings.printing_id - AND Printing.is_hidden <> HiddenPrintings.should_be_hidden + SET is_hidden = Printing.printing_id IN ( + SELECT HiddenPrintingIDs.printing_id FROM HiddenPrintingIDs + ) + WHERE is_hidden <> (Printing.printing_id IN ( + SELECT HiddenPrintingIDs.printing_id FROM HiddenPrintingIDs + )) ; """)) progress_signal(2) logger.debug("Update the FaceName.is_hidden column") self.db.execute(cached_dedent("""\ Index: mtg_proxy_printer/model/carddb.sql ================================================================== --- mtg_proxy_printer/model/carddb.sql +++ mtg_proxy_printer/model/carddb.sql @@ -12,11 +12,11 @@ -- You should have received a copy of the GNU General Public License -- along with this program. If not, see . -PRAGMA user_version = 0000028; +PRAGMA user_version = 0000029; PRAGMA foreign_keys = on; BEGIN TRANSACTION; CREATE TABLE PrintLanguage ( @@ -107,18 +107,18 @@ CREATE TABLE PrintingDisplayFilter ( -- Stores which filter applies to which printing. printing_id INTEGER NOT NULL REFERENCES Printing (printing_id) ON DELETE CASCADE, filter_id INTEGER NOT NULL REFERENCES DisplayFilters (filter_id) ON DELETE CASCADE, - filter_applies INTEGER NOT NULL CHECK (filter_applies IN (TRUE, FALSE)), PRIMARY KEY (printing_id, filter_id) ) WITHOUT ROWID; -CREATE VIEW HiddenPrintings AS - SELECT printing_id, sum(filter_applies * filter_active) > 0 AS should_be_hidden +CREATE VIEW HiddenPrintingIDs AS +SELECT printing_id FROM PrintingDisplayFilter JOIN DisplayFilters USING (filter_id) + WHERE filter_active IS TRUE GROUP BY printing_id ; CREATE TABLE LastImageUseTimestamps ( -- Used to store the last image use timestamp and usage count of each image. @@ -156,16 +156,15 @@ AND FaceName.is_hidden IS FALSE ; CREATE VIEW AllPrintings AS SELECT card_name, set_code, set_name, "language", collector_number, scryfall_id, highres_image, face_number, - is_front, is_oversized, png_image_uri, oracle_id, release_date, wackiness_score, Printing.is_hidden, - release_date + is_front, is_oversized, png_image_uri, oracle_id, release_date, wackiness_score, Printing.is_hidden FROM Card JOIN Printing USING (card_id) JOIN MTGSet USING (set_id) JOIN CardFace USING (printing_id) JOIN FaceName USING (face_name_id) JOIN PrintLanguage USING (language_id) ; COMMIT; Index: mtg_proxy_printer/model/carddb_migrations.py ================================================================== --- mtg_proxy_printer/model/carddb_migrations.py +++ mtg_proxy_printer/model/carddb_migrations.py @@ -564,10 +564,52 @@ JOIN FaceName USING (face_name_id) JOIN PrintLanguage USING (language_id)"""), ]: db.execute(statement) + +def _migrate_28_to_29(db: sqlite3.Connection): + db.execute("DROP VIEW HiddenPrintings\n") + db.execute(textwrap.dedent("""\ + CREATE TABLE PrintingDisplayFilter2 ( + -- Stores which filter applies to which printing. + printing_id INTEGER NOT NULL REFERENCES Printing (printing_id) ON DELETE CASCADE, + filter_id INTEGER NOT NULL REFERENCES DisplayFilters (filter_id) ON DELETE CASCADE, + PRIMARY KEY (printing_id, filter_id) + ) WITHOUT ROWID; + """)) + db.execute(textwrap.dedent("""\ + INSERT INTO PrintingDisplayFilter2 (printing_id, filter_id) + SELECT printing_id, filter_id + FROM PrintingDisplayFilter + WHERE filter_applies IS TRUE + """)) + db.execute("DROP TABLE PrintingDisplayFilter\n") + db.execute("ALTER TABLE PrintingDisplayFilter2 RENAME TO PrintingDisplayFilter\n") + db.execute(textwrap.dedent("""\ + CREATE VIEW HiddenPrintingIDs AS + SELECT printing_id + FROM PrintingDisplayFilter + JOIN DisplayFilters USING (filter_id) + WHERE filter_active IS TRUE + GROUP BY printing_id + ; + """)) + db.execute("DROP VIEW AllPrintings\n") + db.execute(textwrap.dedent("""\ + CREATE VIEW AllPrintings AS + SELECT card_name, set_code, set_name, "language", collector_number, scryfall_id, highres_image, face_number, + is_front, is_oversized, png_image_uri, oracle_id, release_date, wackiness_score, Printing.is_hidden + FROM Card + JOIN Printing USING (card_id) + JOIN MTGSet USING (set_id) + JOIN CardFace USING (printing_id) + JOIN FaceName USING (face_name_id) + JOIN PrintLanguage USING (language_id) + ; + """)) + MIGRATION_SCRIPTS: MigrationScriptListing = ( # First component of each tuple contains the source schema version, second contains the migration script function. # These MUST be ordered by source schema version, otherwise the migration logic breaks. In other words: APPEND only. (9, _migrate_9_to_10), @@ -587,10 +629,11 @@ (23, _migrate_23_to_24), (24, _migrate_24_to_25), (25, _migrate_25_to_26), (26, _migrate_26_to_27), (27, _migrate_27_to_28), + (28, _migrate_28_to_29), ) def migrate_card_database_location(): from mtg_proxy_printer.model.carddb import DEFAULT_DATABASE_LOCATION, OLD_DATABASE_LOCATION @@ -614,23 +657,24 @@ :param db: card database, given as a plain sqlite3 database connection object :param migration_scripts: List of migration script functions to run, if applicable. Defaults to a built-in list of migration scripts. Should only be passed explicitly for testing purposes. """ - current_schema_version = db.execute("PRAGMA user_version").fetchone()[0] + begin_schema_version = db.execute("PRAGMA user_version\n").fetchone()[0] if mtg_proxy_printer.sqlite_helpers.check_database_schema_version(db, "carddb") > 0: - logger.info(f"Database schema outdated, running database migrations. {current_schema_version=}") + logger.info(f"Database schema outdated, running database migrations. {begin_schema_version=}") if migration_scripts is not MIGRATION_SCRIPTS: logger.debug(f"Custom migration scripts passed: {migration_scripts}") else: logger.info("Database schema recent, not running any database migrations") return for source_version, migration_script in migration_scripts: - if db.execute("PRAGMA user_version").fetchone()[0] == source_version: + if db.execute("PRAGMA user_version\n").fetchone()[0] == source_version: logger.info(f"Running migration task for schema version {source_version}") - db.execute("BEGIN TRANSACTION") + db.execute("BEGIN TRANSACTION\n") migration_script(db) - db.execute(f"PRAGMA user_version = {source_version + 1}") + db.execute(f"PRAGMA user_version = {source_version + 1}\n") db.commit() - - current_schema_version = db.execute("PRAGMA user_version").fetchone()[0] - logger.info(f"Finished database migrations. {current_schema_version=}") + current_schema_version = db.execute("PRAGMA user_version\n").fetchone()[0] + logger.info(f"Finished database migrations, rebuilding database. {current_schema_version=}") + db.execute("VACUUM\n") + logger.info("Rebuild done.") Index: tests/helpers.py ================================================================== --- tests/helpers.py +++ tests/helpers.py @@ -61,10 +61,18 @@ @functools.lru_cache() def load_json(name: str) -> mtg_proxy_printer.card_info_downloader.JSONType: return json.loads(pkg_resources.resource_string("tests.json_samples", f"{name}.json").decode("utf-8")) + +def load_multiple_json_cards( + json_files_or_names: typing.List[typing.Union[str, mtg_proxy_printer.card_info_downloader.JSONType]]): + return [ + load_json(json_file_or_name) if isinstance(json_file_or_name, str) else json_file_or_name + for json_file_or_name in json_files_or_names + ] + def fill_card_database_with_json_cards( qtbot: QtBot, card_db: mtg_proxy_printer.model.carddb.CardDatabase, json_files_or_names: typing.List[typing.Union[str, mtg_proxy_printer.card_info_downloader.JSONType]], @@ -71,14 +79,11 @@ filter_settings: typing.Dict[str, str] = None) -> mtg_proxy_printer.model.carddb.CardDatabase: section = mtg_proxy_printer.settings.settings["card-filter"] settings_to_use = {filter_name: "False" for filter_name in section.keys()} if filter_settings: settings_to_use.update(filter_settings) - data = [ - load_json(json_file_or_name) if isinstance(json_file_or_name, str) else json_file_or_name - for json_file_or_name in json_files_or_names - ] + data = load_multiple_json_cards(json_files_or_names) with patch.dict(section, settings_to_use): populate_database(qtbot, card_db, data) return card_db Index: tests/test_card_info_downloader.py ================================================================== --- tests/test_card_info_downloader.py +++ tests/test_card_info_downloader.py @@ -12,10 +12,11 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . import dataclasses +import sqlite3 import typing import unittest.mock from hamcrest import * import pytest @@ -185,11 +186,11 @@ data := card_db.db.execute(f"SELECT png_image_uri, is_front, face_number FROM {relation_name}").fetchall(), contains_inanyorder(*test_case.db_card_face()), f"CardFace relation contains unexpected data: {data}") -def _assert_all_printings_contains(card_db: CardDatabase, test_case: TestCaseData): +def _assert_visible_printings_contains(card_db: CardDatabase, test_case: TestCaseData): """ Checks card_name, set_code, "language", collector_number, scryfall_id, highres_image, png_image_uri, is_front, is_oversized """ @@ -209,11 +210,11 @@ _assert_card_face_contains(card_db, test_case) _assert_face_name_contains(card_db, test_case) _assert_set_contains(card_db, test_case) _assert_card_contains(card_db, test_case) _assert_print_language_contains(card_db, test_case) - _assert_all_printings_contains(card_db, test_case) + _assert_visible_printings_contains(card_db, test_case) def assert_hidden_import(card_db: CardDatabase, test_case: TestCaseData): """ Verifies that the printing is correctly stored, but invisible in all VIEWs that filter out unwanted printings. @@ -443,19 +444,53 @@ filter_name = "hide-oversized-cards" # Pass 1: Populate the database and exclude the card. The card should not be visible fill_card_database_with_json_card(qtbot, card_db, test_case.json_name, {filter_name: "True"}) assert_hidden_import(card_db, test_case) # Pass 2: Re-Populate the database, but include the card now. - fill_card_database_with_json_card(qtbot, card_db, test_case.json_name, {filter_name: "False"}) + fill_card_database_with_json_card(qtbot, card_db, test_case.json_name) # The card should be in the database. The RemovedPrintings table should be empty assert_visible_import(card_db, test_case) assert_that( card_db.db.execute("SELECT scryfall_id, oracle_id FROM RemovedPrintings").fetchall(), is_(empty()), "RemovedPrintings table not properly cleaned up." ) + +@pytest.mark.parametrize("test_case_data", [ + TestCaseData( # English "Fury Sliver" from Time Spiral + "regular_english_card", True, ( + FaceData("Fury Sliver", "https://c1.scryfall.com/file/scryfall-cards/png/front/0/0/0000579f-7b35-4ed3-b44c-db2a538066fe.png?1562894979", True), + ), DatabaseSetData("tsp", "Time Spiral", "https://scryfall.com/sets/tsp?utm_source=api", "2006-10-06"), + "en", "157", "0000579f-7b35-4ed3-b44c-db2a538066fe", "44623693-51d6-49ad-8cd7-140505caf02f", False, + ), +]) +def test_re_import_after_card_ban_hides_it(qtbot, card_db: CardDatabase, test_case_data: TestCaseData): + card_json = load_json(test_case_data.json_name) + with unittest.mock.patch.dict(card_json["legalities"], {"commander": "banned"}): + fill_card_database_with_json_card(qtbot, card_db, card_json, {"hide-banned-in-commander": "True"}) + assert_hidden_import(card_db, test_case_data) + fill_card_database_with_json_card(qtbot, card_db, card_json, {"hide-banned-in-commander": "True"}) + assert_visible_import(card_db, test_case_data) + + +@pytest.mark.parametrize("test_case_data", [ + TestCaseData( # English "Fury Sliver" from Time Spiral + "regular_english_card", True, ( + FaceData("Fury Sliver", "https://c1.scryfall.com/file/scryfall-cards/png/front/0/0/0000579f-7b35-4ed3-b44c-db2a538066fe.png?1562894979", True), + ), DatabaseSetData("tsp", "Time Spiral", "https://scryfall.com/sets/tsp?utm_source=api", "2006-10-06"), + "en", "157", "0000579f-7b35-4ed3-b44c-db2a538066fe", "44623693-51d6-49ad-8cd7-140505caf02f", False, + ), +]) +def test_re_import_after_unban_makes_card_visible(qtbot, card_db: CardDatabase, test_case_data: TestCaseData): + card_json = load_json(test_case_data.json_name) + fill_card_database_with_json_card(qtbot, card_db, card_json, {"hide-banned-in-commander": "True"}) + assert_visible_import(card_db, test_case_data) + with unittest.mock.patch.dict(card_json["legalities"], {"commander": "banned"}): + fill_card_database_with_json_card(qtbot, card_db, card_json, {"hide-banned-in-commander": "True"}) + assert_hidden_import(card_db, test_case_data) + def test_updates_language(qtbot, card_db: CardDatabase): test_case = TestCaseData( # English "Fury Sliver" from Time Spiral. Modified the language "regular_english_card", True, ( FaceData("Fury Sliver",