fix(undo): record NULL version_id when inline capture is empty#60
Merged
Conversation
ExecuteUndoTransaction's Step 3 inserts a type='undo' log entry for
every affected file, with version_id pointing at the row that the
archive BEFORE trigger wrote during Step 1's DELETE or Step 2's
UPSERT (captured inline immediately after each mutation). The capture
invariant assumes the archive trigger fires -- which it does in any
properly-configured history-enabled workspace.
If the trigger is missing or disabled (a malformed workspace), the
captured-version_id map ends up with no entry for the affected file
and Step 3 reads back the empty string. The previous code passed that
empty string as a UUID parameter, which PG rejected with the cryptic
error: ERROR: invalid input syntax for type uuid: "" (SQLSTATE 22P02).
The entire undo transaction rolled back.
Convert the empty string to a SQL NULL (via a nil interface{}) before
the INSERT, so the transaction completes successfully and records the
undo entry as "no captured snapshot." NULL is already a legitimate
version_id (type='create' log entries record NULL too -- there is no
before-state for a fresh row), and downstream undo-of-undo dispatch
already returns a clear "cannot undo operation: no version_id" error
when it encounters one. The defensive path costs one small helper
applied in both the DELETE and RESTORE Step 3 loops.
To keep the workspace-inconsistency signal loud despite the
transaction succeeding, emit a logging.Warn from the helper with the
affected file_id, the branch (delete vs restore), and a hint to
verify the archive trigger exists on the source table. Operators
investigating the warning find the malformed workspace; the undo
itself isn't blocked by the missing trigger.
Updates TestExecuteUndoTransaction_DeleteOnlyUndo's leading comment
to describe the dual purpose it now serves: the original empty-
RestoreFileIDs branch guard, plus the new defensive NULL-version_id
path. The test passes without any trigger installation, matching the
no-trigger scenario the production fix targets.
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.
ExecuteUndoTransaction's Step 3 inserts a type='undo' log entry for every affected file, with version_id pointing at the row that the archive BEFORE trigger wrote during Step 1's DELETE or Step 2's UPSERT (captured inline immediately after each mutation). The capture invariant assumes the archive trigger fires -- which it does in any properly-configured history-enabled workspace.
If the trigger is missing or disabled (a malformed workspace), the captured-version_id map ends up with no entry for the affected file and Step 3 reads back the empty string. The previous code passed that empty string as a UUID parameter, which PG rejected with the cryptic error: ERROR: invalid input syntax for type uuid: "" (SQLSTATE 22P02). The entire undo transaction rolled back.
Convert the empty string to a SQL NULL (via a nil interface{}) before the INSERT, so the transaction completes successfully and records the undo entry as "no captured snapshot." NULL is already a legitimate version_id (type='create' log entries record NULL too -- there is no before-state for a fresh row), and downstream undo-of-undo dispatch already returns a clear "cannot undo operation: no version_id" error when it encounters one. The defensive path costs one small helper applied in both the DELETE and RESTORE Step 3 loops.
To keep the workspace-inconsistency signal loud despite the transaction succeeding, emit a logging.Warn from the helper with the affected file_id, the branch (delete vs restore), and a hint to verify the archive trigger exists on the source table. Operators investigating the warning find the malformed workspace; the undo itself isn't blocked by the missing trigger.
Updates TestExecuteUndoTransaction_DeleteOnlyUndo's leading comment to describe the dual purpose it now serves: the original empty- RestoreFileIDs branch guard, plus the new defensive NULL-version_id path. The test passes without any trigger installation, matching the no-trigger scenario the production fix targets.