diff --git a/src/api/general-services/pipeline-manage/index.js b/src/api/general-services/pipeline-manage/index.js index 6876ed594..b50784d3d 100644 --- a/src/api/general-services/pipeline-manage/index.js +++ b/src/api/general-services/pipeline-manage/index.js @@ -164,6 +164,8 @@ const createQCPipeline = async (experimentId, processingConfigUpdates) => { const { samples } = await samplesService.getSamplesByExperimentId(experimentId); + const sampleIds = Object.keys(samples); + if (processingConfigUpdates) { processingConfigUpdates.forEach(({ name, body }) => { if (!processingConfig[name]) { @@ -182,14 +184,14 @@ const createQCPipeline = async (experimentId, processingConfigUpdates) => { const mergedProcessingConfig = _.cloneDeepWith(processingConfig, (o) => { if (_.isObject(o) && !o.dataIntegration && !o.embeddingSettings && typeof o.enabled === 'boolean') { // Find which samples have sample-specific configurations. - const sampleConfigs = _.intersection(Object.keys(o), samples.ids); + const sampleConfigs = _.intersection(Object.keys(o), sampleIds); // Get an object that is only the "raw" configuration. const rawConfig = _.omit(o, sampleConfigs); const result = {}; - samples.ids.forEach((sample) => { + sampleIds.forEach((sample) => { result[sample] = _.merge({}, rawConfig, o[sample]); }); @@ -219,7 +221,7 @@ const createQCPipeline = async (experimentId, processingConfigUpdates) => { logger.log(`State machine with ARN ${stateMachineArn} created, launching it...`); const execInput = { - samples: samples.ids.map((sampleUuid, index) => ({ sampleUuid, index })), + samples: sampleIds.map((sampleUuid, index) => ({ sampleUuid, index })), }; const executionArn = await executeStateMachine(stateMachineArn, execInput); @@ -237,14 +239,15 @@ const createGem2SPipeline = async (experimentId) => { const { metadataKeys } = await projectService.getProject(experiment.projectId); const defaultMetadataValue = 'N.A.'; + const samplesEntries = Object.entries(samples); const taskParams = { projectId: experiment.projectId, experimentName: experiment.experimentName, organism: experiment.meta.organism, input: { type: experiment.meta.type }, - sampleIds: samples.ids, - sampleNames: samples.ids.map((id) => samples[id].name), + sampleIds: samplesEntries.map(([sampleId]) => sampleId), + sampleNames: samplesEntries.map(([, sample]) => sample.name), }; if (metadataKeys.length) { @@ -252,9 +255,9 @@ const createGem2SPipeline = async (experimentId) => { // Make sure the key does not contain '-' as it will cause failure in GEM2S const sanitizedKey = key.replace(/-+/g, '_'); - acc[sanitizedKey] = samples.ids.map( - // Fetch using unsanitized key as it is the key used to store metadata in sample - (sampleUuid) => samples[sampleUuid].metadata[key] || defaultMetadataValue, + // Fetch using unsanitized key as it is the key used to store metadata in sample + acc[sanitizedKey] = samplesEntries.map( + ([, sample]) => sample.metadata[key] || defaultMetadataValue, ); return acc; }, {}); diff --git a/src/api/route-services/__mocks__/samples.js b/src/api/route-services/__mocks__/samples.js index 294c0636b..dcd44dd63 100644 --- a/src/api/route-services/__mocks__/samples.js +++ b/src/api/route-services/__mocks__/samples.js @@ -1,7 +1,6 @@ const { OK } = require('../../../utils/responses'); const mockSamples = { - ids: ['sample-1'], 'sample-1': { name: 'sample-1', projectUuid: 'project-1', @@ -75,7 +74,7 @@ const mock = jest.fn().mockImplementation(() => ({ getSamples: mockGetSamples, getSamplesByExperimentId: mockGetByExperimentId, updateSamples: mockUpdateSamples, - deleteSamples: mockDeleteSamples, + deleteSamplesEntry: mockDeleteSamples, })); module.exports = mock; diff --git a/src/api/route-services/projects.js b/src/api/route-services/projects.js index e4fa4b261..68253ac12 100644 --- a/src/api/route-services/projects.js +++ b/src/api/route-services/projects.js @@ -191,12 +191,12 @@ class ProjectsService { const dynamodb = createDynamoDbInstance(); try { - const { experiments } = await this.getProject(projectUuid); + const { experiments, samples: sampleUuids } = await this.getProject(projectUuid); if (experiments.length > 0) { const deletePromises = experiments.reduce((acc, experimentId) => { acc.push(experimentService.deleteExperiment(experimentId)); - acc.push(samplesService.deleteSamples(projectUuid, experimentId)); + acc.push(samplesService.deleteSamplesEntry(projectUuid, experimentId, sampleUuids)); return acc; }, []); diff --git a/src/api/route-services/samples.js b/src/api/route-services/samples.js index 3bf5a3038..122a7a372 100644 --- a/src/api/route-services/samples.js +++ b/src/api/route-services/samples.js @@ -1,15 +1,21 @@ +const _ = require('lodash'); + const { NotFoundError, OK } = require('../../utils/responses'); +const { undefinedIfNotFound } = require('./utils'); + const config = require('../../config'); const { createDynamoDbInstance, convertToJsObject, convertToDynamoDbRecord, } = require('../../utils/dynamoDb'); -const logger = require('../../utils/logging'); +const AWS = require('../../utils/requireAWS'); +const logger = require('../../utils/logging'); class SamplesService { constructor() { this.tableName = `samples-${config.clusterEnv}`; + this.sampleFilesBucketName = `biomage-originals-${config.clusterEnv}`; } async getSamples(projectUuid) { @@ -33,7 +39,14 @@ class SamplesService { throw new NotFoundError('Samples not found!'); } - return items.map((item) => convertToJsObject(item)); + return items.map((item) => { + const prettyItem = convertToJsObject(item); + + // Remove ids property from old sample entries that still have it + delete prettyItem.samples.ids; + + return prettyItem; + }); } @@ -54,6 +67,9 @@ class SamplesService { if (response.Item) { const prettyResponse = convertToJsObject(response.Item); + + // Remove ids property from old sample entries that still have it + delete prettyResponse.samples.ids; return prettyResponse; } @@ -72,7 +88,6 @@ class SamplesService { ':projectUuid': projectUuid, }); - // Update samples const params = { TableName: this.tableName, @@ -92,29 +107,102 @@ class SamplesService { } } - async deleteSamples(projectUuid, experimentId) { - logger.log(`Deleting sample for project ${projectUuid} and expId ${experimentId}`); + async deleteSamplesFromS3(projectUuid, samplesToRemoveUuids, allSamples) { + const s3 = new AWS.S3(); + + const sampleObjectsToDelete = samplesToRemoveUuids.map((sampleUuid) => { + const fileKeysToDelete = Object.keys(allSamples[sampleUuid].files); + + return fileKeysToDelete.map((fileKey) => ({ Key: `${projectUuid}/${sampleUuid}/${fileKey}` })); + }); + + const s3Params = { + Bucket: this.sampleFilesBucketName, + Delete: { + Objects: _.flatten(sampleObjectsToDelete), + Quiet: false, + }, + }; + + const result = await s3.deleteObjects(s3Params).promise(); + + if (result.Errors.length) { + throw Error(`Delete S3 object errors: ${JSON.stringify(result.Errors)}`); + } + } + + async removeSamples(projectUuid, experimentId, sampleUuids) { + logger.log(`Removing samples in an entry for project ${projectUuid} and expId ${experimentId}`); const marshalledKey = convertToDynamoDbRecord({ experimentId, }); + const updateExpressionList = sampleUuids.map((sampleUuid, index) => `samples.#val${index}`); + const expressionAttributeNames = sampleUuids.reduce((acc, sampleId, index) => { + acc[`#val${index}`] = sampleId; + return acc; + }, {}); + const params = { TableName: this.tableName, Key: marshalledKey, + UpdateExpression: `REMOVE ${updateExpressionList.join(', ')}`, + ExpressionAttributeNames: expressionAttributeNames, + ReturnValues: 'ALL_NEW', + }; + + const a = await undefinedIfNotFound( + this.getSamplesByExperimentId(experimentId), + ) || {}; + + const { samples: allSamples = {} } = a; + + const promises = [ + createDynamoDbInstance().updateItem(params).promise(), + this.deleteSamplesFromS3(projectUuid, sampleUuids, allSamples), + ]; + + await Promise.all(promises); + + return OK(); + } + + async deleteSamplesEntry(projectUuid, experimentId, sampleUuids) { + logger.log(`Deleting samples entry for project ${projectUuid} and expId ${experimentId}`); + + const { samples: allSamples = {} } = await undefinedIfNotFound( + this.getSamplesByExperimentId(experimentId), + ) || {}; + + const marshalledKey = convertToDynamoDbRecord({ + experimentId, + }); + + const dynamodbParams = { + TableName: this.tableName, + Key: marshalledKey, }; const dynamodb = createDynamoDbInstance(); + const promises = []; + try { - await dynamodb.deleteItem(params).send(); - return OK(); + promises.push(await dynamodb.deleteItem(dynamodbParams).send()); } catch (e) { if (e.statusCode === 404) throw NotFoundError('Project not found'); throw e; } + + if (sampleUuids.length) { + promises.push(await this.deleteSamplesFromS3(projectUuid, sampleUuids, allSamples)); + } + + await Promise.all(promises); + + return OK(); } } - module.exports = SamplesService; diff --git a/src/api/route-services/utils.js b/src/api/route-services/utils.js new file mode 100644 index 000000000..a57fd7040 --- /dev/null +++ b/src/api/route-services/utils.js @@ -0,0 +1,32 @@ +// Updates each sub attribute separately for +// one particular attribute (of type object) of a dynamodb entry + +const { NotFoundError } = require('../../utils/responses'); + +const removePropertiesFromObject = async ( + entryKey, attributeName, propertyToRemove, + tableName, dynamodb, +) => { + const params = { + TableName: tableName, + Key: entryKey, + UpdateExpression: `REMOVE ${attributeName}.${propertyToRemove}`, + ReturnValues: 'ALL_NEW', + }; + + await dynamodb.updateItem(params).promise(); +}; + +const undefinedIfNotFound = async (promise) => { + try { + return await promise; + } catch (e) { + if (e instanceof NotFoundError) { + return undefined; + } + + throw e; + } +}; + +module.exports = { removePropertiesFromObject, undefinedIfNotFound }; diff --git a/src/api/routes/samples.js b/src/api/routes/samples.js index 896cba653..e55a1da1c 100644 --- a/src/api/routes/samples.js +++ b/src/api/routes/samples.js @@ -15,8 +15,16 @@ module.exports = { }, 'samples#update': (req, res, next) => { const { body, params: { projectUuid } } = req; + samplesService.updateSamples(projectUuid, body) .then((data) => res.json(data)) .catch(next); }, + 'samples#remove': (req, res, next) => { + const { body: { ids }, params: { projectUuid, experimentId } } = req; + + samplesService.removeSamples(projectUuid, experimentId, ids) + .then((data) => res.json(data)) + .catch(next); + }, }; diff --git a/src/specs/api.yaml b/src/specs/api.yaml index 6c09ad7f7..244af64cd 100644 --- a/src/specs/api.yaml +++ b/src/specs/api.yaml @@ -806,6 +806,7 @@ paths: type: string get: summary: Get samples under a project + description: Get all samples under a project operationId: getProjectSamples x-eov-operation-id: samples#get x-eov-operation-handler: routes/samples @@ -831,7 +832,6 @@ paths: application/json: schema: $ref: ./models/HTTPError.v1.yaml - description: Get all samples under a project '/projects/{projectUuid}/{experimentId}/samples': parameters: - name: projectUuid @@ -845,8 +845,8 @@ paths: schema: type: string put: - summary: Update sample - operationId: updateSample + summary: Update samples + operationId: updateSamples x-eov-operation-id: samples#update x-eov-operation-handler: routes/samples responses: @@ -874,19 +874,51 @@ paths: application/json: schema: $ref: ./models/HTTPError.v1.yaml - description: Updates a sample + description: Updates a samples entry requestBody: content: application/json: schema: type: object properties: - projectUuid: - type: string - experimentId: - type: string samples: $ref: ./models/api-body-schemas/Samples.v1.yaml + delete: + summary: Remove samples + operationId: removeSamples + x-eov-operation-id: samples#remove + x-eov-operation-handler: routes/samples + responses: + '200': + description: OK + content: + application/json: + schema: + $ref: ./models/HTTPSuccess.v1.yaml + '400': + description: Bad Request + content: + application/json: + schema: + $ref: ./models/HTTPError.v1.yaml + '404': + description: Not Found + content: + application/json: + schema: + $ref: ./models/HTTPError.v1.yaml + '415': + description: Unsupported Media Type + content: + application/json: + schema: + $ref: ./models/HTTPError.v1.yaml + description: Removes samples from an entry + requestBody: + content: + application/json: + schema: + $ref: ./models/samples-bodies/SamplesRemoveBody.v1.yaml '/projects': get: summary: 'Get the list of projects' diff --git a/src/specs/models/api-body-schemas/Samples.v1.yaml b/src/specs/models/api-body-schemas/Samples.v1.yaml index 50c8e88f9..64244d169 100644 --- a/src/specs/models/api-body-schemas/Samples.v1.yaml +++ b/src/specs/models/api-body-schemas/Samples.v1.yaml @@ -1,37 +1,5 @@ title: Sample object description: Schema for a sample object type: object -properties: - ids: - type: array - items: - type: string additionalProperties: - type: object - properties: - name: - type: string - projectUuid: - type: string - uuid: - type: string - species: - nullable: true - type: string - createdDate: - type: string - lastModified: - nullable: true - type: string - complete: - type: boolean - error: - type: boolean - fileNames: - type: array - items: - type: string - files: - type: object -required: - - ids \ No newline at end of file + $ref: ../samples-bodies/Sample.v1.yaml \ No newline at end of file diff --git a/src/specs/models/samples-bodies/Sample.v1.yaml b/src/specs/models/samples-bodies/Sample.v1.yaml new file mode 100644 index 000000000..41feafcd7 --- /dev/null +++ b/src/specs/models/samples-bodies/Sample.v1.yaml @@ -0,0 +1,28 @@ +title: Sample +description: A single sample +type: object +properties: + name: + type: string + projectUuid: + type: string + uuid: + type: string + species: + nullable: true + type: string + createdDate: + type: string + lastModified: + nullable: true + type: string + complete: + type: boolean + error: + type: boolean + fileNames: + type: array + items: + type: string + files: + type: object \ No newline at end of file diff --git a/src/specs/models/samples-bodies/SamplesRemoveBody.v1.yaml b/src/specs/models/samples-bodies/SamplesRemoveBody.v1.yaml new file mode 100644 index 000000000..bc6d61729 --- /dev/null +++ b/src/specs/models/samples-bodies/SamplesRemoveBody.v1.yaml @@ -0,0 +1,11 @@ +title: Remove Samples Body +description: A wrapper object for the array of sample ids to be removed +type: object +properties: + ids: + description: An array of sample ids specifying which samples to delete + type: array + items: + type: string +required: +- ids \ No newline at end of file diff --git a/tests/api/event-services/work-request.test.js b/tests/api/event-services/work-request.test.js index f1cca58d1..8dc873a93 100644 --- a/tests/api/event-services/work-request.test.js +++ b/tests/api/event-services/work-request.test.js @@ -155,8 +155,6 @@ describe('handleWorkRequest', () => { try { await handleWorkRequest(workRequest, socket); } catch (e) { - console.log(e.message); - expect(e.message).toMatch( 'Work request can not be handled because pipeline is RUNNING', ); diff --git a/tests/api/general-services/pipeline-manage.test.js b/tests/api/general-services/pipeline-manage.test.js index e2490ea7f..63ad55546 100644 --- a/tests/api/general-services/pipeline-manage.test.js +++ b/tests/api/general-services/pipeline-manage.test.js @@ -8,38 +8,28 @@ jest.mock('crypto', () => ({ })); jest.mock('../../../src/utils/asyncTimer'); -const { createQCPipeline } = jest.requireActual('../../../src/api/general-services/pipeline-manage'); - -describe('test for pipeline services', () => { - afterEach(() => { - AWSMock.restore('EKS'); - AWSMock.restore('StepFunctions'); - }); - - - const MockProcessingConfig = { - Item: { - processingConfig: { - M: { - doubletScores: { - M: { - enabled: { - BOOL: true, - }, - filterSettings: { - M: { - oneSetting: { - N: 1, - }, +const MockProcessingConfig = { + Item: { + processingConfig: { + M: { + doubletScores: { + M: { + enabled: { + BOOL: true, + }, + filterSettings: { + M: { + oneSetting: { + N: 1, }, }, - oneSample: { - M: { - filterSettings: { - M: { - oneSetting: { - N: 1, - }, + }, + oneSample: { + M: { + filterSettings: { + M: { + oneSetting: { + N: 1, }, }, }, @@ -49,23 +39,39 @@ describe('test for pipeline services', () => { }, }, }, - }; + }, +}; - const MockSamples = { - Item: { - samples: { - M: { - ids: { - L: [ - { S: 'oneSample' }, - { S: 'otherSample' }, - ], +const MockSamples = { + Item: { + samples: { + M: { + oneSample: { + M: { + uuid: { + S: 'oneSample', + }, + }, + }, + otherSample: { + M: { + uuid: { + S: 'otherSample', + }, }, }, }, }, + }, +}; - }; +const { createQCPipeline } = jest.requireActual('../../../src/api/general-services/pipeline-manage'); + +describe('test for pipeline services', () => { + afterEach(() => { + AWSMock.restore('EKS'); + AWSMock.restore('StepFunctions'); + }); const mockCluster = { cluster: { @@ -196,10 +202,10 @@ describe('test for pipeline services', () => { AWSMock.mock('DynamoDB', 'getItem', (params, callback) => { if (params.TableName.match('experiments')) { getProcessingConfigSpy(params); - callback(null, _.cloneDeep(MockProcessingConfig)); + callback(null, MockProcessingConfig); } else if (params.TableName.match('samples')) { getSamplesSpy(params); - callback(null, _.cloneDeep(MockSamples)); + callback(null, MockSamples); } }); diff --git a/tests/api/route-services/projects.test.js b/tests/api/route-services/projects.test.js index adc369752..38baa9761 100644 --- a/tests/api/route-services/projects.test.js +++ b/tests/api/route-services/projects.test.js @@ -252,13 +252,14 @@ describe('tests for the projects service', () => { it('DeleteProject deletes project and samples properly', async (done) => { const experiments = ['project-1']; + const samples = []; const marshalledKey = AWS.DynamoDB.Converter.marshall({ projectUuid: 'project-1', }); const deleteSpy = mockDynamoDeleteItem(); - const getSpy = mockDynamoGetItem({ projects: { experiments } }); + const getSpy = mockDynamoGetItem({ projects: { experiments, samples }, samples }); (new ProjectsService()).deleteProject('project-1') .then((res) => { diff --git a/tests/api/route-services/samples.test.js b/tests/api/route-services/samples.test.js index 35affc2dd..ae930fecc 100644 --- a/tests/api/route-services/samples.test.js +++ b/tests/api/route-services/samples.test.js @@ -8,6 +8,7 @@ const { mockDynamoQuery, mockDynamoUpdateItem, mockDynamoDeleteItem, + mockS3DeleteObjects, } = require('../../test-utils/mockAWSServices'); describe('tests for the samples service', () => { @@ -19,7 +20,6 @@ describe('tests for the samples service', () => { it('Get samples by projectUuid works', async (done) => { const jsData = { samples: { - ids: ['sample-1'], 'sample-1': { name: 'sample-1', }, @@ -48,7 +48,6 @@ describe('tests for the samples service', () => { it('Get sample by experimentId works', async (done) => { const jsData = { samples: { - ids: ['sample-1', 'sample-2'], 'sample-1': { name: 'sample-1' }, 'sample-2': { name: 'sample-2' }, }, @@ -77,7 +76,6 @@ describe('tests for the samples service', () => { projectUuid: 'project-1', experimentId: 'experiment-1', samples: { - ids: ['sample-1', 'sample-2'], 'sample-1': { name: 'sample-1' }, 'sample-2': { name: 'sample-2' }, }, @@ -112,15 +110,42 @@ describe('tests for the samples service', () => { experimentId: 'experiment-1', }); - const getFnSpy = mockDynamoDeleteItem(); + const deleteDynamoFnSpy = mockDynamoDeleteItem(); - (new SamplesService()).deleteSamples('project-1', 'experiment-1') + mockDynamoGetItem({ + samples: { + 'sampleUuid-1': { + files: { + 'barcodes.tsv.gz': {}, + 'features.tsv.gz': {}, + 'matrix.mtx.gz': {}, + }, + }, + }, + }); + + const deleteS3FnSpy = mockS3DeleteObjects({ Errors: [] }); + + const s3DeleteParams = { + Bucket: 'biomage-originals-test', + Delete: { + Objects: [ + { Key: 'project-1/sampleUuid-1/barcodes.tsv.gz' }, + { Key: 'project-1/sampleUuid-1/features.tsv.gz' }, + { Key: 'project-1/sampleUuid-1/matrix.mtx.gz' }, + ], + Quiet: false, + }, + }; + + (new SamplesService()).deleteSamplesEntry('project-1', 'experiment-1', ['sampleUuid-1'], {}) .then((data) => { expect(data).toEqual(OK()); - expect(getFnSpy).toHaveBeenCalledWith({ + expect(deleteDynamoFnSpy).toHaveBeenCalledWith({ TableName: 'samples-test', Key: marshalledKey, }); + expect(deleteS3FnSpy).toHaveBeenCalledWith(s3DeleteParams); }) .then(() => done()); }); diff --git a/tests/api/routes/samples.test.js b/tests/api/routes/samples.test.js index ee8363565..54f2aa627 100644 --- a/tests/api/routes/samples.test.js +++ b/tests/api/routes/samples.test.js @@ -46,7 +46,6 @@ describe('tests for samples route', () => { projectUuid: 'project-uuid', experimentId: 'experiment-id', samples: { - ids: ['sample-1'], 'sample-1': { name: 'sample-1', }, diff --git a/tests/test-utils/mockAWSServices.js b/tests/test-utils/mockAWSServices.js index a7b2e9af6..a849658ed 100644 --- a/tests/test-utils/mockAWSServices.js +++ b/tests/test-utils/mockAWSServices.js @@ -116,6 +116,18 @@ const mockS3PutObject = (payload = {}, error = null) => { return fnSpy; }; +const mockS3DeleteObjects = (payload = {}, error = null) => { + const fnSpy = jest.fn((x) => x); + AWSMock.setSDKInstance(AWS); + + AWSMock.mock('S3', 'deleteObjects', (params, callback) => { + fnSpy(params); + callback(error, payload); + }); + + return fnSpy; +}; + module.exports = { mockDynamoGetItem, mockDynamoBatchGetItem, @@ -125,4 +137,5 @@ module.exports = { mockDynamoDeleteItem, mockS3GetObject, mockS3PutObject, + mockS3DeleteObjects, };