Skip to content

Commit 4a8ae72

Browse files
minotticminottic
committed
refactor: simplify job validation, reduce db calls
1 parent 6a05e53 commit 4a8ae72

5 files changed

Lines changed: 198 additions & 148 deletions

File tree

‎src/jobs/jobs.controller.utils.ts‎

Lines changed: 62 additions & 142 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,8 @@ import {
4343
import { toObject } from "src/config/job-config/actions/actionutils";
4444
import { loadDatasets } from "src/config/job-config/actions/actionutils";
4545
import { DatasetClass } from "src/datasets/schemas/dataset.schema";
46+
import { validate } from "class-validator";
47+
import { plainToInstance } from "class-transformer";
4648

4749
@Injectable()
4850
export class JobsControllerUtils {
@@ -81,63 +83,30 @@ export class JobsControllerUtils {
8183
JobParams.DatasetList
8284
] as Array<DatasetListDto>;
8385
// check that datasetList is a non empty array
84-
if (!Array.isArray(datasetList)) {
85-
throw new HttpException(
86-
{
87-
status: HttpStatus.BAD_REQUEST,
88-
message: "Invalid dataset list",
89-
},
90-
HttpStatus.BAD_REQUEST,
91-
);
92-
}
93-
if (datasetList.length == 0) {
94-
throw new HttpException(
95-
{
96-
status: HttpStatus.BAD_REQUEST,
97-
message: "List of passed datasets is empty.",
98-
},
99-
HttpStatus.BAD_REQUEST,
86+
if (!Array.isArray(datasetList))
87+
throw new UnprocessableEntityException("Invalid dataset list");
88+
if (datasetList.length == 0)
89+
throw new UnprocessableEntityException(
90+
"List of passed datasets is empty.",
10091
);
101-
}
10292

10393
// check that datasetList is of type DatasetListDto[]
104-
const datasetListDtos: DatasetListDto[] = datasetList.map((item) => {
105-
return Object.assign(new DatasetListDto(), item);
106-
});
107-
const allowedKeys = [JobParams.Pid, JobParams.Files] as string[];
108-
for (const datasetListDto of datasetListDtos) {
109-
const keys = Object.keys(datasetListDto);
110-
if (
111-
keys.length !== 2 ||
112-
!keys.every((key) => allowedKeys.includes(key))
113-
) {
114-
throw new HttpException(
115-
{
116-
status: HttpStatus.BAD_REQUEST,
117-
message:
118-
"Dataset list is expected to contain sets of pid and files.",
119-
},
120-
HttpStatus.BAD_REQUEST,
121-
);
122-
}
123-
if (typeof datasetListDto[JobParams.Pid] !== "string") {
124-
throw new HttpException(
125-
{
126-
status: HttpStatus.BAD_REQUEST,
127-
message: "In datasetList each 'pid' field should be a string.",
128-
},
129-
HttpStatus.BAD_REQUEST,
130-
);
131-
}
132-
if (!Array.isArray(datasetListDto[JobParams.Files])) {
133-
throw new HttpException(
134-
{
135-
status: HttpStatus.BAD_REQUEST,
136-
message: "In datasetList each 'files' field should be an array.",
137-
},
138-
HttpStatus.BAD_REQUEST,
139-
);
140-
}
94+
const datasetListDtos: DatasetListDto[] = plainToInstance(
95+
DatasetListDto,
96+
datasetList,
97+
);
98+
const nestedErrors = await Promise.all(
99+
datasetListDtos.map((dto) => validate(dto)),
100+
);
101+
const validateErrors = nestedErrors.flat();
102+
if (validateErrors.length > 0) {
103+
const minimalErrors = validateErrors.map(({ property, constraints }) => ({
104+
property,
105+
constraints,
106+
}));
107+
throw new UnprocessableEntityException(
108+
"Invalid dataset list. " + JSON.stringify(minimalErrors),
109+
);
141110
}
142111

143112
// check that all requested pids exist
@@ -152,33 +121,22 @@ export class JobsControllerUtils {
152121
* Check that the dataset pids are valid
153122
*/
154123
async checkDatasetPids(datasetList: DatasetListDto[]): Promise<void> {
155-
interface condition {
156-
where: {
157-
pid: { $in: string[] };
158-
};
159-
}
160-
161124
const datasetIds = datasetList.map((x) => x.pid);
162-
const filter: condition = {
125+
const filter: FilterQuery<DatasetClass> = {
163126
where: {
164127
pid: { $in: datasetIds },
165128
},
129+
fields: ["pid"],
166130
};
167131

168-
const findDatasetsById = await this.datasetsService.findAll(filter);
169-
const findIds = findDatasetsById.map(({ pid }) => pid);
170-
const nonExistIds = datasetIds.filter((x) => !findIds.includes(x));
132+
const datasets = await this.datasetsService.findAll(filter);
133+
const findIds = new Set(datasets.map(({ pid }) => pid));
134+
const nonExistIds = datasetIds.filter((x) => !findIds.has(x));
171135

172-
if (nonExistIds.length != 0) {
173-
throw new HttpException(
174-
{
175-
status: HttpStatus.BAD_REQUEST,
176-
message: `Datasets with pid ${nonExistIds} do not exist.`,
177-
},
178-
HttpStatus.BAD_REQUEST,
179-
);
180-
}
181-
return;
136+
if (nonExistIds.length == 0) return;
137+
throw new UnprocessableEntityException(
138+
`Datasets with pid ${nonExistIds} do not exist.`,
139+
);
182140
}
183141

184142
/**
@@ -187,74 +145,36 @@ export class JobsControllerUtils {
187145
async checkDatasetFiles(datasetList: DatasetListDto[]): Promise<void> {
188146
const datasetsToCheck = datasetList.filter((x) => x.files.length > 0);
189147
const ids = datasetsToCheck.map((x) => x.pid);
190-
if (ids.length > 0) {
191-
const filter = {
192-
fields: {
193-
pid: true,
194-
datasetId: true,
195-
dataFileList: true,
196-
},
197-
where: {
198-
pid: {
199-
$in: ids,
200-
},
201-
},
202-
};
203-
// Indexing originDataBlock with pid and create set of files for each dataset
204-
const datasets = await this.datasetsService.findAll(filter);
205-
// Include origdatablocks
206-
let datasetOrigDatablocks: OrigDatablock[] = [];
207-
await Promise.all(
208-
datasets.map(async (dataset) => {
209-
datasetOrigDatablocks = await this.origDatablocksService.findAll({
210-
where: { datasetId: dataset.pid },
211-
});
212-
}),
213-
);
214-
const result: Record<string, Set<string>> = datasets.reduce(
215-
(acc: Record<string, Set<string>>, dataset) => {
216-
// Using Set make searching more efficient
217-
const files = datasetOrigDatablocks.reduce((acc, block) => {
218-
block.dataFileList.forEach((file) => {
219-
acc.add(file.path);
220-
});
221-
return acc;
222-
}, new Set<string>());
223-
acc[dataset.pid] = files;
224-
return acc;
225-
},
226-
{},
227-
);
228-
// Get a list of requested files that were not found
229-
const checkResults = datasetsToCheck.reduce(
230-
(acc: { pid: string; nonExistFiles: string[] }[], x) => {
231-
const pid = x.pid;
232-
const referenceFiles = result[pid];
233-
const nonExistFiles = x.files.filter((f) => !referenceFiles.has(f));
234-
if (nonExistFiles.length > 0) {
235-
acc.push({ pid, nonExistFiles });
236-
}
237-
return acc;
238-
},
239-
[],
240-
);
241-
if (checkResults.length > 0) {
242-
throw new HttpException(
243-
{
244-
status: HttpStatus.BAD_REQUEST,
245-
message: "At least one requested file could not be found.",
246-
error: JSON.stringify(
247-
checkResults.map(({ pid, nonExistFiles }) => ({
248-
pid,
249-
nonExistFiles,
250-
})),
251-
),
252-
},
253-
HttpStatus.BAD_REQUEST,
254-
);
255-
}
256-
}
257-
return;
148+
if (ids.length == 0) return;
149+
// Indexing originDataBlock with pid and create set of files for each dataset
150+
const datasetOrigDatablocks: OrigDatablock[] =
151+
await this.origDatablocksService.findAll({
152+
where: { datasetId: { $in: ids } },
153+
fields: ["datasetId", "dataFileList.path"],
154+
});
155+
156+
const origsMappedByDatasetId = datasetOrigDatablocks.reduce(
157+
(acc, orig) => {
158+
const set = (acc[orig.datasetId] ??= new Set<string>());
159+
orig.dataFileList.forEach((file) => set.add(file.path));
160+
return acc;
161+
},
162+
{} as Record<string, Set<string>>,
163+
);
164+
// Get a list of requested files that were not found
165+
const checkResults = datasetsToCheck
166+
.map(({ pid, files }) => {
167+
const referenceFiles = origsMappedByDatasetId[pid] ?? new Set<string>();
168+
const nonExistFiles = files.filter((f) => !referenceFiles.has(f));
169+
return { pid, nonExistFiles };
170+
})
171+
.filter((result) => result.nonExistFiles.length > 0);
172+
173+
if (checkResults.length == 0) return;
174+
throw new UnprocessableEntityException({
175+
message: "At least one requested file could not be found.",
176+
error: JSON.stringify(checkResults),
177+
});
258178
}
259179

260180
/**

‎test/Jobs.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -253,7 +253,7 @@ describe("1110: Jobs: Test New Job Model: possible real configurations", () => {
253253
.send(newJob)
254254
.set("Accept", "application/json")
255255
.set({ Authorization: `Bearer ${accessTokenUser51}` })
256-
.expect(TestData.BadRequestStatusCode)
256+
.expect(TestData.UnprocessableEntityStatusCode)
257257
.expect("Content-Type", /json/)
258258
.then((res) => {
259259
res.body.should.not.have.property("id");

‎test/JobsAll.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ describe("1120: Jobs: Test New Job Model Authorization for all_access jobs type"
174174
.send(newJob)
175175
.set("Accept", "application/json")
176176
.set({ Authorization: `Bearer ${accessTokenAdmin}` })
177-
.expect(TestData.BadRequestStatusCode)
177+
.expect(TestData.UnprocessableEntityStatusCode)
178178
.expect("Content-Type", /json/)
179179
.then((res) => {
180180
res.body.should.not.have.property("id");
@@ -202,7 +202,7 @@ describe("1120: Jobs: Test New Job Model Authorization for all_access jobs type"
202202
.send(newJob)
203203
.set("Accept", "application/json")
204204
.set({ Authorization: `Bearer ${accessTokenAdmin}` })
205-
.expect(TestData.BadRequestStatusCode)
205+
.expect(TestData.UnprocessableEntityStatusCode)
206206
.expect("Content-Type", /json/)
207207
.then((res) => {
208208
res.body.should.not.have.property("id");

0 commit comments

Comments
 (0)