diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/build-creator.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/build-creator.ts index 8b292544fb..183bf9a196 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/build-creator.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/lib/upload-server/build-creator.ts @@ -18,45 +18,17 @@ export class BuildCreator extends EventEmitter { } // Methods - Public - public changePrVisibility(pr: string, makePublic: boolean): Promise { - const {oldPrDir, newPrDir} = this.getCandidatePrDirs(pr, makePublic); - - return Promise. - all([this.exists(oldPrDir), this.exists(newPrDir)]). - then(([oldPrDirExisted, newPrDirExisted]) => { - if (!oldPrDirExisted) { - throw new UploadError(404, `Request to move non-existing directory '${oldPrDir}' to '${newPrDir}'.`); - } else if (newPrDirExisted) { - throw new UploadError(409, `Request to move '${oldPrDir}' to existing directory '${newPrDir}'.`); - } - - return Promise.resolve(). - then(() => shell.mv(oldPrDir, newPrDir)). - then(() => this.listShasByDate(newPrDir)). - then(shas => this.emit(ChangedPrVisibilityEvent.type, new ChangedPrVisibilityEvent(+pr, shas, makePublic))). - then(() => undefined); - }). - catch(err => { - if (!(err instanceof UploadError)) { - err = new UploadError(500, `Error while making PR ${pr} ${makePublic ? 'public' : 'hidden'}.\n${err}`); - } - - throw err; - }); - } - public create(pr: string, sha: string, archivePath: string, isPublic: boolean): Promise { // Use only part of the SHA for more readable URLs. sha = sha.substr(0, SHORT_SHA_LEN); - const {oldPrDir: otherVisPrDir, newPrDir: prDir} = this.getCandidatePrDirs(pr, isPublic); + const {newPrDir: prDir} = this.getCandidatePrDirs(pr, isPublic); const shaDir = path.join(prDir, sha); let dirToRemoveOnError: string; return Promise.resolve(). - then(() => this.exists(otherVisPrDir)). // If the same PR exists with different visibility, update the visibility first. - then(otherVisPrDirExisted => (otherVisPrDirExisted && this.changePrVisibility(pr, isPublic)) as any). + then(() => this.updatePrVisibility(pr, isPublic)). then(() => Promise.all([this.exists(prDir), this.exists(shaDir)])). then(([prDirExisted, shaDirExisted]) => { if (shaDirExisted) { @@ -84,6 +56,36 @@ export class BuildCreator extends EventEmitter { }); } + public updatePrVisibility(pr: string, makePublic: boolean): Promise { + const {oldPrDir: otherVisPrDir, newPrDir: targetVisPrDir} = this.getCandidatePrDirs(pr, makePublic); + + return Promise. + all([this.exists(otherVisPrDir), this.exists(targetVisPrDir)]). + then(([otherVisPrDirExisted, targetVisPrDirExisted]) => { + if (!otherVisPrDirExisted) { + // No visibility change: Either the visibility is up-to-date or the PR does not exist. + return false; + } else if (targetVisPrDirExisted) { + // Error: Directories for both visibilities exist. + throw new UploadError(409, `Request to move '${otherVisPrDir}' to existing directory '${targetVisPrDir}'.`); + } + + // Visibility change: Moving `otherVisPrDir` to `targetVisPrDir`. + return Promise.resolve(). + then(() => shell.mv(otherVisPrDir, targetVisPrDir)). + then(() => this.listShasByDate(targetVisPrDir)). + then(shas => this.emit(ChangedPrVisibilityEvent.type, new ChangedPrVisibilityEvent(+pr, shas, makePublic))). + then(() => true); + }). + catch(err => { + if (!(err instanceof UploadError)) { + err = new UploadError(500, `Error while making PR ${pr} ${makePublic ? 'public' : 'hidden'}.\n${err}`); + } + + throw err; + }); + } + // Methods - Protected protected exists(fileOrDir: string): Promise { return new Promise(resolve => fs.access(fileOrDir, err => resolve(!err))); diff --git a/aio/aio-builds-setup/dockerbuild/scripts-js/test/upload-server/build-creator.spec.ts b/aio/aio-builds-setup/dockerbuild/scripts-js/test/upload-server/build-creator.spec.ts index c665e3b1a4..6125205ba3 100644 --- a/aio/aio-builds-setup/dockerbuild/scripts-js/test/upload-server/build-creator.spec.ts +++ b/aio/aio-builds-setup/dockerbuild/scripts-js/test/upload-server/build-creator.spec.ts @@ -43,178 +43,25 @@ describe('BuildCreator', () => { }); - describe('changePrVisibility()', () => { - let bcEmitSpy: jasmine.Spy; - let bcExistsSpy: jasmine.Spy; - let bcListShasByDate: jasmine.Spy; - let shellMvSpy: jasmine.Spy; - - beforeEach(() => { - bcEmitSpy = spyOn(bc, 'emit'); - bcExistsSpy = spyOn(bc as any, 'exists'); - bcListShasByDate = spyOn(bc as any, 'listShasByDate'); - shellMvSpy = spyOn(shell, 'mv'); - - bcExistsSpy.and.returnValues(Promise.resolve(true), Promise.resolve(false)); - bcListShasByDate.and.returnValue([]); - }); - - - it('should return a promise', done => { - const promise = bc.changePrVisibility(pr, true); - promise.then(done); // Do not complete the test (and release the spies) synchronously - // to avoid running the actual `extractArchive()`. - - expect(promise).toEqual(jasmine.any(Promise)); - }); - - - [true, false].forEach(makePublic => { - const oldPrDir = makePublic ? hiddenPrDir : publicPrDir; - const newPrDir = makePublic ? publicPrDir : hiddenPrDir; - - - it('should rename the directory', done => { - bc.changePrVisibility(pr, makePublic). - then(() => expect(shellMvSpy).toHaveBeenCalledWith(oldPrDir, newPrDir)). - then(done); - }); - - - it('should emit a ChangedPrVisibilityEvent on success', done => { - let emitted = false; - - bcEmitSpy.and.callFake((type: string, evt: ChangedPrVisibilityEvent) => { - expect(type).toBe(ChangedPrVisibilityEvent.type); - expect(evt).toEqual(jasmine.any(ChangedPrVisibilityEvent)); - expect(evt.pr).toBe(+pr); - expect(evt.shas).toEqual(jasmine.any(Array)); - expect(evt.isPublic).toBe(makePublic); - - emitted = true; - }); - - bc.changePrVisibility(pr, makePublic). - then(() => expect(emitted).toBe(true)). - then(done); - }); - - - it('should include all shas in the emitted event', done => { - const shas = ['foo', 'bar', 'baz']; - let emitted = false; - - bcListShasByDate.and.returnValue(Promise.resolve(shas)); - bcEmitSpy.and.callFake((type: string, evt: ChangedPrVisibilityEvent) => { - expect(bcListShasByDate).toHaveBeenCalledWith(newPrDir); - - expect(type).toBe(ChangedPrVisibilityEvent.type); - expect(evt).toEqual(jasmine.any(ChangedPrVisibilityEvent)); - expect(evt.pr).toBe(+pr); - expect(evt.shas).toBe(shas); - expect(evt.isPublic).toBe(makePublic); - - emitted = true; - }); - - bc.changePrVisibility(pr, makePublic). - then(() => expect(emitted).toBe(true)). - then(done); - }); - - - describe('on error', () => { - - it('should abort and skip further operations if the old directory does not exist', done => { - bcExistsSpy.and.callFake((dir: string) => dir !== oldPrDir); - bc.changePrVisibility(pr, makePublic).catch(err => { - expectToBeUploadError(err, 404, `Request to move non-existing directory '${oldPrDir}' to '${newPrDir}'.`); - expect(shellMvSpy).not.toHaveBeenCalled(); - expect(bcListShasByDate).not.toHaveBeenCalled(); - expect(bcEmitSpy).not.toHaveBeenCalled(); - done(); - }); - }); - - - it('should abort and skip further operations if the new directory does already exist', done => { - bcExistsSpy.and.returnValue(true); - bc.changePrVisibility(pr, makePublic).catch(err => { - expectToBeUploadError(err, 409, `Request to move '${oldPrDir}' to existing directory '${newPrDir}'.`); - expect(shellMvSpy).not.toHaveBeenCalled(); - expect(bcListShasByDate).not.toHaveBeenCalled(); - expect(bcEmitSpy).not.toHaveBeenCalled(); - done(); - }); - }); - - - it('should abort and skip further operations if it fails to rename the directory', done => { - shellMvSpy.and.throwError(''); - bc.changePrVisibility(pr, makePublic).catch(() => { - expect(shellMvSpy).toHaveBeenCalled(); - expect(bcListShasByDate).not.toHaveBeenCalled(); - expect(bcEmitSpy).not.toHaveBeenCalled(); - done(); - }); - }); - - - it('should abort and skip further operations if it fails to list the SHAs', done => { - bcListShasByDate.and.throwError(''); - bc.changePrVisibility(pr, makePublic).catch(() => { - expect(shellMvSpy).toHaveBeenCalled(); - expect(bcListShasByDate).toHaveBeenCalled(); - expect(bcEmitSpy).not.toHaveBeenCalled(); - done(); - }); - }); - - - it('should reject with an UploadError', done => { - shellMvSpy.and.callFake(() => { throw 'Test'; }); - bc.changePrVisibility(pr, makePublic).catch(err => { - expectToBeUploadError(err, 500, `Error while making PR ${pr} ${makePublic ? 'public' : 'hidden'}.\nTest`); - done(); - }); - }); - - - it('should pass UploadError instances unmodified', done => { - shellMvSpy.and.callFake(() => { throw new UploadError(543, 'Test'); }); - bc.changePrVisibility(pr, makePublic).catch(err => { - expectToBeUploadError(err, 543, 'Test'); - done(); - }); - }); - - }); - - }); - - }); - - describe('create()', () => { - let bcChangePrVisibilitySpy: jasmine.Spy; let bcEmitSpy: jasmine.Spy; let bcExistsSpy: jasmine.Spy; let bcExtractArchiveSpy: jasmine.Spy; + let bcUpdatePrVisibilitySpy: jasmine.Spy; let shellMkdirSpy: jasmine.Spy; let shellRmSpy: jasmine.Spy; beforeEach(() => { - bcChangePrVisibilitySpy = spyOn(bc, 'changePrVisibility'); bcEmitSpy = spyOn(bc, 'emit'); bcExistsSpy = spyOn(bc as any, 'exists'); bcExtractArchiveSpy = spyOn(bc as any, 'extractArchive'); + bcUpdatePrVisibilitySpy = spyOn(bc, 'updatePrVisibility'); shellMkdirSpy = spyOn(shell, 'mkdir'); shellRmSpy = spyOn(shell, 'rm'); }); [true, false].forEach(isPublic => { - const otherVisPrDir = isPublic ? hiddenPrDir : publicPrDir; const prDir = isPublic ? publicPrDir : hiddenPrDir; const shaDir = isPublic ? publicShaDir : hiddenShaDir; @@ -228,20 +75,12 @@ describe('BuildCreator', () => { }); - it('should not update the PR\'s visibility first if not necessary', done => { - bc.create(pr, sha, archive, isPublic). - then(() => expect(bcChangePrVisibilitySpy).not.toHaveBeenCalled()). - then(done); - }); - - it('should update the PR\'s visibility first if necessary', done => { - bcChangePrVisibilitySpy.and.callFake(() => expect(shellMkdirSpy).not.toHaveBeenCalled()); - bcExistsSpy.and.callFake((dir: string) => dir === otherVisPrDir); + bcUpdatePrVisibilitySpy.and.callFake(() => expect(shellMkdirSpy).not.toHaveBeenCalled()); bc.create(pr, sha, archive, isPublic). then(() => { - expect(bcChangePrVisibilitySpy).toHaveBeenCalledWith(pr, isPublic); + expect(bcUpdatePrVisibilitySpy).toHaveBeenCalledWith(pr, isPublic); expect(shellMkdirSpy).toHaveBeenCalled(); }). then(done); @@ -286,7 +125,6 @@ describe('BuildCreator', () => { beforeEach(() => { existsValues = { - [otherVisPrDir]: false, [prDir]: false, [shaDir]: false, }; @@ -297,14 +135,12 @@ describe('BuildCreator', () => { it('should abort and skip further operations if changing the PR\'s visibility fails', done => { const mockError = new UploadError(543, 'Test'); - - existsValues[otherVisPrDir] = true; - bcChangePrVisibilitySpy.and.returnValue(Promise.reject(mockError)); + bcUpdatePrVisibilitySpy.and.returnValue(Promise.reject(mockError)); bc.create(pr, sha, archive, isPublic).catch(err => { expect(err).toBe(mockError); - expect(bcExistsSpy).toHaveBeenCalledTimes(1); + expect(bcExistsSpy).not.toHaveBeenCalled(); expect(shellMkdirSpy).not.toHaveBeenCalled(); expect(bcExtractArchiveSpy).not.toHaveBeenCalled(); expect(bcEmitSpy).not.toHaveBeenCalled(); @@ -327,8 +163,10 @@ describe('BuildCreator', () => { it('should detect existing build directory after visibility change', done => { - existsValues[otherVisPrDir] = true; - bcChangePrVisibilitySpy.and.callFake(() => existsValues[prDir] = existsValues[shaDir] = true); + bcUpdatePrVisibilitySpy.and.callFake(() => existsValues[prDir] = existsValues[shaDir] = true); + + expect(bcExistsSpy(prDir)).toBe(false); + expect(bcExistsSpy(shaDir)).toBe(false); bc.create(pr, sha, archive, isPublic).catch(err => { expectToBeUploadError(err, 409, `Request to overwrite existing directory: ${shaDir}`); @@ -406,6 +244,190 @@ describe('BuildCreator', () => { }); + describe('updatePrVisibility()', () => { + let bcEmitSpy: jasmine.Spy; + let bcExistsSpy: jasmine.Spy; + let bcListShasByDate: jasmine.Spy; + let shellMvSpy: jasmine.Spy; + + beforeEach(() => { + bcEmitSpy = spyOn(bc, 'emit'); + bcExistsSpy = spyOn(bc as any, 'exists'); + bcListShasByDate = spyOn(bc as any, 'listShasByDate'); + shellMvSpy = spyOn(shell, 'mv'); + + bcExistsSpy.and.returnValues(Promise.resolve(true), Promise.resolve(false)); + bcListShasByDate.and.returnValue([]); + }); + + + it('should return a promise', done => { + const promise = bc.updatePrVisibility(pr, true); + promise.then(done); // Do not complete the test (and release the spies) synchronously + // to avoid running the actual `extractArchive()`. + + expect(promise).toEqual(jasmine.any(Promise)); + }); + + + [true, false].forEach(makePublic => { + const oldPrDir = makePublic ? hiddenPrDir : publicPrDir; + const newPrDir = makePublic ? publicPrDir : hiddenPrDir; + + + it('should rename the directory', done => { + bc.updatePrVisibility(pr, makePublic). + then(() => expect(shellMvSpy).toHaveBeenCalledWith(oldPrDir, newPrDir)). + then(done); + }); + + + describe('when the visibility is updated', () => { + + it('should resolve to true', done => { + bc.updatePrVisibility(pr, makePublic). + then(result => expect(result).toBe(true)). + then(done); + }); + + + it('should rename the directory', done => { + bc.updatePrVisibility(pr, makePublic). + then(() => expect(shellMvSpy).toHaveBeenCalledWith(oldPrDir, newPrDir)). + then(done); + }); + + + it('should emit a ChangedPrVisibilityEvent on success', done => { + let emitted = false; + + bcEmitSpy.and.callFake((type: string, evt: ChangedPrVisibilityEvent) => { + expect(type).toBe(ChangedPrVisibilityEvent.type); + expect(evt).toEqual(jasmine.any(ChangedPrVisibilityEvent)); + expect(evt.pr).toBe(+pr); + expect(evt.shas).toEqual(jasmine.any(Array)); + expect(evt.isPublic).toBe(makePublic); + + emitted = true; + }); + + bc.updatePrVisibility(pr, makePublic). + then(() => expect(emitted).toBe(true)). + then(done); + }); + + + it('should include all shas in the emitted event', done => { + const shas = ['foo', 'bar', 'baz']; + let emitted = false; + + bcListShasByDate.and.returnValue(Promise.resolve(shas)); + bcEmitSpy.and.callFake((type: string, evt: ChangedPrVisibilityEvent) => { + expect(bcListShasByDate).toHaveBeenCalledWith(newPrDir); + + expect(type).toBe(ChangedPrVisibilityEvent.type); + expect(evt).toEqual(jasmine.any(ChangedPrVisibilityEvent)); + expect(evt.pr).toBe(+pr); + expect(evt.shas).toBe(shas); + expect(evt.isPublic).toBe(makePublic); + + emitted = true; + }); + + bc.updatePrVisibility(pr, makePublic). + then(() => expect(emitted).toBe(true)). + then(done); + }); + + }); + + + it('should do nothing if the visibility is already up-to-date', done => { + bcExistsSpy.and.callFake((dir: string) => dir === newPrDir); + bc.updatePrVisibility(pr, makePublic). + then(result => { + expect(result).toBe(false); + expect(shellMvSpy).not.toHaveBeenCalled(); + expect(bcListShasByDate).not.toHaveBeenCalled(); + expect(bcEmitSpy).not.toHaveBeenCalled(); + }). + then(done); + }); + + + it('should do nothing if the PR directory does not exist', done => { + bcExistsSpy.and.returnValue(false); + bc.updatePrVisibility(pr, makePublic). + then(result => { + expect(result).toBe(false); + expect(shellMvSpy).not.toHaveBeenCalled(); + expect(bcListShasByDate).not.toHaveBeenCalled(); + expect(bcEmitSpy).not.toHaveBeenCalled(); + }). + then(done); + }); + + + describe('on error', () => { + + it('should abort and skip further operations if both directories exist', done => { + bcExistsSpy.and.returnValue(true); + bc.updatePrVisibility(pr, makePublic).catch(err => { + expectToBeUploadError(err, 409, `Request to move '${oldPrDir}' to existing directory '${newPrDir}'.`); + expect(shellMvSpy).not.toHaveBeenCalled(); + expect(bcListShasByDate).not.toHaveBeenCalled(); + expect(bcEmitSpy).not.toHaveBeenCalled(); + done(); + }); + }); + + + it('should abort and skip further operations if it fails to rename the directory', done => { + shellMvSpy.and.throwError(''); + bc.updatePrVisibility(pr, makePublic).catch(() => { + expect(shellMvSpy).toHaveBeenCalled(); + expect(bcListShasByDate).not.toHaveBeenCalled(); + expect(bcEmitSpy).not.toHaveBeenCalled(); + done(); + }); + }); + + + it('should abort and skip further operations if it fails to list the SHAs', done => { + bcListShasByDate.and.throwError(''); + bc.updatePrVisibility(pr, makePublic).catch(() => { + expect(shellMvSpy).toHaveBeenCalled(); + expect(bcListShasByDate).toHaveBeenCalled(); + expect(bcEmitSpy).not.toHaveBeenCalled(); + done(); + }); + }); + + + it('should reject with an UploadError', done => { + shellMvSpy.and.callFake(() => { throw 'Test'; }); + bc.updatePrVisibility(pr, makePublic).catch(err => { + expectToBeUploadError(err, 500, `Error while making PR ${pr} ${makePublic ? 'public' : 'hidden'}.\nTest`); + done(); + }); + }); + + + it('should pass UploadError instances unmodified', done => { + shellMvSpy.and.callFake(() => { throw new UploadError(543, 'Test'); }); + bc.updatePrVisibility(pr, makePublic).catch(err => { + expectToBeUploadError(err, 543, 'Test'); + done(); + }); + }); + + }); + + }); + + }); + + // Protected methods describe('exists()', () => { diff --git a/aio/aio-builds-setup/docs/overview--http-status-codes.md b/aio/aio-builds-setup/docs/overview--http-status-codes.md index dff11ae01f..1fcd6eda35 100644 --- a/aio/aio-builds-setup/docs/overview--http-status-codes.md +++ b/aio/aio-builds-setup/docs/overview--http-status-codes.md @@ -42,10 +42,6 @@ with a bried explanation of what they mean: - **403 (Forbidden)**: Unable to verify build (e.g. invalid JWT token, or unable to talk to 3rd-party APIs, etc). -- **404 (Not Found)**: - Tried to change PR visibility but the source directory did not exist. - (Currently, this can only happen as a rare race condition during build deployment.) - - **405 (Method Not Allowed)**: Request method other than POST.