Limit update() to only work on primary keys#1862
Closed
coolreader18 wants to merge 1 commit intomasterfrom
Closed
Conversation
Contributor
|
Please also update the API proposal |
Contributor
|
Should we change the API? If updates are only by PK, and a table has at most one PK, then they don't really belong on unique indices, do they? Should it be EDIT: Or do we want to leave it on the PK index to make it clear how the old row is found during the update? |
coolreader18
force-pushed
the
noa/no-update-for-non-pk
branch
2 times, most recently
from
4d5a078 to
64a2b2b
Compare
Collaborator
Author
I think that's beneficial - I just added a section in the doc comment for |
coolreader18
force-pushed
the
noa/no-update-for-non-pk
branch
from
64a2b2b to
43d33e0
Compare
coolreader18
force-pushed
the
noa/no-update-for-non-pk
branch
from
43d33e0 to
91d3aa6
Compare
|
|
cloutiertyler
added
release-2.0
release-2.0-nice-to-have
api-break
Contributor
|
I'm going to try to get this to merge, but I don't want to make the |
coolreader18
force-pushed
the
noa/no-update-for-non-pk
branch
from
91d3aa6 to
947f95a
Compare
Collaborator
Author
|
That should be working for the rust module bindings. |
cloutiertyler
added a commit
that referenced
this pull request
Feb 12, 2026
Restrict `update()` in both Rust and C# module bindings to only work on primary key columns, not all unique columns. Calling `update()` on a non-PK unique column is semantically a delete+insert — clients won't see it as a row update unless the primary key stays the same. Rust changes: - Add `PrimaryKey` marker trait in `crates/bindings/src/table.rs` - Add `where Col: PrimaryKey` bound on `UniqueColumn::update()` - Generate `impl PrimaryKey` in the bindings macro for PK columns C# changes: - Only emit `Update()` on `UniqueIndex` when the column is a PK - Update `unique_*` test reducers to use `Delete()`+`Insert()` instead Closes #1862
5 tasks
Contributor
|
Closing in favor of #4279, which re-implements this on top of current master and extends the change to C# and TypeScript module bindings as well. |
github-merge-queue Bot
pushed a commit
that referenced
this pull request
Feb 13, 2026
## Summary - Restricts `update()` across all three server-side SDKs (Rust, C#, TypeScript) to only work on primary key columns, not all unique columns - Calling `update()` on a non-PK unique column is semantically a delete+insert — clients won't see it as a row update unless the primary key stays the same - Fresh re-implementation of #1862, extended to cover C# and TypeScript bindings ### Design principle Primary key is a constraint on columns, not a property of indexes. An index is unique or not unique — whether `update()` is available is derived from the column's attributes at the point of use, not stored on the index. This differs from #1862 which introduced a `Uniqueness` enum (`No | Unique | PrimaryKey`) on the index itself. Instead, we keep `is_unique: bool` and check the column metadata where needed. ### Rust changes - Add `PrimaryKey` marker trait and `where Col: PrimaryKey` bound on `UniqueColumn::update()` - `marker_type()` receives `primary_key_column` and checks the column directly — no `is_pk` field on the index - `sdk-test` unique tables use `update_non_pk_by` (delete+insert) instead of `update_by` - Benchmarks that used `update()` on `#[unique]` columns changed to either `#[primary_key]` or delete+insert ### C# changes - Only emit `Update()` on `UniqueIndex` when the column has `PrimaryKey` attrs - Update `unique_*` test reducers in `sdk-test-cs` to use `Delete()`+`Insert()` instead ### TypeScript changes - `UniqueIndex` now has only `find` + `delete` (no `update`) - `AllColumnsPrimaryKey` type-level helper checks column metadata from the `TableDef` - `Index` routing conditionally intersects `{ update() }` when columns are PK - Runtime only attaches `update` method for PK unique indexes - `sdk-test-ts` unique tables use delete+insert instead of update - Type-level test in `schema.test-d.ts` verifies `id.update()` compiles (PK) and `name2.update` errors (non-PK unique) ## Test plan - [x] C# codegen tests pass (`dotnet test crates/bindings-csharp/Codegen.Tests`) - [x] Rust SDK test module compiles - [x] Benchmarks compile - [x] TypeScript type checks pass (`npx tsc --noEmit`) - [ ] CI passes Closes #1862
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
6 participants
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.
Description of Changes
@cloutiertyler and I were talking about this. Makes the semantic mapping for people easier, a row update on the client is an update() on the server. Doesn't make sense for non-pk to have
.update()but it doesn't trigger an update.Expected complexity level and risk
1