fix: improve validation error details in validateCommonProjectManifest #2966
Conversation
Replace TODO comment with detailed error formatting for validation failures. Error messages now include property names and constraint details instead of generic toString() output, making it easier to identify validation issues.
WalkthroughValidation error formatting was changed to recursively format class-validator Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
packages/**/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧬 Code graph analysis (1)packages/common/src/project/versioned/base.ts (1)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/common/src/project/load.ts (1)
94-98: Excellent improvement to error messaging.The structured error formatting is much more actionable than generic
toString()output. The defensive check for constraints and clear property-level formatting will help developers quickly identify validation issues.One optional enhancement:
ValidationErrorobjects can have nested errors via thechildrenproperty for complex object validations. If manifest structures become more deeply nested in the future, consider recursively handling these children.Example recursive approach if nested validation becomes relevant:
const formatError = (error: ValidationError, indent = ' '): string => { const property = error.property; const constraints = error.constraints ? Object.values(error.constraints).join(', ') : ''; let message = `${indent}- ${property}${constraints ? ': ' + constraints : ''}`; if (error.children?.length) { const childMessages = error.children.map(child => formatError(child, indent + ' ')).join('\n'); message += '\n' + childMessages; } return message; }; const errorMsgs = errors.map(error => formatError(error)).join('\n');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/common/src/project/load.ts(2 hunks)
🔇 Additional comments (1)
packages/common/src/project/load.ts (1)
8-8: Good addition for type safety.Adding the
ValidationErrortype import enables proper type annotation in the error formatting logic, improving code maintainability and IDE support.
|
Hi @Lil-Duckling-22, thanks for your PR. Can you please update tests and provide an example of how the error output has changed |
|
@stwiname sure thing, here are the tests |
|
How do the errors look for an error in a child object? e.g. An invalid filter on a mapping handler |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/common/src/project/versioned/base.ts (1)
54-83: Private ValidationError formatter is correct but duplicates utils.ts logicThe private
formatValidationErrorsmirrors the utils.ts helper: it builds dotted paths, uses[index]for numericpropertyvalues, and walkschildrenrecursively. This is correct for class-validator’s nested error trees and will produce paths likedataSources[0].mapping.handlers[1].filter.However, this is now duplicated with
formatValidationErrorsinpackages/common/src/project/utils.ts. Consider centralizing this formatter (e.g., exporting it from utils.ts or a dedicated validation utility module) and reusing it here to keep error formatting consistent and reduce maintenance overhead if the format ever changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
packages/common/src/project/load.spec.ts(1 hunks)packages/common/src/project/load.ts(2 hunks)packages/common/src/project/utils.ts(2 hunks)packages/common/src/project/versioned/base.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/common/src/project/load.ts
- packages/common/src/project/load.spec.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.{ts,tsx}: Use TypeScript throughout with strict configuration in the monorepo
Use Ethers.js for Ethereum integration
Files:
packages/common/src/project/utils.tspackages/common/src/project/versioned/base.ts
packages/**/src/**/*.{ts,tsx}
📄 CodeRabbit inference engine (CLAUDE.md)
Run ESLint with TypeScript support, Prettier, and various plugins across all TypeScript files using configured husky pre-commit hooks and lint-staged
Files:
packages/common/src/project/utils.tspackages/common/src/project/versioned/base.ts
🧬 Code graph analysis (1)
packages/common/src/project/utils.ts (1)
packages/common/src/project/versioned/base.ts (1)
formatValidationErrors(59-83)
🔇 Additional comments (3)
packages/common/src/project/utils.ts (2)
203-232: Structured ValidationError formatter correctly handles nesting and array indicesThe recursive formatter looks solid: it builds property paths with
parent.childand[index]for numericpropertyvalues, and only emits lines whenconstraintsexist while still recursing intochildren. This should yield clear messages likedataSources[0].mapping.handlers[1].filter: ...for deeply nested validation issues.
234-239: validateObject now surfaces per-property, multi-line validation detailsSwitching from a generic
toString()toformatValidationErrors(errors).join('\n')significantly improves debuggability while preserving the existingerrorMessageprefix. Callers will now see one line per failing property, including full paths into child objects/arrays, which aligns with the new manifest validation behavior.packages/common/src/project/versioned/base.ts (1)
85-90: validate() now throws a clear, multi-line manifest parse error with full property pathsUsing
validateSync(this.deployment, {whitelist: true, forbidNonWhitelisted: true})combined withthis.formatValidationErrors(errors).join('\n')produces a concise header (Failed to parse project...) followed by one bullet per invalid property, including nested/array paths. For example, an invalid filter on a handler inside a mapping should show up as something like:
- dataSources[0].mapping.handlers[1].filter: <constraint messages>This is a meaningful improvement over the previous
error.toString()output and directly addresses the need to understand errors in child objects.
| /** | ||
| * Recursively formats validation errors into a structured format. | ||
| * Handles nested errors (errors with children) by recursively processing them. | ||
| * Formats array indices with brackets for better readability (e.g., dataSources[0].mapping.handlers[1].filter). | ||
| */ | ||
| private formatValidationErrors(errors: ValidationError[], parentPath = ''): string[] { | ||
| const errorMessages: string[] = []; | ||
|
|
||
| for (const error of errors) { | ||
| // Check if property is a numeric string (array index) | ||
| const isArrayIndex = /^\d+$/.test(error.property); | ||
| const propertyPath = parentPath | ||
| ? isArrayIndex | ||
| ? `${parentPath}[${error.property}]` | ||
| : `${parentPath}.${error.property}` | ||
| : error.property; | ||
|
|
||
| if (error.constraints && Object.keys(error.constraints).length > 0) { | ||
| const constraints = Object.values(error.constraints).join(', '); | ||
| errorMessages.push(` - ${propertyPath}: ${constraints}`); | ||
| } | ||
|
|
||
| // Recursively handle nested errors | ||
| if (error.children && error.children.length > 0) { | ||
| errorMessages.push(...this.formatValidationErrors(error.children, propertyPath)); | ||
| } | ||
| } | ||
|
|
||
| return errorMessages; | ||
| } |
There was a problem hiding this comment.
This function is also defined in utils.ts and load.ts. Please just provide one implementation
There was a problem hiding this comment.
All tests that have a try/catch should also have a fail call. If the function being tested doesn't throw then there will be no expectations.
| // For nested array errors, the path should use bracket notation | ||
| // Example: dataSources[0].mapping.handlers[1].filter.specVersion | ||
| // Note: The exact path depends on class-validator's error structure, | ||
| // but we verify that array indices are formatted with brackets | ||
| const hasArrayIndexFormat = /\[\d+\]/.test(errorMessage); | ||
|
|
||
| // The error message should be structured and readable | ||
| expect(errorMessage).toMatch(/ - .+:/); |
There was a problem hiding this comment.
I don't think this is sufficient validation of the error message. We should be able to expect a full message example
2 participants
Replace TODO comment with detailed error formatting for validation failures. Error messages now include property names and constraint details instead of generic toString() output, making it easier to identify validation issues.
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.