Ensure primary keys created via the JSON API are NOT NULL - #2826
Open
PranavMishra28 wants to merge 2 commits into
Open
Ensure primary keys created via the JSON API are NOT NULL#2826PranavMishra28 wants to merge 2 commits into
PranavMishra28 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>
PranavMishra28
marked this pull request as ready for review
July 7, 2026 20:50
Author
|
@simonw lemme know when you can throw some eyes here or require any other changes |
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).
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.
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.