diff --git a/e2e-docker/npm_local_test.go b/e2e-docker/npm_local_test.go index 30c873d..9455611 100644 --- a/e2e-docker/npm_local_test.go +++ b/e2e-docker/npm_local_test.go @@ -36,7 +36,7 @@ mk() { d="/w/src/$1-$2"; mkdir -p "$d"; cd "$d" echo "module.exports = '$1@$2';" > index.js; } mk e2e-npm-plain 1.0.0 && npm publish mk @e2e/npm-scoped 1.0.0 && npm publish -mk e2e-npm-plain 1.0.0 && echo "// changed" >> index.js +mk e2e-npm-plain 1.0.0 if npm publish 2>/dev/null; then echo "republish accepted"; exit 1; fi mkdir -p /w/app && cd /w/app && npm init -y >/dev/null npm install --no-audit --cache /tmp/fresh-cache e2e-npm-plain @e2e/npm-scoped diff --git a/internal/api/v1/npm.go b/internal/api/v1/npm.go index 41cc5e1..18604cf 100644 --- a/internal/api/v1/npm.go +++ b/internal/api/v1/npm.go @@ -176,23 +176,27 @@ func (h *ProxyHandler) handleLocalPut(w http.ResponseWriter, r *http.Request) { base := pkg[strings.LastIndex(pkg, "/")+1:] stem := pkg + "/-/" + base + "-" + rel.version // The manifest gates visibility and tags may only name visible versions, so - // the order is tarball, manifest, tags. Identical bytes make retries idempotent. - for _, f := range []struct { - path, ct string - data []byte - }{ - {stem + ".tgz", "application/octet-stream", rel.tarball}, - {stem + ".json", "application/json", rel.manifest}, - } { - ok, err := h.npmStoreImmutable(r.Context(), remote.Name, f.path, f.data, f.ct) - if err != nil { - npmError(w, http.StatusInternalServerError, err.Error()) - return - } - if !ok { - npmError(w, http.StatusConflict, fmt.Sprintf("%s@%s already exists; overwrites are not allowed", pkg, rel.version)) + // the order is tarball, manifest, tags. A tarball left by a failed publish is + // accepted again only with identical bytes; an existing manifest always conflicts. + conflict := func() { + npmError(w, http.StatusConflict, fmt.Sprintf("%s@%s already exists; overwrites are not allowed", pkg, rel.version)) + } + ok, err = h.npmStoreImmutable(r.Context(), remote.Name, stem+".tgz", rel.tarball, "application/octet-stream") + if err != nil { + npmError(w, http.StatusInternalServerError, err.Error()) + return + } + if !ok { + conflict() + return + } + if _, err := h.npmStore(r.Context(), remote.Name, stem+".json", rel.manifest, "application/json", false); err != nil { + if errors.Is(err, database.ErrAlreadyExists) { + conflict() return } + npmError(w, http.StatusInternalServerError, err.Error()) + return } for _, tag := range rel.tags { if _, err := h.npmStore(r.Context(), remote.Name, pkg+"/-/dist-tags/"+tag, []byte(rel.version), "text/plain", true); err != nil { diff --git a/internal/api/v1/npm_handler_test.go b/internal/api/v1/npm_handler_test.go index 70821d6..8b21c09 100644 --- a/internal/api/v1/npm_handler_test.go +++ b/internal/api/v1/npm_handler_test.go @@ -163,8 +163,14 @@ func TestNPMPublishHandler(t *testing.T) { if code, _ := npmDo(t, http.MethodPut, base+"repub", publishBody(t, "repub", []byte("different"), publishOpts{})); code != http.StatusConflict { t.Errorf("different bytes = %d, want 409", code) } - if code, _ := npmDo(t, http.MethodPut, base+"repub", publishBody(t, "repub", []byte("v1"), publishOpts{})); code != http.StatusCreated { - t.Errorf("identical republish = %d, want 201", code) + if code, b := npmDo(t, http.MethodPut, base+"repub", publishBody(t, "repub", []byte("v2"), publishOpts{version: "1.1.0"})); code != http.StatusCreated { + t.Fatalf("publish 1.1.0 = %d: %s", code, b) + } + if code, _ := npmDo(t, http.MethodPut, base+"repub", publishBody(t, "repub", []byte("v1"), publishOpts{})); code != http.StatusConflict { + t.Errorf("identical republish = %d, want 409", code) + } + if tags := packumentTags(t, base+"repub"); tags["latest"] != "1.1.0" { + t.Errorf("latest = %q, want 1.1.0", tags["latest"]) } }) @@ -204,8 +210,11 @@ func TestNPMPublishHandler(t *testing.T) { t.Run("concurrent identical", func(t *testing.T) { body := publishBody(t, "same", []byte("same tgz"), publishOpts{}) codes := concurrentPuts(t, base+"same", body, body) - if codes[0] != http.StatusCreated || codes[1] != http.StatusCreated { - t.Fatalf("codes = %v", codes) + if codes[0]+codes[1] != http.StatusCreated+http.StatusConflict || codes[0] == codes[1] { + t.Fatalf("codes = %v, want one 201 and one 409", codes) + } + if tags := packumentTags(t, base+"same"); len(tags) != 1 || tags["latest"] != "1.0.0" { + t.Errorf("tags = %v", tags) } })