From 18ff2eb693531f8c4bc1637dea02abd6445e8fb4 Mon Sep 17 00:00:00 2001 From: ifielker Date: Mon, 10 Feb 2020 16:49:29 -0500 Subject: [PATCH 1/2] Added DeleteModel functionality and tests --- .../machine-learning-api-client.ts | 12 +++ src/machine-learning/machine-learning.ts | 2 +- test/integration/machine-learning.spec.ts | 17 ++++ .../machine-learning-api-client.spec.ts | 78 +++++++++++++++++++ .../machine-learning/machine-learning.spec.ts | 20 +++++ 5 files changed, 128 insertions(+), 1 deletion(-) diff --git a/src/machine-learning/machine-learning-api-client.ts b/src/machine-learning/machine-learning-api-client.ts index 766752e7e0..8331924371 100644 --- a/src/machine-learning/machine-learning-api-client.ts +++ b/src/machine-learning/machine-learning-api-client.ts @@ -83,6 +83,18 @@ export class MachineLearningApiClient { }); } + public deleteModel(modelId: string): Promise { + return this.getUrl() + .then((url) => { + const modelName = this.getModelName(modelId); + const request: HttpRequestConfig = { + method: 'DELETE', + url: `${url}/${modelName}`, + }; + return this.sendRequest(request); + }); + } + /** * Gets the specified resource from the ML API. Resource names must be the short names without project * ID prefix (e.g. `models/123456789`). diff --git a/src/machine-learning/machine-learning.ts b/src/machine-learning/machine-learning.ts index 9e61e42120..cb4ae0cf6a 100644 --- a/src/machine-learning/machine-learning.ts +++ b/src/machine-learning/machine-learning.ts @@ -168,7 +168,7 @@ export class MachineLearning implements FirebaseServiceInterface { * @param {string} modelId The id of the model to delete. */ public deleteModel(modelId: string): Promise { - throw new Error('NotImplemented'); + return this.client.deleteModel(modelId); } } diff --git a/test/integration/machine-learning.spec.ts b/test/integration/machine-learning.spec.ts index 5663b0dc62..e625dca562 100644 --- a/test/integration/machine-learning.spec.ts +++ b/test/integration/machine-learning.spec.ts @@ -31,4 +31,21 @@ describe('admin.machineLearning', () => { 'code', 'machine-learning/invalid-argument'); }); }); + + describe('deleteModel()', () => { + it('rejects with not-found when the Model does not exist', () => { + const nonExistingName = '00000000'; + return admin.machineLearning().deleteModel(nonExistingName) + .should.eventually.be.rejected.and.have.property( + 'code', 'machine-learning/not-found'); + }); + + it('rejects with invalid-argument when the Model ID is invalid', () => { + return admin.machineLearning().deleteModel('invalid-model-id') + .should.eventually.be.rejected.and.have.property( + 'code', 'machine-learning/invalid-argument'); + }); + + + }); }); diff --git a/test/unit/machine-learning/machine-learning-api-client.spec.ts b/test/unit/machine-learning/machine-learning-api-client.spec.ts index 16875e292d..4840052f0d 100644 --- a/test/unit/machine-learning/machine-learning-api-client.spec.ts +++ b/test/unit/machine-learning/machine-learning-api-client.spec.ts @@ -156,4 +156,82 @@ describe('MachineLearningApiClient', () => { .should.eventually.be.rejected.and.deep.equal(expected); }); }); + + describe('deleteModel', () => { + const INVALID_NAMES: any[] = [null, undefined, '', 1, true, {}, []]; + INVALID_NAMES.forEach((invalidName) => { + it(`should reject when called with: ${JSON.stringify(invalidName)}`, () => { + return apiClient.deleteModel(invalidName) + .should.eventually.be.rejected.and.have.property( + 'message', 'Model ID must be a non-empty string.'); + }); + }); + + it(`should reject when called with prefixed name`, () => { + return apiClient.deleteModel('projects/foo/rulesets/bar') + .should.eventually.be.rejected.and.have.property( + 'message', 'Model ID must not contain any "/" characters.'); + }); + + it(`should reject when project id is not available`, () => { + return clientWithoutProjectId.deleteModel(MODEL_ID) + .should.eventually.be.rejectedWith(noProjectId); + }); + + it('should resolve on success', () => { + const stub = sinon + .stub(HttpClient.prototype, 'send') + .resolves(utils.responseFrom({})); + stubs.push(stub); + return apiClient.deleteModel(MODEL_ID) + .then(() => { + expect(stub).to.have.been.calledOnce.and.calledWith({ + method: 'DELETE', + url: 'https://mlkit.googleapis.com/v1beta1/projects/test-project/models/1234567', + headers: EXPECTED_HEADERS, + }); + }); + }); + + it('should reject when a full platform error response is received', () => { + const stub = sinon + .stub(HttpClient.prototype, 'send') + .rejects(utils.errorFrom(ERROR_RESPONSE, 404)); + stubs.push(stub); + const expected = new FirebaseMachineLearningError('not-found', 'Requested entity not found'); + return apiClient.deleteModel(MODEL_ID) + .should.eventually.be.rejected.and.deep.equal(expected); + }); + + it('should reject unknown-error when error code is not present', () => { + const stub = sinon + .stub(HttpClient.prototype, 'send') + .rejects(utils.errorFrom({}, 404)); + stubs.push(stub); + const expected = new FirebaseMachineLearningError('unknown-error', 'Unknown server error: {}'); + return apiClient.deleteModel(MODEL_ID) + .should.eventually.be.rejected.and.deep.equal(expected); + }); + + it('should reject unknown-error for non-json response', () => { + const stub = sinon + .stub(HttpClient.prototype, 'send') + .rejects(utils.errorFrom('not json', 404)); + stubs.push(stub); + const expected = new FirebaseMachineLearningError( + 'unknown-error', 'Unexpected response with status: 404 and body: not json'); + return apiClient.deleteModel(MODEL_ID) + .should.eventually.be.rejected.and.deep.equal(expected); + }); + + it('should reject when rejected with a FirebaseAppError', () => { + const expected = new FirebaseAppError('network-error', 'socket hang up'); + const stub = sinon + .stub(HttpClient.prototype, 'send') + .rejects(expected); + stubs.push(stub); + return apiClient.deleteModel(MODEL_ID) + .should.eventually.be.rejected.and.deep.equal(expected); + }); + }); }); diff --git a/test/unit/machine-learning/machine-learning.spec.ts b/test/unit/machine-learning/machine-learning.spec.ts index 7652aa7944..bb0c5988f1 100644 --- a/test/unit/machine-learning/machine-learning.spec.ts +++ b/test/unit/machine-learning/machine-learning.spec.ts @@ -241,4 +241,24 @@ describe('MachineLearning', () => { }); }); }); + + describe('deleteModel', () => { + it('should propagate API errors', () => { + const stub = sinon + .stub(MachineLearningApiClient.prototype, 'deleteModel') + .rejects(EXPECTED_ERROR); + stubs.push(stub); + return machineLearning.deleteModel('1234567') + .should.eventually.be.rejected.and.deep.equal(EXPECTED_ERROR); + }); + + it('should resolve on success', () => { + const stub = sinon + .stub(MachineLearningApiClient.prototype, 'deleteModel') + .resolves({}); + stubs.push(stub); + + return machineLearning.deleteModel('1234567'); + }); + }); }); From a5b4c144eaff7a18871ba69b2a83bd11267cba92 Mon Sep 17 00:00:00 2001 From: ifielker Date: Mon, 10 Feb 2020 17:41:29 -0500 Subject: [PATCH 2/2] review comments --- test/integration/machine-learning.spec.ts | 2 -- .../machine-learning/machine-learning-api-client.spec.ts | 6 +++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/test/integration/machine-learning.spec.ts b/test/integration/machine-learning.spec.ts index e625dca562..1da8a69729 100644 --- a/test/integration/machine-learning.spec.ts +++ b/test/integration/machine-learning.spec.ts @@ -45,7 +45,5 @@ describe('admin.machineLearning', () => { .should.eventually.be.rejected.and.have.property( 'code', 'machine-learning/invalid-argument'); }); - - }); }); diff --git a/test/unit/machine-learning/machine-learning-api-client.spec.ts b/test/unit/machine-learning/machine-learning-api-client.spec.ts index 4840052f0d..a3b3935fc6 100644 --- a/test/unit/machine-learning/machine-learning-api-client.spec.ts +++ b/test/unit/machine-learning/machine-learning-api-client.spec.ts @@ -203,7 +203,7 @@ describe('MachineLearningApiClient', () => { .should.eventually.be.rejected.and.deep.equal(expected); }); - it('should reject unknown-error when error code is not present', () => { + it('should reject with unknown-error when error code is not present', () => { const stub = sinon .stub(HttpClient.prototype, 'send') .rejects(utils.errorFrom({}, 404)); @@ -213,7 +213,7 @@ describe('MachineLearningApiClient', () => { .should.eventually.be.rejected.and.deep.equal(expected); }); - it('should reject unknown-error for non-json response', () => { + it('should reject with unknown-error for non-json response', () => { const stub = sinon .stub(HttpClient.prototype, 'send') .rejects(utils.errorFrom('not json', 404)); @@ -224,7 +224,7 @@ describe('MachineLearningApiClient', () => { .should.eventually.be.rejected.and.deep.equal(expected); }); - it('should reject when rejected with a FirebaseAppError', () => { + it('should reject when failed with a FirebaseAppError', () => { const expected = new FirebaseAppError('network-error', 'socket hang up'); const stub = sinon .stub(HttpClient.prototype, 'send')