Skip to content

Commit

Permalink
Revert #216 until I can get tests to pass in Travis
Browse files Browse the repository at this point in the history
Revert "Fix for _sort_desc=sortable_with_nulls test, refs #216"

This reverts commit 07fc2d1.

Revert "Fixed #216 - paginate correctly when sorting by nullable column"

This reverts commit 2abe539.
  • Loading branch information
simonw committed Apr 17, 2018
1 parent 07fc2d1 commit 5364fa7
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 61 deletions.
49 changes: 12 additions & 37 deletions datasette/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -739,9 +739,6 @@ async def data(self, request, name, hash, table):
# 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
Expand All @@ -763,35 +760,15 @@ async def data(self, request, name, hash, table):

# 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)

Expand Down Expand Up @@ -846,12 +823,10 @@ async def data(self, request, name, hash, table):
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,
}
Expand Down
5 changes: 1 addition & 4 deletions tests/fixtures.py
Original file line number Diff line number Diff line change
Expand Up @@ -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']),
}


Expand All @@ -79,7 +78,6 @@ def generate_sortable_rows(num):
'sortable',
'sortable_with_nulls',
'sortable_with_nulls_2',
'text',
]
},
'no_primary_key': {
Expand Down Expand Up @@ -155,7 +153,6 @@ def convert_units(amount, from_, to_):
sortable integer,
sortable_with_nulls real,
sortable_with_nulls_2 real,
text text,
PRIMARY KEY (pk1, pk2)
);
Expand Down Expand Up @@ -238,7 +235,7 @@ def convert_units(amount, from_, to_):
]) + '\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)
Expand Down
21 changes: 1 addition & 20 deletions tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand Down

0 comments on commit 5364fa7

Please sign in to comment.