From 5364fa7f3357f2de24fd45c85832205377642f19 Mon Sep 17 00:00:00 2001 From: Simon Willison Date: Mon, 16 Apr 2018 17:52:22 -0700 Subject: [PATCH] Revert #216 until I can get tests to pass in Travis Revert "Fix for _sort_desc=sortable_with_nulls test, refs #216" This reverts commit 07fc2d113e462bfd8d7d56152c0d1fc55e0fdbe9. Revert "Fixed #216 - paginate correctly when sorting by nullable column" This reverts commit 2abe539a0f9f967ec0de6894774cb7ee83c4b3b9. --- datasette/app.py | 49 ++++++++++++----------------------------------- tests/fixtures.py | 5 +---- tests/test_api.py | 21 +------------------- 3 files changed, 14 insertions(+), 61 deletions(-) diff --git a/datasette/app.py b/datasette/app.py index cc16ae97..d17f2096 100644 --- a/datasette/app.py +++ b/datasette/app.py @@ -739,9 +739,6 @@ class TableView(RowTableShared): # If a sort order is applied, the first of these is the sort value if sort or sort_desc: sort_value = components[0] - # Special case for if non-urlencoded first token was $null - if _next.split(',')[0] == '$null': - sort_value = None components = components[1:] # Figure out the SQL for next-based-on-primary-key first @@ -763,35 +760,15 @@ class TableView(RowTableShared): # Now add the sort SQL, which may incorporate next_by_pk_clauses if sort or sort_desc: - if sort_value is None: - if sort_desc: - # Just items where column is null ordered by pk - where_clauses.append( - '({column} is null and {next_clauses})'.format( - column=escape_sqlite(sort_desc), - next_clauses=' and '.join(next_by_pk_clauses), - ) - ) - else: - where_clauses.append( - '({column} is not null or ({column} is null and {next_clauses}))'.format( - column=escape_sqlite(sort), - next_clauses=' and '.join(next_by_pk_clauses), - ) - ) - else: - where_clauses.append( - '({column} {op} :p{p}{extra_desc_only} or ({column} = :p{p} and {next_clauses}))'.format( - column=escape_sqlite(sort or sort_desc), - op='>' if sort else '<', - p=len(params), - extra_desc_only='' if sort else ' or {column2} is null'.format( - column2=escape_sqlite(sort or sort_desc), - ), - next_clauses=' and '.join(next_by_pk_clauses), - ) + where_clauses.append( + '({column} {op} :p{p} or ({column} = :p{p} and {next_clauses}))'.format( + column=escape_sqlite(sort or sort_desc), + op='>' if sort else '<', + p=len(params), + next_clauses=' and '.join(next_by_pk_clauses), ) - params['p{}'.format(len(params))] = sort_value + ) + params['p{}'.format(len(params))] = sort_value else: where_clauses.extend(next_by_pk_clauses) @@ -846,12 +823,10 @@ class TableView(RowTableShared): next_value = path_from_row_pks(rows[-2], pks, use_rowid) # If there's a sort or sort_desc, add that value as a prefix if (sort or sort_desc) and not is_view: - prefix = rows[-2][sort or sort_desc] - if prefix is None: - prefix = '$null' - else: - prefix = urllib.parse.quote_plus(str(prefix)) - next_value = '{},{}'.format(prefix, next_value) + prefix = str(rows[-2][sort or sort_desc]) + next_value = '{},{}'.format( + urllib.parse.quote_plus(prefix), next_value + ) added_args = { '_next': next_value, } diff --git a/tests/fixtures.py b/tests/fixtures.py index 29075d7f..38df8b00 100644 --- a/tests/fixtures.py +++ b/tests/fixtures.py @@ -55,7 +55,6 @@ def generate_sortable_rows(num): 'sortable_with_nulls_2': rand.choice([ None, rand.random(), rand.random() ]), - 'text': rand.choice(['$null', '$blah']), } @@ -79,7 +78,6 @@ METADATA = { 'sortable', 'sortable_with_nulls', 'sortable_with_nulls_2', - 'text', ] }, 'no_primary_key': { @@ -155,7 +153,6 @@ CREATE TABLE sortable ( sortable integer, sortable_with_nulls real, sortable_with_nulls_2 real, - text text, PRIMARY KEY (pk1, pk2) ); @@ -238,7 +235,7 @@ CREATE VIEW simple_view AS ]) + '\n'.join([ '''INSERT INTO sortable VALUES ( "{pk1}", "{pk2}", "{content}", {sortable}, - {sortable_with_nulls}, {sortable_with_nulls_2}, "{text}"); + {sortable_with_nulls}, {sortable_with_nulls_2}); '''.format( **row ).replace('None', 'null') for row in generate_sortable_rows(201) diff --git a/tests/test_api.py b/tests/test_api.py index 7d9548b0..21fc34bf 100644 --- a/tests/test_api.py +++ b/tests/test_api.py @@ -156,7 +156,7 @@ def test_database_page(app_client): }, { 'columns': [ 'pk1', 'pk2', 'content', 'sortable', 'sortable_with_nulls', - 'sortable_with_nulls_2', 'text', + 'sortable_with_nulls_2' ], 'name': 'sortable', 'count': 201, @@ -426,25 +426,6 @@ def test_paginate_compound_keys_with_extra_filters(app_client): @pytest.mark.parametrize('query_string,sort_key,human_description_en', [ ('_sort=sortable', lambda row: row['sortable'], 'sorted by sortable'), ('_sort_desc=sortable', lambda row: -row['sortable'], 'sorted by sortable descending'), - ( - '_sort=sortable_with_nulls', - lambda row: ( - 1 if row['sortable_with_nulls'] is not None else 0, - row['sortable_with_nulls'] - ), - 'sorted by sortable_with_nulls' - ), - ( - '_sort_desc=sortable_with_nulls', - lambda row: ( - 1 if row['sortable_with_nulls'] is None else 0, - -row['sortable_with_nulls'] if row['sortable_with_nulls'] is not None else 0, - row['content'] - ), - 'sorted by sortable_with_nulls descending' - ), - # text column contains '$null' - ensure it doesn't confuse pagination: - ('_sort=text', lambda row: row['text'], 'sorted by text'), ]) def test_sortable(app_client, query_string, sort_key, human_description_en): path = '/test_tables/sortable.json?_shape=objects&{}'.format(query_string)