Return 400 instead of 500 for wrong-arity composite-PK row URLs (#2811)#2815
Open
HarperZ9 wants to merge 1 commit into
Open
Return 400 instead of 500 for wrong-arity composite-PK row URLs (#2811)#2815HarperZ9 wants to merge 1 commit into
HarperZ9 wants to merge 1 commit into
Conversation
A row URL with the wrong number of comma-separated primary key components for a compound (multi-column) primary key table raised an uncaught sqlite3.ProgrammingError, surfacing as an HTTP 500. row_sql_params_pks builds one SQL bind placeholder per primary-key column but only binds params for the supplied URL components. When too few components are supplied, a trailing placeholder is left unbound and Datasette.resolve_row executes the SQL with no arity guard, so SQLite raises "You did not supply a value for binding parameter :pN" before the 404 recovery can run. When too many components are supplied the extra params are silently ignored and a misleading result is returned. Add a primary-key arity guard in resolve_row that raises BadRequest (HTTP 400), mirroring the existing guard in datasette/views/table.py (len(pk_values) != len(row_pks) -> BadRequest). BadRequest was not previously imported into app.py, so add it to the .utils.asgi import. Fixes simonw#2811 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2811
The bug
A
GETfor a row page on a table with a composite (multi-column) primary key returns HTTP 500 (sqlite3.ProgrammingError, unbound bind parameter) when the URL has the wrong number of comma-separated primary-key components. The most common trigger in the wild is a proxy/crawler re-encoding the separating comma as%2C, which collapses the arity.Reproduce (confirmed on 1.0a35 / main
34ab85e)Root cause
row_sql_params_pks(datasette/utils/__init__.py) builds one:pNbind placeholder per primary-key column but binds params only for the supplied URL components:When too few components are supplied, the trailing placeholder (e.g.
:p1) is left unbound andDatasette.resolve_row(datasette/app.py) executes the SQL with no arity guard, so SQLite raisesYou did not supply a value for binding parameter :p1— a 500, raised before the existinghandle_404recovery can run. (When too many components are supplied, the extra params are silently ignored and a misleading result is returned.)The identical pk-count mismatch is already guarded with
BadRequestin_fragment_request_for_row(datasette/views/table.py), but the row-page resolution path had no such guard.The fix
Add a primary-key arity guard in
resolve_row, mirroring the existing convention indatasette/views/table.py:A malformed-arity row URL is a client error, so 400 is the appropriate status, consistent with
_fragment_request_for_rowand the?_sorterrors -> 400 change (#1950).BadRequestwas not previously imported intoapp.py, so it is added to thefrom .utils.asgi import (...)block (it is aBase400subclass that yields HTTP 400).After the fix:
A single-column PK value that legitimately contains a comma is unaffected: it tilde-encodes (
a,b->a~2Cb), so it stays one component and resolves normally.Test
Added
test_row_pk_arity_mismatch_returns_400(parametrized over too-few / too-many components and HTML /.json) plustest_row_compound_pk_correct_arityintests/test_api.py, using the existingcompound_primary_keyfixture. The new tests fail onmain(500 / silent-200) and pass with the fix. The 400 assertion style follows the existingtest_sort_errorsprecedent intests/test_table_html.py.Authored with AI assistance (Claude); reviewed and verified before submitting (bug reproduced before the change, fix and tests verified after).