Conversation
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The
UnprocessableEntityExceptionresponses now mix plain string messages and{ message, error }objects (e.g., in dataset list validation vscheckDatasetFiles), which can make the API error shape inconsistent for clients; consider standardizing the payload format for these validation errors. - In
checkDatasetFiles,origsMappedByDatasetIdis keyed byorig.datasetIdbut later accessed usingpid(origsMappedByDatasetId[pid]), which assumesdatasetIdandpidare identical; if they differ in your schema, this lookup will silently fail and should be adjusted or made explicit. - The dataset list validation now returns
JSON.stringify(validateErrors)directly in the error message, which can be verbose and leak internal structure; consider mapping theclass-validatorerrors to a more concise, user-focused error format before including them in the response.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `UnprocessableEntityException` responses now mix plain string messages and `{ message, error }` objects (e.g., in dataset list validation vs `checkDatasetFiles`), which can make the API error shape inconsistent for clients; consider standardizing the payload format for these validation errors.
- In `checkDatasetFiles`, `origsMappedByDatasetId` is keyed by `orig.datasetId` but later accessed using `pid` (`origsMappedByDatasetId[pid]`), which assumes `datasetId` and `pid` are identical; if they differ in your schema, this lookup will silently fail and should be adjusted or made explicit.
- The dataset list validation now returns `JSON.stringify(validateErrors)` directly in the error message, which can be verbose and leak internal structure; consider mapping the `class-validator` errors to a more concise, user-focused error format before including them in the response.
## Individual Comments
### Comment 1
<location path="src/jobs/jobs.controller.utils.ts" line_range="98-99" />
<code_context>
+ DatasetListDto,
+ datasetList,
+ );
+ const nestedErrors = await Promise.all(
+ datasetListDtos.map((dto) => validate(dto)),
+ );
+ const validateErrors = nestedErrors.flat();
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Avoid serializing full validation errors into the response to prevent overly verbose and potentially sensitive output.
`ValidationError` instances can include the full input (`target`) and other metadata, so serializing them directly into the exception message makes responses noisy and may leak user data in HTTP responses/logs. Instead, derive a minimal structure from `validateErrors` (e.g. `{ property, constraints }`) or rely on Nest’s validation pipe to surface only high-level messages, rather than `JSON.stringify(validateErrors)`.
Suggested implementation:
```typescript
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({
message: 'Invalid dataset list.',
errors: minimalErrors,
});
}
```
If this endpoint is already wrapped by a global validation pipe, consider whether you still need this manual validation logic. If you do, you might want to align the error shape (`message`/`errors`) with the rest of your API error format (e.g. add an application-specific `code` or `statusCode` field) so responses are consistent.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
09017bb to
af64aed
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Refactor job validation by reducing DB calls and using class-transformer validation. Also use more specific error codes.
Depends on #2713
Tests included
Documentation
official documentation info
Summary by Sourcery
Refactor job validation logic to rely on DTO validation and more specific HTTP errors while reducing database usage for dataset and file checks.
Bug Fixes:
Enhancements:
Tests: