diff --git a/src/jobs/jobs.controller.utils.ts b/src/jobs/jobs.controller.utils.ts index 2e97ac106..0bc8ab3ed 100644 --- a/src/jobs/jobs.controller.utils.ts +++ b/src/jobs/jobs.controller.utils.ts @@ -43,6 +43,8 @@ import { import { toObject } from "src/config/job-config/actions/actionutils"; import { loadDatasets } from "src/config/job-config/actions/actionutils"; import { DatasetClass } from "src/datasets/schemas/dataset.schema"; +import { validate } from "class-validator"; +import { plainToInstance } from "class-transformer"; @Injectable() export class JobsControllerUtils { @@ -81,63 +83,30 @@ export class JobsControllerUtils { JobParams.DatasetList ] as Array; // check that datasetList is a non empty array - if (!Array.isArray(datasetList)) { - throw new HttpException( - { - status: HttpStatus.BAD_REQUEST, - message: "Invalid dataset list", - }, - HttpStatus.BAD_REQUEST, - ); - } - if (datasetList.length == 0) { - throw new HttpException( - { - status: HttpStatus.BAD_REQUEST, - message: "List of passed datasets is empty.", - }, - HttpStatus.BAD_REQUEST, + if (!Array.isArray(datasetList)) + throw new UnprocessableEntityException("Invalid dataset list"); + if (datasetList.length == 0) + throw new UnprocessableEntityException( + "List of passed datasets is empty.", ); - } // check that datasetList is of type DatasetListDto[] - const datasetListDtos: DatasetListDto[] = datasetList.map((item) => { - return Object.assign(new DatasetListDto(), item); - }); - const allowedKeys = [JobParams.Pid, JobParams.Files] as string[]; - for (const datasetListDto of datasetListDtos) { - const keys = Object.keys(datasetListDto); - if ( - keys.length !== 2 || - !keys.every((key) => allowedKeys.includes(key)) - ) { - throw new HttpException( - { - status: HttpStatus.BAD_REQUEST, - message: - "Dataset list is expected to contain sets of pid and files.", - }, - HttpStatus.BAD_REQUEST, - ); - } - if (typeof datasetListDto[JobParams.Pid] !== "string") { - throw new HttpException( - { - status: HttpStatus.BAD_REQUEST, - message: "In datasetList each 'pid' field should be a string.", - }, - HttpStatus.BAD_REQUEST, - ); - } - if (!Array.isArray(datasetListDto[JobParams.Files])) { - throw new HttpException( - { - status: HttpStatus.BAD_REQUEST, - message: "In datasetList each 'files' field should be an array.", - }, - HttpStatus.BAD_REQUEST, - ); - } + const datasetListDtos: DatasetListDto[] = plainToInstance( + DatasetListDto, + datasetList, + ); + const nestedErrors = await Promise.all( + datasetListDtos.map((dto) => validate(dto)), + ); + const validateErrors = nestedErrors.flat(); + if (validateErrors.length > 0) { + const minimalErrors = validateErrors.map(({ property, constraints }) => ({ + property, + constraints, + })); + throw new UnprocessableEntityException( + "Invalid dataset list. " + JSON.stringify(minimalErrors), + ); } // check that all requested pids exist @@ -152,33 +121,22 @@ export class JobsControllerUtils { * Check that the dataset pids are valid */ async checkDatasetPids(datasetList: DatasetListDto[]): Promise { - interface condition { - where: { - pid: { $in: string[] }; - }; - } - const datasetIds = datasetList.map((x) => x.pid); - const filter: condition = { + const filter: FilterQuery = { where: { pid: { $in: datasetIds }, }, + fields: ["pid"], }; - const findDatasetsById = await this.datasetsService.findAll(filter); - const findIds = findDatasetsById.map(({ pid }) => pid); - const nonExistIds = datasetIds.filter((x) => !findIds.includes(x)); + const datasets = await this.datasetsService.findAll(filter); + const findIds = new Set(datasets.map(({ pid }) => pid)); + const nonExistIds = datasetIds.filter((x) => !findIds.has(x)); - if (nonExistIds.length != 0) { - throw new HttpException( - { - status: HttpStatus.BAD_REQUEST, - message: `Datasets with pid ${nonExistIds} do not exist.`, - }, - HttpStatus.BAD_REQUEST, - ); - } - return; + if (nonExistIds.length == 0) return; + throw new UnprocessableEntityException( + `Datasets with pid ${nonExistIds} do not exist.`, + ); } /** @@ -187,74 +145,36 @@ export class JobsControllerUtils { async checkDatasetFiles(datasetList: DatasetListDto[]): Promise { const datasetsToCheck = datasetList.filter((x) => x.files.length > 0); const ids = datasetsToCheck.map((x) => x.pid); - if (ids.length > 0) { - const filter = { - fields: { - pid: true, - datasetId: true, - dataFileList: true, - }, - where: { - pid: { - $in: ids, - }, - }, - }; - // Indexing originDataBlock with pid and create set of files for each dataset - const datasets = await this.datasetsService.findAll(filter); - // Include origdatablocks - let datasetOrigDatablocks: OrigDatablock[] = []; - await Promise.all( - datasets.map(async (dataset) => { - datasetOrigDatablocks = await this.origDatablocksService.findAll({ - where: { datasetId: dataset.pid }, - }); - }), - ); - const result: Record> = datasets.reduce( - (acc: Record>, dataset) => { - // Using Set make searching more efficient - const files = datasetOrigDatablocks.reduce((acc, block) => { - block.dataFileList.forEach((file) => { - acc.add(file.path); - }); - return acc; - }, new Set()); - acc[dataset.pid] = files; - return acc; - }, - {}, - ); - // Get a list of requested files that were not found - const checkResults = datasetsToCheck.reduce( - (acc: { pid: string; nonExistFiles: string[] }[], x) => { - const pid = x.pid; - const referenceFiles = result[pid]; - const nonExistFiles = x.files.filter((f) => !referenceFiles.has(f)); - if (nonExistFiles.length > 0) { - acc.push({ pid, nonExistFiles }); - } - return acc; - }, - [], - ); - if (checkResults.length > 0) { - throw new HttpException( - { - status: HttpStatus.BAD_REQUEST, - message: "At least one requested file could not be found.", - error: JSON.stringify( - checkResults.map(({ pid, nonExistFiles }) => ({ - pid, - nonExistFiles, - })), - ), - }, - HttpStatus.BAD_REQUEST, - ); - } - } - return; + if (ids.length == 0) return; + // Indexing originDataBlock with pid and create set of files for each dataset + const datasetOrigDatablocks: OrigDatablock[] = + await this.origDatablocksService.findAll({ + where: { datasetId: { $in: ids } }, + fields: ["datasetId", "dataFileList.path"], + }); + + const origsMappedByDatasetId = datasetOrigDatablocks.reduce( + (acc, orig) => { + const set = (acc[orig.datasetId] ??= new Set()); + orig.dataFileList.forEach((file) => set.add(file.path)); + return acc; + }, + {} as Record>, + ); + // Get a list of requested files that were not found + const checkResults = datasetsToCheck + .map(({ pid, files }) => { + const referenceFiles = origsMappedByDatasetId[pid] ?? new Set(); + const nonExistFiles = files.filter((f) => !referenceFiles.has(f)); + return { pid, nonExistFiles }; + }) + .filter((result) => result.nonExistFiles.length > 0); + + if (checkResults.length == 0) return; + throw new UnprocessableEntityException({ + message: "At least one requested file could not be found.", + error: JSON.stringify(checkResults), + }); } /** diff --git a/test/Jobs.js b/test/Jobs.js index 8a6c93f06..6852543ee 100644 --- a/test/Jobs.js +++ b/test/Jobs.js @@ -253,7 +253,7 @@ describe("1110: Jobs: Test New Job Model: possible real configurations", () => { .send(newJob) .set("Accept", "application/json") .set({ Authorization: `Bearer ${accessTokenUser51}` }) - .expect(TestData.BadRequestStatusCode) + .expect(TestData.UnprocessableEntityStatusCode) .expect("Content-Type", /json/) .then((res) => { res.body.should.not.have.property("id"); diff --git a/test/JobsAll.js b/test/JobsAll.js index 11a2a2f40..7356789ca 100644 --- a/test/JobsAll.js +++ b/test/JobsAll.js @@ -174,7 +174,7 @@ describe("1120: Jobs: Test New Job Model Authorization for all_access jobs type" .send(newJob) .set("Accept", "application/json") .set({ Authorization: `Bearer ${accessTokenAdmin}` }) - .expect(TestData.BadRequestStatusCode) + .expect(TestData.UnprocessableEntityStatusCode) .expect("Content-Type", /json/) .then((res) => { res.body.should.not.have.property("id"); @@ -202,7 +202,7 @@ describe("1120: Jobs: Test New Job Model Authorization for all_access jobs type" .send(newJob) .set("Accept", "application/json") .set({ Authorization: `Bearer ${accessTokenAdmin}` }) - .expect(TestData.BadRequestStatusCode) + .expect(TestData.UnprocessableEntityStatusCode) .expect("Content-Type", /json/) .then((res) => { res.body.should.not.have.property("id"); diff --git a/test/JobsGet.js b/test/JobsGet.js index a7919db97..759f0114f 100644 --- a/test/JobsGet.js +++ b/test/JobsGet.js @@ -741,4 +741,134 @@ describe("1165: Jobs test filters and access", () => { j3.datasetDetails.should.be.an("array").to.have.lengthOf(3); }); }); + + it("0120: Create job with a datasetList item missing pid should return 422 and validation error", async () => { + return request(appUrl) + .post("/api/v4/Jobs") + .send({ + type: "dataset_access", + ownerUser: "admin", + ownerGroup: "admin", + jobParams: { + datasetList: [{ files: [] }], + }, + }) + .set("Accept", "application/json") + .auth(accessTokenAdmin, { type: "bearer" }) + .expect(TestData.UnprocessableEntityStatusCode) + .expect("Content-Type", /json/) + .then((res) => { + res.body.should.have.property("message"); + res.body.message.should.contain("Invalid dataset list."); + }); + }); + + it("0130: Create job with a datasetList item with empty pid should return 422 and validation error", async () => { + return request(appUrl) + .post("/api/v4/Jobs") + .send({ + type: "dataset_access", + ownerUser: "admin", + ownerGroup: "admin", + jobParams: { + datasetList: [{ pid: "", files: [] }], + }, + }) + .set("Accept", "application/json") + .auth(accessTokenAdmin, { type: "bearer" }) + .expect(TestData.UnprocessableEntityStatusCode) + .expect("Content-Type", /json/) + .then((res) => { + res.body.should.have.property("message"); + res.body.message.should.contain("Invalid dataset list."); + }); + }); + + it("0140: Create job with a datasetList item with non-string pid should return 422 and validation error", async () => { + return request(appUrl) + .post("/api/v4/Jobs") + .send({ + type: "dataset_access", + ownerUser: "admin", + ownerGroup: "admin", + jobParams: { + datasetList: [{ pid: 12345, files: [] }], + }, + }) + .set("Accept", "application/json") + .auth(accessTokenAdmin, { type: "bearer" }) + .expect(TestData.UnprocessableEntityStatusCode) + .expect("Content-Type", /json/) + .then((res) => { + res.body.should.have.property("message"); + res.body.message.should.contain("Invalid dataset list."); + }); + }); + + it("0150: Create job with a datasetList item with non-array files should return 422 and validation error", async () => { + return request(appUrl) + .post("/api/v4/Jobs") + .send({ + type: "dataset_access", + ownerUser: "admin", + ownerGroup: "admin", + jobParams: { + datasetList: [{ pid: datasetPid1, files: "not-an-array" }], + }, + }) + .set("Accept", "application/json") + .auth(accessTokenAdmin, { type: "bearer" }) + .expect(TestData.UnprocessableEntityStatusCode) + .expect("Content-Type", /json/) + .then((res) => { + res.body.should.have.property("message"); + res.body.message.should.contain("Invalid dataset list."); + }); + }); + + it("0160: Create job with a datasetList item with non-string entries in files array should return 422 and validation error", async () => { + return request(appUrl) + .post("/api/v4/Jobs") + .send({ + type: "dataset_access", + ownerUser: "admin", + ownerGroup: "admin", + jobParams: { + datasetList: [{ pid: datasetPid1, files: [123, 456] }], + }, + }) + .set("Accept", "application/json") + .auth(accessTokenAdmin, { type: "bearer" }) + .expect(TestData.UnprocessableEntityStatusCode) + .expect("Content-Type", /json/) + .then((res) => { + res.body.should.have.property("message"); + res.body.message.should.contain("Invalid dataset list."); + }); + }); + + it("0170: Create job with multiple datasetList items where one has an invalid pid should return 422 and validation error listing the failing property", async () => { + return request(appUrl) + .post("/api/v4/Jobs") + .send({ + type: "dataset_access", + ownerUser: "admin", + ownerGroup: "admin", + jobParams: { + datasetList: [ + { pid: datasetPid1, files: [] }, + { pid: "", files: [] }, + ], + }, + }) + .set("Accept", "application/json") + .auth(accessTokenAdmin, { type: "bearer" }) + .expect(TestData.UnprocessableEntityStatusCode) + .expect("Content-Type", /json/) + .then((res) => { + res.body.should.have.property("message"); + res.body.message.should.contain("Invalid dataset list."); + res.body.message.should.contain("pid"); + }); + }); }); diff --git a/test/JobsV3.js b/test/JobsV3.js index 86c4e0c58..9bde5dfed 100644 --- a/test/JobsV3.js +++ b/test/JobsV3.js @@ -222,7 +222,7 @@ describe("1191: Jobs: Test Backwards Compatibility", () => { .send(newJob) .set("Accept", "application/json") .set({ Authorization: `Bearer ${accessTokenAdmin}` }) - .expect(TestData.BadRequestStatusCode) + .expect(TestData.UnprocessableEntityStatusCode) .expect("Content-Type", /json/) .then((res) => { res.body.should.not.have.property("id"); @@ -246,7 +246,7 @@ describe("1191: Jobs: Test Backwards Compatibility", () => { .send(newJob) .set("Accept", "application/json") .set({ Authorization: `Bearer ${accessTokenAdmin}` }) - .expect(TestData.BadRequestStatusCode) + .expect(TestData.UnprocessableEntityStatusCode) .expect("Content-Type", /json/) .then((res) => { res.body.should.not.have.property("id"); @@ -448,7 +448,7 @@ describe("1191: Jobs: Test Backwards Compatibility", () => { .send(jobCreateDtoForUser51) .set("Accept", "application/json") .set({ Authorization: `Bearer ${accessTokenAdmin}` }) - .expect(TestData.BadRequestStatusCode) + .expect(TestData.UnprocessableEntityStatusCode) .expect("Content-Type", /json/) .then((res) => { res.body.should.not.have.property("id");