Add helper method to ensure benchmark folder existency#2011
Add helper method to ensure benchmark folder existency#2011ravikiranvm wants to merge 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a small benchmark service layer to ensure a provider-specific benchmark folder exists (creating it if necessary), and introduces unit tests for the new behavior. This fits into the existing benchmark feature area alongside the wizard/provider adapters.
Changes:
- Added
getBenchmarkFolderDisplayName,ensureBenchmarkFolder, andcreateBenchmarkin the benchmark service layer. - Added unit tests covering provider display name resolution, folder ensure behavior, and basic
createBenchmarkresponse shape.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/server/api/src/app/benchmark/create-benchmark.service.ts | Introduces helper(s) to resolve benchmark folder name, ensure the folder exists via flowFolderService, and build a CreateBenchmarkResponse. |
| packages/server/api/test/unit/benchmark/create-benchmark.service.test.ts | Adds unit coverage for provider handling and folder creation/reuse behavior via mocked flowFolderService. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/server/api/src/app/benchmark/create-benchmark.service.ts
Outdated
Show resolved
Hide resolved
packages/server/api/test/unit/benchmark/create-benchmark.service.test.ts
Outdated
Show resolved
Hide resolved
|
| let existingFolder = | ||
| await flowFolderService.getOneByDisplayNameCaseInsensitive({ | ||
| projectId, | ||
| displayName, | ||
| contentType: ContentType.WORKFLOW, | ||
| }); | ||
|
|
||
| if (existingFolder) { | ||
| return flowFolderService.getOneOrThrow({ | ||
| projectId, | ||
| folderId: existingFolder.id, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Okay, this was my thought.
We might not re-create benchmark folder once it was created. So I thought instead of catching error on every call, I thought we first check for existence and then try to create if not already created.
Anyways I might be wrong; I will change it
There was a problem hiding this comment.
This is being done in the create method. There is no need to make two database calls to check the same behaviour.
async create(params: UpsertParams): Promise<FolderDto> {
const { projectId, request } = params;
const requestContentType = request.contentType ?? ContentType.WORKFLOW;
const folderWithDisplayName = await this.getOneByDisplayNameCaseInsensitive(
{
projectId,
displayName: request.displayName,
contentType: requestContentType,
},
);
if (!isNil(folderWithDisplayName)) {
throw new ApplicationError({
code: ErrorCode.FOLDER_ALREADY_EXISTS,
params: { folderName: request.displayName },
});
}
| existingFolder = | ||
| await flowFolderService.getOneByDisplayNameCaseInsensitive({ | ||
| projectId, | ||
| displayName, | ||
| contentType: ContentType.WORKFLOW, | ||
| }); |
There was a problem hiding this comment.
The create method already does this call. So, if you are here, with this exception you just need to call getOneOrThrow.
There was a problem hiding this comment.
ouch I get it now :'(
Fixes OPS-3752
Additional Notes
This PR implements minimal service for create benchmark endpoint that ensures Benchmark folder exists for a given provider.