mdcode: don't let an ambiguous 403 masquerade as "absent" in sync - #317
Draft
libei wants to merge 1 commit into
Draft
mdcode: don't let an ambiguous 403 masquerade as "absent" in sync#317libei wants to merge 1 commit into
libei wants to merge 1 commit into
Conversation
The Catalog API returns 403 for both "entry does not exist" and "caller
lacks permission" (noted at sync.ts). Both sync paths treated any non-200
lookup as "absent", producing a wrong outcome each way:
- pull skipped every 403'd entry yet returned {success: true}, so the user
saw "Successfully updated local snapshot" over an empty/partial snapshot
with nothing indicating entries were skipped. Now pull counts the skips
and, when any occur, returns success:false with a message naming the
ambiguity and the first skipped entry (readable entries are still stored,
so the pull is partial rather than aborted).
- push read a 403 lookup as "not there", tried to createEntry, got 409, and
aborted the whole push reporting "Failed to create entry" — pointing at the
wrong problem. Now a 409 on create is reported as already-exists, i.e. the
lookup's 403 was a permission problem, not a missing entry.
Adds a unit test covering both failure modes.
Fixes GoogleCloudPlatform#308
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
1 participant
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.
Fixes #308.
The Catalog API returns 403 for both "entry does not exist" and "caller lacks permission" (noted at
src/libts/sync.ts). Both sync paths treated any non-200 lookup as "absent", producing a wrong outcome in each direction.pull — reported success over a snapshot it did not take
A pull in which every entry 403'd skipped every entry and still returned
{success: true}→ the user saw Successfully updated local snapshot over an empty/partial snapshot.Now pull counts the skips and, when any occur, returns
success: false(surfaced by the CLI, exit 1) with a message that names the 403 ambiguity and the first skipped entry. Readable entries are still stored, so the pull is partial rather than aborted, andSyncResultgains askippedcount.push — attempted a create on an entry that exists
A 403 on the lookup read as "not there", so push called
createEntry, the service returned 409, and the whole push aborted reporting Failed to create entry — the wrong problem.Now a 409 on create is reported as already-exists, i.e. the lookup's 403 was a permission problem on an existing entry, not a missing one. The genuine-new-entry create path is unchanged (new entries also lookup-403, then create succeeds).
Test
Adds
tests/libts/sync.test.tscovering both failure modes (pull skip-counting incl. all-403, push 409→already-exists vs. create-success) with lightweight fakes. Typechecks clean; full existing suite unaffected (pre-existing polyglot-wasm failures are environmental).