chore: validate config to node-utils package #17720
Conversation
WalkthroughThe changes introduce a new JSON schema validation utility and update its usage across two packages. In the Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (3)
packages/node-utils/src/tests/validateJsonSchema.test.ts (2)
16-26: Consider more robust path matching in mock implementation.The current implementation checks if paths end with specific filenames, which could be fragile if paths contain these strings elsewhere.
- if (path.endsWith('config.json')) { + if (path === 'path/to/config.json') { return config; } - if (path.endsWith('schema.json')) { + if (path === 'path/to/schema.json') { return schema; }
32-48: Expand test coverage for error conditions.The current tests cover valid and invalid config scenarios, but consider adding tests for:
- File reading errors
- JSON parsing errors
- Schema compilation errors
These would ensure full coverage of the validation function's behavior.
Example additional test:
it('should throw an error when file cannot be read', () => { (fs.readFileSync as jest.Mock).mockImplementation(() => { throw new Error('ENOENT: no such file or directory'); }); expect(() => validateJsonSchema('nonexistent/config.json', 'nonexistent/schema.json') ).toThrow('ENOENT: no such file or directory'); }); it('should throw an error when config contains invalid JSON', () => { setupMockFileRead('{invalid-json}', mockSchema); expect(() => validateJsonSchema('path/to/config.json', 'path/to/schema.json') ).toThrow(/Invalid JSON/); });packages/node-utils/src/validateJsonSchema.ts (1)
1-17: Consider enhancing error handling and performance.The implementation is functional but could be improved in a few ways:
The synchronous file reading operations (
readFileSync) could block the event loop. Consider using async/await withfs.promisesfor better performance in high-concurrency scenarios.There's no specific error handling for file operations or JSON parsing failures, which might make debugging difficult.
Consider adding Ajv options like
allErrors: trueto get more comprehensive validation results.Include file paths in the error message to help with debugging.
import Ajv from 'ajv'; -import * as fs from 'fs'; +import * as fs from 'fs/promises'; // checks that a config meets the criteria specified by the schema -export const validateJsonSchema = (configPath: string, schemaPath: string) => { - const ajv = new Ajv(); +export const validateJsonSchema = async (configPath: string, schemaPath: string) => { + const ajv = new Ajv({ allErrors: true }); - const config = fs.readFileSync(configPath, 'utf-8'); - const schema = fs.readFileSync(schemaPath, 'utf-8'); + try { + const [configContent, schemaContent] = await Promise.all([ + fs.readFile(configPath, 'utf-8'), + fs.readFile(schemaPath, 'utf-8'), + ]); + + let parsedConfig, parsedSchema; + try { + parsedConfig = JSON.parse(configContent); + parsedSchema = JSON.parse(schemaContent); + } catch (e) { + throw new Error(`Invalid JSON: ${(e as Error).message}`); + } - const validate = ajv.compile(JSON.parse(schema)); - const isValid = validate(JSON.parse(config)); + const validate = ajv.compile(parsedSchema); + const isValid = validate(parsedConfig); - if (!isValid) { - throw Error(`Config is invalid: ${JSON.stringify(validate.errors)}`); - } + if (!isValid) { + throw new Error(`Config at "${configPath}" is invalid: ${JSON.stringify(validate.errors)}`); + } + } catch (error) { + if (error instanceof Error) { + throw new Error(`Validation failed: ${error.message}`); + } + throw error; + } };If you need to maintain synchronous API compatibility, consider offering both sync and async versions:
// Synchronous version (for backward compatibility) export const validateJsonSchema = (configPath: string, schemaPath: string) => { // Current implementation }; // Async version (recommended for new code) export const validateJsonSchemaAsync = async (configPath: string, schemaPath: string) => { // Async implementation };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
packages/node-utils/package.json(1 hunks)packages/node-utils/src/index.ts(1 hunks)packages/node-utils/src/tests/validateJsonSchema.test.ts(1 hunks)packages/node-utils/src/validateJsonSchema.ts(1 hunks)suite-common/message-system/package.json(0 hunks)suite-common/message-system/scripts/validate-config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- suite-common/message-system/package.json
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: Socket Security: Pull Request Alerts
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
🔇 Additional comments (4)
packages/node-utils/src/tests/validateJsonSchema.test.ts (1)
6-14: Well-structured test setup with clear mock data.The test provides good mock data and structure for validating the core functionality.
suite-common/message-system/scripts/validate-config.ts (1)
2-12: Clean refactoring that reduces code duplication.The code has been effectively refactored to use the shared
validateJsonSchemautility from the@trezor/node-utilspackage, which aligns perfectly with the PR objectives. This change centralizes the validation logic, making it easier to maintain and update in the future.One small suggestion: Consider adding a
process.exit(1)in the catch block to ensure the script exits with a non-zero status code when validation fails. This would be helpful for CI/CD pipelines that need to detect validation failures.} catch (error) { console.error(error.message); + process.exit(1); }packages/node-utils/src/index.ts (1)
15-15: Export looks good.The export of
validateJsonSchemafollows the established pattern in this file and makes the utility function available to other modules.packages/node-utils/package.json (1)
15-16: Ajv dependency added correctly.Adding Ajv as a dependency is appropriate for implementing JSON schema validation functionality. The version specification uses proper semver notation.
|
🚀 Expo preview is ready!
|
karliatto
added
the
no-project
karliatto
force-pushed
the
feat/validate-config-to-utils
branch
from
bd73718 to
eec8fb9
Compare
karliatto
changed the title
karliatto
force-pushed
the
feat/validate-config-to-utils
branch
from
eec8fb9 to
27a68e4
Compare
I agree 💯 % with you so I updated it to do only one thing 0ae0797#diff-6973949d0de6a51e54a8442d9b8014f177c4af917978bac4bfd497d8d4cff1b3 Thanks 🙏 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/node-utils/src/validateJsonSchema.ts (4)
4-8: Consider reusing the Ajv instance for better performance.Creating a new Ajv instance for each validation call could impact performance if this function is called frequently.
You could create a singleton Ajv instance or accept an optional Ajv instance parameter:
+const defaultAjv = new Ajv(); + -export const validateJsonSchema = (config: string, schema: string) => { - const ajv = new Ajv(); +export const validateJsonSchema = (config: string, schema: string, ajv = defaultAjv) => { const validate = ajv.compile(JSON.parse(schema)); const isValid = validate(JSON.parse(config));
5-5: Consider configuring Ajv with appropriate options.The current implementation uses Ajv with default settings, but you might want to enable specific features based on your validation requirements.
- const ajv = new Ajv(); + const ajv = new Ajv({ + allErrors: true, // Return all errors, not just the first one + strict: true, // Enable strict mode + // Add any other options relevant to your use case + });
3-4: Improve function documentation with JSDoc.While there is a comment explaining the function, a more detailed JSDoc would provide better documentation, especially for IDE users.
-// checks that a config meets the criteria specified by the schema +/** + * Validates a JSON configuration string against a JSON schema string + * @param {string} config - JSON string containing the configuration to validate + * @param {string} schema - JSON string containing the schema definition + * @throws {Error} Throws an error if the config doesn't conform to the schema + * or if either input is not valid JSON + */ export const validateJsonSchema = (config: string, schema: string) => {
10-12: Consider providing more structured error reporting.The current implementation includes the validation errors as a JSON string in the error message, which is informative but might be difficult to parse programmatically.
Consider creating a custom error class to provide more structured error information:
+class ValidationError extends Error { + constructor(public errors: any[]) { + super(`Config is invalid: ${JSON.stringify(errors)}`); + this.name = 'ValidationError'; + } +} export const validateJsonSchema = (config: string, schema: string) => { // ... if (!isValid) { - throw Error(`Config is invalid: ${JSON.stringify(validate.errors)}`); + throw new ValidationError(validate.errors); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
packages/node-utils/package.json(1 hunks)packages/node-utils/src/index.ts(1 hunks)packages/node-utils/src/tests/validateJsonSchema.test.ts(1 hunks)packages/node-utils/src/validateJsonSchema.ts(1 hunks)suite-common/message-system/package.json(0 hunks)suite-common/message-system/scripts/validate-config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- suite-common/message-system/package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/node-utils/src/index.ts
- packages/node-utils/package.json
- packages/node-utils/src/tests/validateJsonSchema.test.ts
- suite-common/message-system/scripts/validate-config.ts
🧰 Additional context used
🧬 Code Definitions (1)
packages/node-utils/src/validateJsonSchema.ts (1)
packages/node-utils/src/index.ts (1) (1)
validateJsonSchema(15:15)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: Socket Security: Pull Request Alerts
|
|
||
| validateConfigStructure(); | ||
| } catch (error) { | ||
| console.error(error.message); |
There was a problem hiding this comment.
btw wasnt it supposed to throw before?
There was a problem hiding this comment.
Yes, you are right. I add it to rethrow here below.
karliatto
force-pushed
the
feat/validate-config-to-utils
branch
from
27a68e4 to
540aaa8
Compare
karliatto
force-pushed
the
feat/validate-config-to-utils
branch
from
540aaa8 to
260a329
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/node-utils/src/validateJsonSchema.ts (4)
1-5: Consider passing options to Ajv for more flexible validation.The current implementation creates an Ajv instance with default settings. For more robust validation, consider passing the
{ allErrors: true }option to collect all validation errors rather than stopping at the first one.export const validateJsonSchema = (config: string, schema: string) => { - const ajv = new Ajv(); + const ajv = new Ajv({ allErrors: true });
4-28: Consider accepting parsed objects as an alternative input format.Currently, the function only accepts strings that must be parsed. For improved flexibility and performance, consider modifying the function to accept either strings or pre-parsed objects.
- export const validateJsonSchema = (config: string, schema: string) => { + export const validateJsonSchema = ( + config: string | Record<string, unknown>, + schema: string | Record<string, unknown> + ) => { const ajv = new Ajv({ allErrors: true }); let parsedConfig; let parsedSchema; - try { - parsedConfig = JSON.parse(config); - } catch (err) { - throw Error(`Invalid config JSON format: ${err.message}`); - } + if (typeof config === 'string') { + try { + parsedConfig = JSON.parse(config); + } catch (err) { + throw Error(`Invalid config JSON format: ${err.message}`); + } + } else { + parsedConfig = config; + } - try { - parsedSchema = JSON.parse(schema); - } catch (err) { - throw Error(`Invalid schema JSON format: ${err.message}`); - } + if (typeof schema === 'string') { + try { + parsedSchema = JSON.parse(schema); + } catch (err) { + throw Error(`Invalid schema JSON format: ${err.message}`); + } + } else { + parsedSchema = schema; + } const validate = ajv.compile(parsedSchema); const isValid = validate(parsedConfig); if (!isValid) { throw Error(`Config is invalid: ${JSON.stringify(validate.errors)}`); } };
7-20: Swap the order of parsing operations for more logical error handling.The current implementation parses the config before parsing the schema. Since the schema is a prerequisite for validation, it would be more logical to validate the schema first, so that any issues with the schema are reported before attempting to parse the config.
let parsedConfig; let parsedSchema; try { - parsedConfig = JSON.parse(config); + parsedSchema = JSON.parse(schema); } catch (err) { - throw Error(`Invalid config JSON format: ${err.message}`); + throw Error(`Invalid schema JSON format: ${err.message}`); } try { - parsedSchema = JSON.parse(schema); + parsedConfig = JSON.parse(config); } catch (err) { - throw Error(`Invalid schema JSON format: ${err.message}`); + throw Error(`Invalid config JSON format: ${err.message}`); }
25-27: Add more user-friendly error reporting.The current error message includes the raw validation errors in JSON format, which might be hard to read. Consider formatting the errors in a more user-friendly way, especially for complex validation failures.
if (!isValid) { - throw Error(`Config is invalid: ${JSON.stringify(validate.errors)}`); + const errorMessages = validate.errors?.map(err => + `${err.instancePath} ${err.message}` + ).join('\n - '); + throw Error(`Config validation failed:\n - ${errorMessages}`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (6)
packages/node-utils/package.json(1 hunks)packages/node-utils/src/index.ts(1 hunks)packages/node-utils/src/tests/validateJsonSchema.test.ts(1 hunks)packages/node-utils/src/validateJsonSchema.ts(1 hunks)suite-common/message-system/package.json(0 hunks)suite-common/message-system/scripts/validate-config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- suite-common/message-system/package.json
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/node-utils/src/index.ts
- packages/node-utils/package.json
- packages/node-utils/src/tests/validateJsonSchema.test.ts
- suite-common/message-system/scripts/validate-config.ts
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: PR-check / node-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / node-override init-api-flaky
- GitHub Check: PR-check / web-authorizeCoinjoin cancelCoinjoinAuthorization passphrase unlockPath setBusy checkFirmwareAuthenticity keepSession cancel.test info.test resetDevice-api
- GitHub Check: PR-check / web-override init-api-flaky
- GitHub Check: build-deploy
- GitHub Check: build-deploy
- GitHub Check: prepare_android_test_app
- GitHub Check: EAS Update
- GitHub Check: transport-e2e-test
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: build-web
- GitHub Check: test
- GitHub Check: Socket Security: Pull Request Alerts
Description
In preparation for creating releases json schema this would be useful in order to avoid duplicated code since we will have to validate the new schema for releases messages.