fix(table): reject deleted schema move targets - #1905
Conversation
Signed-off-by: Matt Faltyn <faltyn.matthew@gmail.com>
laskoviymishka
left a comment
laskoviymishka
left a comment
There was a problem hiding this comment.
Nice, targeted fix: the two symmetric guards are the right instinct, and they close the exact #1903 repro cleanly in both orderings.
I'd hold for now. The guards patch the two entry points, but the actual silent drop lives in moveFields: the !found { continue } branch (~line 1370) pulls the field out of the working slice and then bails with no error. This PR makes that branch unreachable for the two tested orderings, but it's still a landmine for any future path — and there's already one it doesn't cover: MoveBefore(["age"], ["name"]) then DeleteColumn(["age"]) slips past both guards and silently swallows the staged move. I'd rather fix moveFields to not drop on !found so the root is closed in one place, instead of enumerating call-site orderings.
A couple of things I'd like to settle before merge:
- fix (or at least guard) the silent drop at its root in
moveFields, so the move-source-delete ordering surfaces an error too - tighten the test to distinguish the two guards (split on the distinct messages) and add a positive case where a delete plus an unrelated move still succeeds
Once those are settled, happy to take another pass.
| return fmt.Errorf("field that has updates cannot be deleted: %s", fullName) | ||
| } | ||
| } | ||
| for _, move := range u.moves[parentID] { |
There was a problem hiding this comment.
the two guards close both orderings in #1903, but they're patching entry points — the real silent drop is in moveFields, where the !found { continue } branch (~line 1370) removes the field from the working slice and then bails with no error. this PR makes that branch unreachable for the tested orderings, but it's still a landmine for any future path.
and there's already one it doesn't cover: MoveBefore(["age"], ["name"]) then DeleteColumn(["age"]). this guard only inspects move.RelativeTo, not move.FieldID, so neither guard fires and age's staged move is silently swallowed at apply (output happens to be correct, but there's no diagnostic).
I'd rather fix moveFields to not drop on !found — restore the field and continue, or return an error — so the root is closed in one place instead of enumerating call-site orderings. wdyt?
There was a problem hiding this comment.
“fix
moveFieldsto not drop on!found”
Great catch — moveFields now fails closed for missing sources or targets, with source-delete and direct target-loss coverage.
| t.Run(tt.name, func(t *testing.T) { | ||
| tbl := New([]string{"id"}, testMetadata, "", nil, nil) | ||
| _, err := tt.build(NewUpdateSchema(tbl.NewTransaction(), true, true)).Apply() | ||
| require.ErrorContains(t, err, "move target") |
There was a problem hiding this comment.
these assertions can't tell the two guards apart — both error messages contain "move target", so cases 1/3 exercise the moveColumn guard and 2/4 the deleteColumn guard, but all four pass on the same substring. if one guard broke and the other picked up the slack, this would stay green.
I'd split it by the distinguishing text: assert "cannot be deleted" for the move-then-delete cases and "has been deleted" for the delete-then-move cases.
There was a problem hiding this comment.
“split it by the distinguishing text”
Good suggestion — the two orderings now assert has been deleted and cannot be deleted separately.
| }) | ||
| } | ||
|
|
||
| func TestMoveRelativeToDeletedColumnRejected(t *testing.T) { |
There was a problem hiding this comment.
all four cases expect an error, so this proves the guards fire but not that they fire only when they should. I'd add a positive case — DeleteColumn(["name"]) then MoveBefore(["age"], ["id"]) should still succeed and land in the expected order — so an over-eager guard can't slip through unnoticed.
There was a problem hiding this comment.
“add a positive case”
Agreed — unrelated deletion plus move now succeeds and asserts the final field order.
| { | ||
| name: "delete then move before", | ||
| build: func(update *UpdateSchema) *UpdateSchema { | ||
| return update.DeleteColumn([]string{"name"}).MoveBefore([]string{"age"}, []string{"name"}) |
There was a problem hiding this comment.
these are all flat top-level fields, so the deleteColumn guard only ever scans u.moves[-1] (parentID resolves to the root). a nested target like address.city keys its move under address's ID — a different bucket that's currently untested. one nested case would pin that path down.
There was a problem hiding this comment.
“one nested case would pin that path down”
Nice call — added a nested address.city target case to exercise the parent-ID bucket.
Signed-off-by: Matt Faltyn <faltyn.matthew@gmail.com>
2 participants
Description
Fixes #1903.
Reject schema updates that either move a field relative to a column already staged for deletion or delete a column already referenced as a move target. This prevents
moveFieldsfrom silently omitting the moved field when the target is absent from the resulting schema.The regression test covers both operation orders and both
MoveBeforeandMoveAfter.Testing
go test ./table -run 'TestMoveRelativeToDeletedColumnRejected|TestMoveColumn|TestDeleteColumn|TestChainedOperations' -count=1make testmake test-assertmake test-racemake lintgo build ./...