From 41beef009fb4e3de4d269cb113e93f06e9bc5ed7 Mon Sep 17 00:00:00 2001 From: Forest Gregg Date: Tue, 24 Jan 2023 16:30:51 -0500 Subject: [PATCH 1/4] spike to fix half of #1099 --- datasette/utils/__init__.py | 37 ++++++++++++++----------------------- datasette/views/row.py | 33 +++++++++++++++++++-------------- datasette/views/table.py | 9 +++++++-- 3 files changed, 40 insertions(+), 39 deletions(-) diff --git a/datasette/utils/__init__.py b/datasette/utils/__init__.py index 841d6c6c78..8f016ec2c5 100644 --- a/datasette/utils/__init__.py +++ b/datasette/utils/__init__.py @@ -523,30 +523,21 @@ def detect_primary_keys(conn, table): def get_outbound_foreign_keys(conn, table): infos = conn.execute(f"PRAGMA foreign_key_list([{table}])").fetchall() - fks = [] + fks = {} for info in infos: if info is not None: id, seq, table_name, from_, to_, on_update, on_delete, match = info - fks.append( - { - "column": from_, + if id in fks: + fk_info = fks[id] + fk_info["columns"] += (from_,) + fk_info["other_columns"] += (to_,) + else: + fks[id] = { "other_table": table_name, - "other_column": to_, - "id": id, - "seq": seq, + "columns": (from_,), + "other_columns": (to_,), } - ) - # Filter out compound foreign keys by removing any where "id" is not unique - id_counts = Counter(fk["id"] for fk in fks) - return [ - { - "column": fk["column"], - "other_table": fk["other_table"], - "other_column": fk["other_column"], - } - for fk in fks - if id_counts[fk["id"]] == 1 - ] + return list(fks.values()) def get_all_foreign_keys(conn): @@ -560,17 +551,17 @@ def get_all_foreign_keys(conn): fks = get_outbound_foreign_keys(conn, table) for fk in fks: table_name = fk["other_table"] - from_ = fk["column"] - to_ = fk["other_column"] + from_ = fk["columns"] + to_ = fk["other_columns"] if table_name not in table_to_foreign_keys: # Weird edge case where something refers to a table that does # not actually exist continue table_to_foreign_keys[table_name]["incoming"].append( - {"other_table": table, "column": to_, "other_column": from_} + {"other_table": table, "columns": to_, "other_columns": from_} ) table_to_foreign_keys[table]["outgoing"].append( - {"other_table": table_name, "column": from_, "other_column": to_} + {"other_table": table_name, "columns": from_, "other_columns": to_} ) return table_to_foreign_keys diff --git a/datasette/views/row.py b/datasette/views/row.py index b3c4574632..45af159f59 100644 --- a/datasette/views/row.py +++ b/datasette/views/row.py @@ -97,8 +97,6 @@ async def template_data(): ) async def foreign_key_tables(self, database, table, pk_values): - if len(pk_values) != 1: - return [] db = self.ds.databases[database] all_foreign_keys = await db.get_all_foreign_keys() foreign_keys = all_foreign_keys[table]["incoming"] @@ -107,37 +105,44 @@ async def foreign_key_tables(self, database, table, pk_values): sql = "select " + ", ".join( [ - "(select count(*) from {table} where {column}=:id)".format( + "(select count(*) from {table} where {condition})".format( table=escape_sqlite(fk["other_table"]), - column=escape_sqlite(fk["other_column"]), + condition=" and ".join( + "{column}=:id{i}".format(column=column, i=i) + for i, column in enumerate(fk["other_columns"]) + ), ) for fk in foreign_keys ] ) try: - rows = list(await db.execute(sql, {"id": pk_values[0]})) + rows = list( + await db.execute( + sql, {"id{i}".format(i=i): pk for i, pk in enumerate(pk_values)} + ) + ) except QueryInterrupted: # Almost certainly hit the timeout return [] foreign_table_counts = dict( zip( - [(fk["other_table"], fk["other_column"]) for fk in foreign_keys], + [(fk["other_table"], fk["other_columns"]) for fk in foreign_keys], list(rows[0]), ) ) foreign_key_tables = [] for fk in foreign_keys: count = ( - foreign_table_counts.get((fk["other_table"], fk["other_column"])) or 0 + foreign_table_counts.get((fk["other_table"], fk["other_columns"])) or 0 + ) + query_pairs = zip(fk["other_columns"], pk_values) + query = "&".join( + "{}={}".format(col + "__exact" if col.startswith("_") else col, pk) + for col, pk in query_pairs ) - key = fk["other_column"] - if key.startswith("_"): - key += "__exact" - link = "{}?{}={}".format( - self.ds.urls.table(database, fk["other_table"]), - key, - ",".join(pk_values), + link = "{}?{}".format( + self.ds.urls.table(database, fk["other_table"]), query ) foreign_key_tables.append({**fk, **{"count": count, "link": link}}) return foreign_key_tables diff --git a/datasette/views/table.py b/datasette/views/table.py index ad45ecd365..4c91e4c90f 100644 --- a/datasette/views/table.py +++ b/datasette/views/table.py @@ -644,7 +644,11 @@ async def execute_suggested_facets(): columns_to_expand = request.args.getlist("_label") if columns_to_expand is None and all_labels: # expand all columns with foreign keys - columns_to_expand = [fk["column"] for fk, _ in expandable_columns] + columns_to_expand = [ + fk["columns"][0] + for fk, _ in expandable_columns + if len(fk["columns"]) == 1 + ] if columns_to_expand: expanded_labels = {} @@ -920,8 +924,9 @@ async def display_columns_and_rows( ) column_to_foreign_key_table = { - fk["column"]: fk["other_table"] + fk["columns"][0]: fk["other_table"] for fk in await db.foreign_keys_for_table(table_name) + if len(fk["columns"]) == 1 } cell_rows = [] From 1e842effa7a4f6ad68a4fc147c9b99c6290842d5 Mon Sep 17 00:00:00 2001 From: Forest Gregg Date: Tue, 24 Jan 2023 16:57:41 -0500 Subject: [PATCH 2/4] more narrowing to singletons --- datasette/app.py | 4 +++- datasette/templates/row.html | 2 +- datasette/views/row.py | 15 ++++++++++++++- datasette/views/table.py | 15 +++++++++------ 4 files changed, 27 insertions(+), 9 deletions(-) diff --git a/datasette/app.py b/datasette/app.py index df33386bbb..9aea192a32 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -899,8 +899,10 @@ async def expand_foreign_keys(self, database, table, column, values): fk = [ foreign_key for foreign_key in foreign_keys - if foreign_key["column"] == column + if foreign_key["columns"][0] == column and len(foreign_keys) == 1 ][0] + fk["column"] = fk["columns"][0] + fk["other_column"] = fk["other_columns"][0] except IndexError: return {} label_column = await db.label_column_for_table(fk["other_table"]) diff --git a/datasette/templates/row.html b/datasette/templates/row.html index 1d1b0bfd4f..ef2eff8155 100644 --- a/datasette/templates/row.html +++ b/datasette/templates/row.html @@ -35,7 +35,7 @@

Links from other tables

  • {{ "{:,}".format(other.count) }} row{% if other.count == 1 %}{% else %}s{% endif %} - from {{ other.other_column }} in {{ other.other_table }} + from {{ other.other_columns_reference }} in {{ other.other_table }}
    • {% endfor %} diff --git a/datasette/views/row.py b/datasette/views/row.py index 45af159f59..cb76d9f431 100644 --- a/datasette/views/row.py +++ b/datasette/views/row.py @@ -144,7 +144,20 @@ async def foreign_key_tables(self, database, table, pk_values): link = "{}?{}".format( self.ds.urls.table(database, fk["other_table"]), query ) - foreign_key_tables.append({**fk, **{"count": count, "link": link}}) + if len(pk_values) == 1: + other_columns_reference = fk["other_columns"][0] + else: + other_columns_reference = "({})".format(", ".join(fk["other_columns"])) + foreign_key_tables.append( + { + **fk, + **{ + "count": count, + "link": link, + "other_columns_reference": other_columns_reference, + }, + } + ) return foreign_key_tables diff --git a/datasette/views/table.py b/datasette/views/table.py index 4c91e4c90f..7483e5bd93 100644 --- a/datasette/views/table.py +++ b/datasette/views/table.py @@ -88,8 +88,15 @@ async def expandable_columns(self, database_name, table_name): expandables = [] db = self.ds.databases[database_name] for fk in await db.foreign_keys_for_table(table_name): + if len(fk["other_columns"]) > 1: + continue label_column = await db.label_column_for_table(fk["other_table"]) - expandables.append((fk, label_column)) + singleton_fk = { + "other_table": fk["other_table"], + "other_column": fk["other_columns"][0], + "column": fk["columns"][0], + } + expandables.append((singleton_fk, label_column)) return expandables async def post(self, request): @@ -644,11 +651,7 @@ async def execute_suggested_facets(): columns_to_expand = request.args.getlist("_label") if columns_to_expand is None and all_labels: # expand all columns with foreign keys - columns_to_expand = [ - fk["columns"][0] - for fk, _ in expandable_columns - if len(fk["columns"]) == 1 - ] + columns_to_expand = [fk["column"] for fk, _ in expandable_columns] if columns_to_expand: expanded_labels = {} From 36fa32dba12757250f7beb1eaad8c618f5ed3a4a Mon Sep 17 00:00:00 2001 From: Forest Gregg Date: Tue, 24 Jan 2023 19:06:20 -0500 Subject: [PATCH 3/4] getting tests working --- datasette/app.py | 3 ++- datasette/filters.py | 6 +++++- datasette/views/row.py | 2 +- 3 files changed, 8 insertions(+), 3 deletions(-) diff --git a/datasette/app.py b/datasette/app.py index 9aea192a32..0cd929e852 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -899,7 +899,8 @@ async def expand_foreign_keys(self, database, table, column, values): fk = [ foreign_key for foreign_key in foreign_keys - if foreign_key["columns"][0] == column and len(foreign_keys) == 1 + if foreign_key["columns"][0] == column + and len(foreign_key["columns"]) == 1 ][0] fk["column"] = fk["columns"][0] fk["other_column"] = fk["other_columns"][0] diff --git a/datasette/filters.py b/datasette/filters.py index 5ea3488bd7..58399e0985 100644 --- a/datasette/filters.py +++ b/datasette/filters.py @@ -135,8 +135,12 @@ async def inner(): outgoing_foreign_keys = await db.foreign_keys_for_table(through_table) try: fk_to_us = [ - fk for fk in outgoing_foreign_keys if fk["other_table"] == table + fk + for fk in outgoing_foreign_keys + if fk["other_table"] == table and len(fk["other_columns"]) == 1 ][0] + fk_to_us["column"] = fk_to_us["columns"][0] + fk_to_us["other_column"] = fk_to_us["other_columns"][0] except IndexError: raise DatasetteError( "Invalid _through - could not find corresponding foreign key" diff --git a/datasette/views/row.py b/datasette/views/row.py index cb76d9f431..0e4adfe273 100644 --- a/datasette/views/row.py +++ b/datasette/views/row.py @@ -108,7 +108,7 @@ async def foreign_key_tables(self, database, table, pk_values): "(select count(*) from {table} where {condition})".format( table=escape_sqlite(fk["other_table"]), condition=" and ".join( - "{column}=:id{i}".format(column=column, i=i) + "{column}=:id{i}".format(column=escape_sqlite(column), i=i) for i, column in enumerate(fk["other_columns"]) ), ) From 1e5b42f9d6490926300953837cbaa571ef81d772 Mon Sep 17 00:00:00 2001 From: Forest Gregg Date: Tue, 24 Jan 2023 19:35:45 -0500 Subject: [PATCH 4/4] adjust tests --- tests/test_api.py | 151 ++++++++++++++++++------------- tests/test_internals_database.py | 41 ++++++--- 2 files changed, 115 insertions(+), 77 deletions(-) diff --git a/tests/test_api.py b/tests/test_api.py index 5a75148701..b48b2a8e08 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -96,8 +96,8 @@ async def test_database_page(ds_client): "incoming": [ { "other_table": "roadside_attraction_characteristics", - "column": "pk", - "other_column": "characteristic_id", + "columns": ["pk"], + "other_columns": ["characteristic_id"], } ], "outgoing": [], @@ -126,18 +126,18 @@ async def test_database_page(ds_client): "outgoing": [ { "other_table": "simple_primary_key", - "column": "f3", - "other_column": "id", + "columns": ["f3"], + "other_columns": ["id"], }, { "other_table": "simple_primary_key", - "column": "f2", - "other_column": "id", + "columns": ["f2"], + "other_columns": ["id"], }, { "other_table": "simple_primary_key", - "column": "f1", - "other_column": "id", + "columns": ["f1"], + "other_columns": ["id"], }, ], }, @@ -150,7 +150,19 @@ async def test_database_page(ds_client): "count": 2, "hidden": False, "fts_table": None, - "foreign_keys": {"incoming": [], "outgoing": []}, + "foreign_keys": { + "incoming": [ + { + "other_table": "foreign_key_references", + "columns": ["pk1", "pk2"], + "other_columns": [ + "foreign_key_compound_pk1", + "foreign_key_compound_pk2", + ], + } + ], + "outgoing": [], + }, "private": False, }, { @@ -175,8 +187,8 @@ async def test_database_page(ds_client): "outgoing": [ { "other_table": "primary_key_multiple_columns_explicit_label", - "column": "foreign_key_with_custom_label", - "other_column": "id", + "columns": ["foreign_key_with_custom_label"], + "other_columns": ["id"], } ], }, @@ -193,8 +205,8 @@ async def test_database_page(ds_client): "incoming": [ { "other_table": "facetable", - "column": "id", - "other_column": "_city_id", + "columns": ["id"], + "other_columns": ["_city_id"], } ], "outgoing": [], @@ -225,8 +237,8 @@ async def test_database_page(ds_client): "outgoing": [ { "other_table": "facet_cities", - "column": "_city_id", - "other_column": "id", + "columns": ["_city_id"], + "other_columns": ["id"], } ], }, @@ -249,20 +261,28 @@ async def test_database_page(ds_client): "foreign_keys": { "incoming": [], "outgoing": [ + { + "other_table": "compound_primary_key", + "columns": [ + "foreign_key_compound_pk1", + "foreign_key_compound_pk2", + ], + "other_columns": ["pk1", "pk2"], + }, { "other_table": "primary_key_multiple_columns", - "column": "foreign_key_with_no_label", - "other_column": "id", + "columns": ["foreign_key_with_no_label"], + "other_columns": ["id"], }, { "other_table": "simple_primary_key", - "column": "foreign_key_with_blank_label", - "other_column": "id", + "columns": ["foreign_key_with_blank_label"], + "other_columns": ["id"], }, { "other_table": "simple_primary_key", - "column": "foreign_key_with_label", - "other_column": "id", + "columns": ["foreign_key_with_label"], + "other_columns": ["id"], }, ], }, @@ -290,8 +310,8 @@ async def test_database_page(ds_client): "incoming": [ { "other_table": "foreign_key_references", - "column": "id", - "other_column": "foreign_key_with_no_label", + "columns": ["id"], + "other_columns": ["foreign_key_with_no_label"], } ], "outgoing": [], @@ -309,8 +329,8 @@ async def test_database_page(ds_client): "incoming": [ { "other_table": "custom_foreign_key_label", - "column": "id", - "other_column": "foreign_key_with_custom_label", + "columns": ["id"], + "other_columns": ["foreign_key_with_custom_label"], } ], "outgoing": [], @@ -329,13 +349,13 @@ async def test_database_page(ds_client): "outgoing": [ { "other_table": "attraction_characteristic", - "column": "characteristic_id", - "other_column": "pk", + "columns": ["characteristic_id"], + "other_columns": ["pk"], }, { "other_table": "roadside_attractions", - "column": "attraction_id", - "other_column": "pk", + "columns": ["attraction_id"], + "other_columns": ["pk"], }, ], }, @@ -352,8 +372,8 @@ async def test_database_page(ds_client): "incoming": [ { "other_table": "roadside_attraction_characteristics", - "column": "pk", - "other_column": "attraction_id", + "columns": ["pk"], + "other_columns": ["attraction_id"], } ], "outgoing": [], @@ -371,8 +391,8 @@ async def test_database_page(ds_client): "incoming": [ { "other_table": "searchable_tags", - "column": "pk", - "other_column": "searchable_id", + "columns": ["pk"], + "other_columns": ["searchable_id"], } ], "outgoing": [], @@ -389,11 +409,15 @@ async def test_database_page(ds_client): "foreign_keys": { "incoming": [], "outgoing": [ - {"other_table": "tags", "column": "tag", "other_column": "tag"}, + { + "other_table": "tags", + "columns": ["tag"], + "other_columns": ["tag"], + }, { "other_table": "searchable", - "column": "searchable_id", - "other_column": "pk", + "columns": ["searchable_id"], + "other_columns": ["pk"], }, ], }, @@ -420,28 +444,28 @@ async def test_database_page(ds_client): "incoming": [ { "other_table": "foreign_key_references", - "column": "id", - "other_column": "foreign_key_with_blank_label", + "columns": ["id"], + "other_columns": ["foreign_key_with_blank_label"], }, { "other_table": "foreign_key_references", - "column": "id", - "other_column": "foreign_key_with_label", + "columns": ["id"], + "other_columns": ["foreign_key_with_label"], }, { "other_table": "complex_foreign_keys", - "column": "id", - "other_column": "f3", + "columns": ["id"], + "other_columns": ["f3"], }, { "other_table": "complex_foreign_keys", - "column": "id", - "other_column": "f2", + "columns": ["id"], + "other_columns": ["f2"], }, { "other_table": "complex_foreign_keys", - "column": "id", - "other_column": "f1", + "columns": ["id"], + "other_columns": ["f1"], }, ], "outgoing": [], @@ -487,8 +511,8 @@ async def test_database_page(ds_client): "incoming": [ { "other_table": "searchable_tags", - "column": "tag", - "other_column": "tag", + "columns": ["tag"], + "other_columns": ["tag"], } ], "outgoing": [], @@ -517,11 +541,7 @@ async def test_database_page(ds_client): }, { "name": "searchable_fts", - "columns": [ - "text1", - "text2", - "name with . and spaces", - ] + "columns": ["text1", "text2", "name with . and spaces"] + ( [ "searchable_fts", @@ -720,38 +740,43 @@ async def test_row_foreign_key_tables(ds_client): assert response.json()["foreign_key_tables"] == [ { "other_table": "foreign_key_references", - "column": "id", - "other_column": "foreign_key_with_blank_label", + "columns": ["id"], + "other_columns": ["foreign_key_with_blank_label"], "count": 0, "link": "/fixtures/foreign_key_references?foreign_key_with_blank_label=1", + "other_columns_reference": "foreign_key_with_blank_label", }, { "other_table": "foreign_key_references", - "column": "id", - "other_column": "foreign_key_with_label", + "columns": ["id"], + "other_columns": ["foreign_key_with_label"], "count": 1, "link": "/fixtures/foreign_key_references?foreign_key_with_label=1", + "other_columns_reference": "foreign_key_with_label", }, { "other_table": "complex_foreign_keys", - "column": "id", - "other_column": "f3", + "columns": ["id"], + "other_columns": ["f3"], "count": 1, "link": "/fixtures/complex_foreign_keys?f3=1", + "other_columns_reference": "f3", }, { "other_table": "complex_foreign_keys", - "column": "id", - "other_column": "f2", + "columns": ["id"], + "other_columns": ["f2"], "count": 0, "link": "/fixtures/complex_foreign_keys?f2=1", + "other_columns_reference": "f2", }, { "other_table": "complex_foreign_keys", - "column": "id", - "other_column": "f1", + "columns": ["id"], + "other_columns": ["f1"], "count": 1, "link": "/fixtures/complex_foreign_keys?f1=1", + "other_columns_reference": "f1", }, ] diff --git a/tests/test_internals_database.py b/tests/test_internals_database.py index 647ae7bdc7..10d5ebd2c2 100644 --- a/tests/test_internals_database.py +++ b/tests/test_internals_database.py @@ -329,13 +329,13 @@ async def test_get_all_foreign_keys(db): "outgoing": [ { "other_table": "attraction_characteristic", - "column": "characteristic_id", - "other_column": "pk", + "columns": ("characteristic_id",), + "other_columns": ("pk",), }, { "other_table": "roadside_attractions", - "column": "attraction_id", - "other_column": "pk", + "columns": ("attraction_id",), + "other_columns": ("pk",), }, ], } @@ -343,34 +343,47 @@ async def test_get_all_foreign_keys(db): "incoming": [ { "other_table": "roadside_attraction_characteristics", - "column": "pk", - "other_column": "characteristic_id", + "columns": ("pk",), + "other_columns": ("characteristic_id",), } ], "outgoing": [], } assert all_foreign_keys["compound_primary_key"] == { - # No incoming because these are compound foreign keys, which we currently ignore - "incoming": [], + "incoming": [ + { + "other_table": "foreign_key_references", + "columns": ("pk1", "pk2"), + "other_columns": ( + "foreign_key_compound_pk1", + "foreign_key_compound_pk2", + ), + } + ], "outgoing": [], } assert all_foreign_keys["foreign_key_references"] == { "incoming": [], "outgoing": [ + { + "other_table": "compound_primary_key", + "columns": ("foreign_key_compound_pk1", "foreign_key_compound_pk2"), + "other_columns": ("pk1", "pk2"), + }, { "other_table": "primary_key_multiple_columns", - "column": "foreign_key_with_no_label", - "other_column": "id", + "columns": ("foreign_key_with_no_label",), + "other_columns": ("id",), }, { "other_table": "simple_primary_key", - "column": "foreign_key_with_blank_label", - "other_column": "id", + "columns": ("foreign_key_with_blank_label",), + "other_columns": ("id",), }, { "other_table": "simple_primary_key", - "column": "foreign_key_with_label", - "other_column": "id", + "columns": ("foreign_key_with_label",), + "other_columns": ("id",), }, ], }