From 6d12580ed74ff0baf081bf9fabc14eeaf8a3105c Mon Sep 17 00:00:00 2001
From: Simon Willison <swillison@gmail.com>
Date: Tue, 15 May 2018 10:52:02 -0500
Subject: [PATCH] Foreign key facets are now expanded to labels, refs #255

---
 datasette/templates/table.html |   2 +-
 datasette/views/table.py       |  58 ++++++++++++++++++-
 tests/fixtures.py              |  50 +++++++++-------
 tests/test_api.py              | 101 ++++++++++++++++++++++-----------
 tests/test_html.py             |   6 +-
 tests/test_utils.py            |   4 +-
 6 files changed, 162 insertions(+), 59 deletions(-)

diff --git a/datasette/templates/table.html b/datasette/templates/table.html
index 7216a7b7..3abd5b10 100644
--- a/datasette/templates/table.html
+++ b/datasette/templates/table.html
@@ -102,7 +102,7 @@
     <p><strong><a href="{{ path_with_removed_args(request, {'_facet': facet_name}) }}">{{ facet_name }}</a></strong></p>
     <ul>
         {% for facet_value in facet_info.results %}
-            <li><a href="{{ facet_value.toggle_url }}">{{ facet_value.value }}</a> {{ "{:,}".format(facet_value.count) }}</li>
+            <li><a href="{{ facet_value.toggle_url }}">{{ facet_value.label }}</a> {{ "{:,}".format(facet_value.count) }}</li>
         {% endfor %}
         {% if facet_info.truncated %}
             <li>...</li>
diff --git a/datasette/views/table.py b/datasette/views/table.py
index 38185553..3eb41ade 100644
--- a/datasette/views/table.py
+++ b/datasette/views/table.py
@@ -35,6 +35,54 @@ class RowTableShared(BaseView):
             sortable_columns.add("rowid")
         return sortable_columns
 
+    async def expand_foreign_keys(self, database, table, column, values):
+        "Returns dict mapping (column, value) -> label"
+        labeled_fks = {}
+        tables_info = self.ds.inspect()[database]["tables"]
+        table_info = tables_info.get(table) or {}
+        if not table_info:
+            return {}
+        foreign_keys = table_info["foreign_keys"]["outgoing"]
+        # Find the foreign_key for this column
+        try:
+            fk = [
+                foreign_key for foreign_key in foreign_keys
+                if foreign_key["column"] == column
+            ][0]
+        except IndexError:
+            return {}
+        label_column = (
+            # First look in metadata.json for this foreign key table:
+            self.table_metadata(
+                database, fk["other_table"]
+            ).get("label_column")
+            or tables_info.get(fk["other_table"], {}).get("label_column")
+        )
+        if not label_column:
+            return {}
+        labeled_fks = {}
+        sql = '''
+            select {other_column}, {label_column}
+            from {other_table}
+            where {other_column} in ({placeholders})
+        '''.format(
+            other_column=escape_sqlite(fk["other_column"]),
+            label_column=escape_sqlite(label_column),
+            other_table=escape_sqlite(fk["other_table"]),
+            placeholders=", ".join(["?"] * len(set(values))),
+        )
+        try:
+            results = await self.execute(
+                database, sql, list(set(values))
+            )
+        except sqlite3.OperationalError:
+            # Probably hit the timelimit
+            pass
+        else:
+            for id, value in results:
+                labeled_fks[(fk["column"], id)] = value
+        return labeled_fks
+
     async def display_columns_and_rows(
         self,
         database,
@@ -514,7 +562,11 @@ class TableView(RowTableShared):
                     "results": [],
                     "truncated": len(facet_rows) > FACET_SIZE,
                 }
-                for row in facet_rows[:FACET_SIZE]:
+                facet_rows = facet_rows[:FACET_SIZE]
+                # Attempt to expand foreign keys into labels
+                values = [row["value"] for row in facet_rows]
+                expanded = (await self.expand_foreign_keys(name, table, column, values))
+                for row in facet_rows:
                     selected = str(other_args.get(column)) == str(row["value"])
                     if selected:
                         toggle_path = path_with_removed_args(
@@ -526,6 +578,10 @@ class TableView(RowTableShared):
                         )
                     facet_results[column]["results"].append({
                         "value": row["value"],
+                        "label": expanded.get(
+                            (column, row["value"]),
+                            row["value"]
+                        ),
                         "count": row["count"],
                         "toggle_url": urllib.parse.urljoin(
                             request.url, toggle_path
diff --git a/tests/fixtures.py b/tests/fixtures.py
index 99d686e4..5def4292 100644
--- a/tests/fixtures.py
+++ b/tests/fixtures.py
@@ -267,29 +267,41 @@ CREATE TABLE [select] (
 );
 INSERT INTO [select] VALUES ('group', 'having', 'and');
 
+CREATE TABLE facet_cities (
+    id integer primary key,
+    name text
+);
+INSERT INTO facet_cities (id, name) VALUES
+    (1, 'San Francisco'),
+    (2, 'Los Angeles'),
+    (3, 'Detroit'),
+    (4, 'Memnonia')
+;
+
 CREATE TABLE facetable (
     pk integer primary key,
-    planet_id integer,
+    planet_int integer,
     state text,
-    city text,
-    neighborhood text
+    city_id integer,
+    neighborhood text,
+    FOREIGN KEY ("city_id") REFERENCES [facet_cities](id)
 );
-INSERT INTO facetable (planet_id, state, city, neighborhood) VALUES
-    (1, 'CA', 'San Francisco', 'Mission'),
-    (1, 'CA', 'San Francisco', 'Dogpatch'),
-    (1, 'CA', 'San Francisco', 'SOMA'),
-    (1, 'CA', 'San Francisco', 'Tenderloin'),
-    (1, 'CA', 'San Francisco', 'Bernal Heights'),
-    (1, 'CA', 'San Francisco', 'Hayes Valley'),
-    (1, 'CA', 'Los Angeles', 'Hollywood'),
-    (1, 'CA', 'Los Angeles', 'Downtown'),
-    (1, 'CA', 'Los Angeles', 'Los Feliz'),
-    (1, 'CA', 'Los Angeles', 'Koreatown'),
-    (1, 'MI', 'Detroit', 'Downtown'),
-    (1, 'MI', 'Detroit', 'Greektown'),
-    (1, 'MI', 'Detroit', 'Corktown'),
-    (1, 'MI', 'Detroit', 'Mexicantown'),
-    (2, 'MC', 'Memnonia', 'Arcadia Planitia')
+INSERT INTO facetable (planet_int, state, city_id, neighborhood) VALUES
+    (1, 'CA', 1, 'Mission'),
+    (1, 'CA', 1, 'Dogpatch'),
+    (1, 'CA', 1, 'SOMA'),
+    (1, 'CA', 1, 'Tenderloin'),
+    (1, 'CA', 1, 'Bernal Heights'),
+    (1, 'CA', 1, 'Hayes Valley'),
+    (1, 'CA', 2, 'Hollywood'),
+    (1, 'CA', 2, 'Downtown'),
+    (1, 'CA', 2, 'Los Feliz'),
+    (1, 'CA', 2, 'Koreatown'),
+    (1, 'MI', 3, 'Downtown'),
+    (1, 'MI', 3, 'Greektown'),
+    (1, 'MI', 3, 'Corktown'),
+    (1, 'MI', 3, 'Mexicantown'),
+    (2, 'MC', 4, 'Arcadia Planitia')
 ;
 
 INSERT INTO simple_primary_key VALUES (1, 'hello');
diff --git a/tests/test_api.py b/tests/test_api.py
index a68c8a07..2079ef21 100644
--- a/tests/test_api.py
+++ b/tests/test_api.py
@@ -19,7 +19,7 @@ def test_homepage(app_client):
     assert response.json.keys() == {'test_tables': 0}.keys()
     d = response.json['test_tables']
     assert d['name'] == 'test_tables'
-    assert d['tables_count'] == 16
+    assert d['tables_count'] == 17
 
 
 def test_database_page(app_client):
@@ -103,10 +103,33 @@ def test_database_page(app_client):
         'fts_table': None,
         'primary_keys': ['pk'],
     }, {
-        'columns': ['pk', 'planet_id', 'state', 'city', 'neighborhood'],
+        'columns': ['id', 'name'],
+        'name': 'facet_cities',
+        'count': 4,
+        'foreign_keys': {
+            'incoming': [{
+                'column': 'id',
+                'other_column': 'city_id',
+                'other_table': 'facetable',
+            }],
+            'outgoing': []
+        },
+        'fts_table': None,
+        'hidden': False,
+        'label_column': 'name',
+        'primary_keys': ['id'],
+    }, {
+        'columns': ['pk', 'planet_int', 'state', 'city_id', 'neighborhood'],
         'name': 'facetable',
         'count': 15,
-        'foreign_keys': {'incoming': [], 'outgoing': []},
+        'foreign_keys': {
+            'incoming': [],
+            'outgoing': [{
+                'column': 'city_id',
+                'other_column': 'id',
+                'other_table': 'facet_cities'
+            }],
+        },
         'fts_table': None,
         'hidden': False,
         'label_column': None,
@@ -889,57 +912,64 @@ def test_page_size_matching_max_returned_rows(app_client_returend_rows_matches_p
 
 @pytest.mark.parametrize('path,expected_facet_results', [
     (
-        "/test_tables/facetable.json?_facet=state&_facet=city",
+        "/test_tables/facetable.json?_facet=state&_facet=city_id",
         {
             "state": {
                 "name": "state",
                 "results": [
                     {
                         "value": "CA",
+                        "label": "CA",
                         "count": 10,
-                        "toggle_url": "_facet=state&_facet=city&state=CA",
+                        "toggle_url": "_facet=state&_facet=city_id&state=CA",
                         "selected": False,
                     },
                     {
                         "value": "MI",
+                        "label": "MI",
                         "count": 4,
-                        "toggle_url": "_facet=state&_facet=city&state=MI",
+                        "toggle_url": "_facet=state&_facet=city_id&state=MI",
                         "selected": False,
                     },
                     {
                         "value": "MC",
+                        "label": "MC",
                         "count": 1,
-                        "toggle_url": "_facet=state&_facet=city&state=MC",
+                        "toggle_url": "_facet=state&_facet=city_id&state=MC",
                         "selected": False,
                     }
                 ],
                 "truncated": False,
             },
-            "city": {
-                "name": "city",
+            "city_id": {
+                "name": "city_id",
                 "results": [
                     {
-                        "value": "San Francisco",
+                        "value": 1,
+                        "label": "San Francisco",
                         "count": 6,
-                        "toggle_url": "_facet=state&_facet=city&city=San+Francisco",
+                        "toggle_url": "_facet=state&_facet=city_id&city_id=1",
                         "selected": False,
                     },
                     {
-                        "value": "Detroit",
+                        "value": 2,
+                        "label": "Los Angeles",
                         "count": 4,
-                        "toggle_url": "_facet=state&_facet=city&city=Detroit",
+                        "toggle_url": "_facet=state&_facet=city_id&city_id=2",
                         "selected": False,
                     },
                     {
-                        "value": "Los Angeles",
+                        "value": 3,
+                        "label": "Detroit",
                         "count": 4,
-                        "toggle_url": "_facet=state&_facet=city&city=Los+Angeles",
+                        "toggle_url": "_facet=state&_facet=city_id&city_id=3",
                         "selected": False,
                     },
                     {
-                        "value": "Memnonia",
+                        "value": 4,
+                        "label": "Memnonia",
                         "count": 1,
-                        "toggle_url": "_facet=state&_facet=city&city=Memnonia",
+                        "toggle_url": "_facet=state&_facet=city_id&city_id=4",
                         "selected": False,
                     }
                 ],
@@ -947,67 +977,72 @@ def test_page_size_matching_max_returned_rows(app_client_returend_rows_matches_p
             }
         }
     ), (
-        "/test_tables/facetable.json?_facet=state&_facet=city&state=MI",
+        "/test_tables/facetable.json?_facet=state&_facet=city_id&state=MI",
         {
             "state": {
                 "name": "state",
                 "results": [
                     {
                         "value": "MI",
+                        "label": "MI",
                         "count": 4,
                         "selected": True,
-                        "toggle_url": "_facet=state&_facet=city",
+                        "toggle_url": "_facet=state&_facet=city_id",
                     },
                 ],
                 "truncated": False,
             },
-            "city": {
-                "name": "city",
+            "city_id": {
+                "name": "city_id",
                 "results": [
                     {
-                        "value": "Detroit",
+                        "value": 3,
+                        "label": "Detroit",
                         "count": 4,
                         "selected": False,
-                        "toggle_url": "_facet=state&_facet=city&state=MI&city=Detroit",
+                        "toggle_url": "_facet=state&_facet=city_id&state=MI&city_id=3",
                     },
                 ],
                 "truncated": False,
             },
         },
     ), (
-        "/test_tables/facetable.json?_facet=planet_id",
+        "/test_tables/facetable.json?_facet=planet_int",
         {
-            "planet_id": {
-                "name": "planet_id",
+            "planet_int": {
+                "name": "planet_int",
                 "results": [
                     {
                         "value": 1,
+                        "label": 1,
                         "count": 14,
                         "selected": False,
-                        "toggle_url": "_facet=planet_id&planet_id=1",
+                        "toggle_url": "_facet=planet_int&planet_int=1",
                     },
                     {
                         "value": 2,
+                        "label": 2,
                         "count": 1,
                         "selected": False,
-                        "toggle_url": "_facet=planet_id&planet_id=2",
+                        "toggle_url": "_facet=planet_int&planet_int=2",
                     },
                 ],
                 "truncated": False,
             }
         },
     ), (
-        # planet_id is an integer field:
-        "/test_tables/facetable.json?_facet=planet_id&planet_id=1",
+        # planet_int is an integer field:
+        "/test_tables/facetable.json?_facet=planet_int&planet_int=1",
         {
-            "planet_id": {
-                "name": "planet_id",
+            "planet_int": {
+                "name": "planet_int",
                 "results": [
                     {
                         "value": 1,
+                        "label": 1,
                         "count": 14,
                         "selected": True,
-                        "toggle_url": "_facet=planet_id",
+                        "toggle_url": "_facet=planet_int",
                     }
                 ],
                 "truncated": False,
diff --git a/tests/test_html.py b/tests/test_html.py
index 6d0f3164..6c93d453 100644
--- a/tests/test_html.py
+++ b/tests/test_html.py
@@ -203,15 +203,15 @@ def test_sort_links(app_client):
 
 def test_facets_persist_through_filter_form(app_client):
     response = app_client.get(
-        '/test_tables/facetable?_facet=planet_id&_facet=city',
+        '/test_tables/facetable?_facet=planet_int&_facet=city_id',
         gather_request=False
     )
     assert response.status == 200
     inputs = Soup(response.body, 'html.parser').find('form').findAll('input')
     hiddens = [i for i in inputs if i['type'] == 'hidden']
     assert [
-        ('_facet', 'planet_id'),
-        ('_facet', 'city'),
+        ('_facet', 'planet_int'),
+        ('_facet', 'city_id'),
     ] == [
         (hidden['name'], hidden['value']) for hidden in hiddens
     ]
diff --git a/tests/test_utils.py b/tests/test_utils.py
index 73c40405..e701aae6 100644
--- a/tests/test_utils.py
+++ b/tests/test_utils.py
@@ -36,8 +36,8 @@ def test_urlsafe_components(path, expected):
         ('city', 'Detroit'),
     ), '/?_facet=state&_facet=city&state=MI&city=Detroit'),
     ('/?_facet=state&_facet=city', (
-        ('_facet', 'planet_id'),
-    ), '/?_facet=state&_facet=city&_facet=planet_id'),
+        ('_facet', 'planet_int'),
+    ), '/?_facet=state&_facet=city&_facet=planet_int'),
 ])
 def test_path_with_added_args(path, added_args, expected):
     request = Request(