Ensure primary keys created via the JSON API are NOT NULL - #2826
Ensure primary keys created via the JSON API are NOT NULL#2826PranavMishra28 wants to merge 2 commits into
Conversation
The table-create JSON API built its own CREATE TABLE without marking primary key columns NOT NULL, so a text or composite primary key could be created nullable. SQLite then allows rows with a NULL primary key, which cannot be viewed, edited or deleted. Force primary key columns NOT NULL, leaving a lone integer primary key alone since it aliases the rowid and is never NULL. Refs simonw#2807 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@simonw lemme know when you can throw some eyes here or require any other changes |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3956e63650
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
SQLite resolves identifiers case-insensitively, so a request with pk "code" against a column named "Code" still creates that column as the primary key. The membership check was case-sensitive, so that column was left out of not_null and the API could still create a nullable text primary key, which is the exact unviewable/undeletable NULL-PK row this change is meant to prevent. The integer-rowid-alias exemption is matched the same way, so an integer pk whose casing differs still stays a plain rowid alias rather than picking up a constraint it does not need. Two cases added to test_create_table_primary_keys_are_not_null; the text one fails without this change (notnull 0, expected 1).
|
status check rather than a ping, since this has been sitting a month and the tree has moved a lot since I opened it. re-verified against current one thing that did change after I opened it, which is the part worth a second look: the first version only matched primary key names case-sensitively. the review bot caught that SQLite resolves identifiers case-insensitively, so happy to split the case-insensitivity part into its own commit if that reads better in review, or to rebase whenever you want it current. |
1 participant
What was broken
The table-create JSON API (
/db/-/create) builds its ownCREATE TABLE(the explicit-columns path inTableCreateView.create_table) without marking primary key columnsNOT NULL. So a text or composite primary key is created nullable:→
CREATE TABLE "t" ("code" TEXT PRIMARY KEY, ...)—codecan beNULL.Why it matters
SQLite permits a
NULLvalue in any primary key that isn't a single-columnINTEGER PRIMARY KEY(and lets multiple rows share theNULLkey). As #2805 shows, such rows can't be viewed, edited or deleted through Datasette — the row URL resolves toNone/"". #2807 asks to "stem the bleeding" by disallowing null primary keys for tables created via the JSON API.What the fix does
In the explicit-columns create path — the one Simon described as "any time we run our own CREATE TABLE we set not null on any primary key we create" — primary key columns are added to the
not_nullset passed totable.create(). A single-columnINTEGER PRIMARY KEYis skipped because it aliases the rowid and can never beNULL, so no redundant constraint is emitted (and the commonidschema is unchanged).Result:
"code" TEXT PRIMARY KEY NOT NULL; composite keys getNOT NULLon every key column.What it deliberately does not change
INTEGER PRIMARY KEYoutput is untouched (stillINTEGER PRIMARY KEY), so existing schemas/tests/docs are unaffected.rows/insert_allinference path is not changed here. That path can also create a nullable text pk, but because its column types are inferred rather than declared, the single-integer-pk (rowid) exception can't be applied reliably in the same way. Happy to follow up on that path if you'd like it covered too — henceRefs #2807rather thanFixes.Testing
test_create_table_primary_keys_are_not_nullcovers a single text pk and a composite pk (schema +pragma_table_infonotnull), and asserts non-pk columns stay nullable.tests/test_api_write.py(148 passed), plustests/test_schema_endpoints.pyandtests/test_api.py, all green;black --checkandruff checkclean.Refs #2807
Developed with Claude Code; reviewed and tested by Pranav before marking ready for review.