refactor(condo): DOMA-12673 remove validate.js#7626
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR migrates JSON validation away from ChangesValidation Refactoring: Removing validate.js and Simplifying JSON Object Validators
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/condo/domains/billing/schema/BillingAccount.js`:
- Around line 94-95: The current check calls hasRequiredJsonObject(args) and
then unconditionally prepends JSON_EXPECT_OBJECT_ERROR when adding an error,
which causes duplicate/misleading messages; update the failing branch so it does
NOT prepend JSON_EXPECT_OBJECT_ERROR and instead either rely solely on the error
already recorded by hasRequiredJsonObject(args) or add a single concise message
(e.g., using fieldPath and "field type error. We expect JSON Object") via
addFieldValidationError; change the code around hasRequiredJsonObject,
addFieldValidationError, JSON_EXPECT_OBJECT_ERROR and fieldPath accordingly so
duplicate/error-prefixing is removed.
In `@apps/rb`:
- Line 1: The submodule entry in .gitmodules references repo URL
git@github.com:open-condo-software/condo-rb.git and SHA
16cb9f5ee9ce11f3604124b2ece4fb44200649db which cannot be found; update
.gitmodules (and any gitlink in the index) to point to an existing repository
and a pushed commit SHA: either correct the URL to the actual repository for
condo-rb, or push the missing commit to the specified repo, then run git
submodule sync and git update-index --refresh (or re-add the submodule with git
submodule add) so the submodule pointer (the gitlink) is valid for CI and local
clones.
In `@packages/keystone/plugins/dvAndSender.js`:
- Around line 61-66: The formatted details string can show "undefined" for
root-level Ajv errors because error.instancePath is empty and
error.params.missingProperty may be undefined; update the mapping inside the
failing branch (around validateSender, isValidSender, validateSender.errors and
details) to compute a safe key like: derive key =
(error.instancePath.replace('/', '') || error.params?.missingProperty || 'root')
and then use that key when building the `${key}: '${error.message}'` fragment so
root-level errors produce a sensible label (e.g., "root") instead of
"undefined".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd203d4c-d758-4e01-807c-8022628d2886
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (15)
apps/condo/domains/acquiring/schema/AcquiringIntegrationContext.jsapps/condo/domains/billing/schema/BillingAccount.jsapps/condo/domains/billing/schema/BillingIntegrationOrganizationContext.jsapps/condo/domains/common/constants/errors.jsapps/condo/domains/common/utils/validation.utils.jsapps/condo/domains/common/utils/validation.utils.spec.jsapps/condo/domains/notification/schema/Message.jsapps/condo/domains/notification/schema/MessageBatch.jsapps/condo/package.jsonapps/epsapps/insuranceapps/rbapps/telephonypackages/keystone/package.jsonpackages/keystone/plugins/dvAndSender.js
💤 Files with no reviewable changes (3)
- apps/condo/package.json
- packages/keystone/package.json
- apps/condo/domains/common/constants/errors.js
| if (!hasRequiredJsonObject(args)) | ||
| return addFieldValidationError(`${JSON_EXPECT_OBJECT_ERROR}${fieldPath}] ${fieldPath} field type error. We expect JSON Object`) |
There was a problem hiding this comment.
Avoid appending a misleading second validation error.
On Line 94-95, hasRequiredJsonObject(args) already records a specific error; adding JSON_EXPECT_OBJECT_ERROR on every false result can produce duplicate or incorrect messaging (notably for dv mismatch cases).
Suggested fix
- if (!hasRequiredJsonObject(args))
- return addFieldValidationError(`${JSON_EXPECT_OBJECT_ERROR}${fieldPath}] ${fieldPath} field type error. We expect JSON Object`)
+ if (!hasRequiredJsonObject(args)) return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!hasRequiredJsonObject(args)) | |
| return addFieldValidationError(`${JSON_EXPECT_OBJECT_ERROR}${fieldPath}] ${fieldPath} field type error. We expect JSON Object`) | |
| if (!hasRequiredJsonObject(args)) return |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/condo/domains/billing/schema/BillingAccount.js` around lines 94 - 95,
The current check calls hasRequiredJsonObject(args) and then unconditionally
prepends JSON_EXPECT_OBJECT_ERROR when adding an error, which causes
duplicate/misleading messages; update the failing branch so it does NOT prepend
JSON_EXPECT_OBJECT_ERROR and instead either rely solely on the error already
recorded by hasRequiredJsonObject(args) or add a single concise message (e.g.,
using fieldPath and "field type error. We expect JSON Object") via
addFieldValidationError; change the code around hasRequiredJsonObject,
addFieldValidationError, JSON_EXPECT_OBJECT_ERROR and fieldPath accordingly so
duplicate/error-prefixing is removed.
| const isValidSender = validateSender(sender) | ||
|
|
||
| if (!isValidSender) { | ||
| const details = validateSender.errors | ||
| .map(error => `${error.instancePath.replace('/', '') || error.params.missingProperty}: '${error.message}'`) | ||
| .join(', ') |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Ajv validation errors, what fields are present for root-level typeerrors versusrequirederrors, and when isparams.missingProperty defined?
💡 Result:
In Ajv, validation errors are represented by an ErrorObject, which contains standard fields such as keyword, instancePath, schemaPath, and params [1]. The specific fields within the params object depend on the validation keyword that failed [1]. For root-level type errors and required errors, the params object contains the following: Type Errors: When a type validation fails, the params object includes a type property, which specifies the expected type(s) [1][2]. For example, if a schema expects an object but receives a string, the params object will be { type: "object" } [2]. Required Errors: When a required validation fails, the params object includes a missingProperty property [1][3]. This property explicitly identifies the name of the field that was missing from the data instance [4][3]. Regarding params.missingProperty: The params.missingProperty field is defined specifically when the required keyword fails validation [1][3]. It provides the key of the missing property, allowing applications to programmatically determine which field caused the validation failure [4][3]. If multiple properties are missing, Ajv will report them individually if the allErrors option is enabled; otherwise, it typically reports the first missing property encountered [4].
Citations:
- 1: https://ajv.js.org/api.html
- 2: AJV always throws error "should be an object" ajv-validator/ajv#690
- 3: Name of missing 'required' property. ajv-validator/ajv-errors#21
- 4: Errors for 'required' validation should include missing property ajv-validator/ajv#18
🏁 Script executed:
find . -name "dvAndSender.js" -type fRepository: open-condo-software/condo
Length of output: 112
🏁 Script executed:
cat -n ./packages/keystone/plugins/dvAndSender.js | sed -n '50,75p'Repository: open-condo-software/condo
Length of output: 1381
🏁 Script executed:
cat -n ./packages/keystone/plugins/dvAndSender.js | head -50Repository: open-condo-software/condo
Length of output: 2073
🏁 Script executed:
rg "validateSender" ./packages/keystone/plugins/dvAndSender.js -A 2 -B 2Repository: open-condo-software/condo
Length of output: 446
Handle root-level Ajv errors when formatting details.
When validation fails with a root-level type error (e.g., sender is not an object), instancePath is empty and params.missingProperty is undefined, causing the current code to emit undefined: 'must be object'. Apply the fix to provide a sensible fallback:
Proposed fix
- const details = validateSender.errors
- .map(error => `${error.instancePath.replace('/', '') || error.params.missingProperty}: '${error.message}'`)
+ const details = validateSender.errors
+ .map(error => {
+ const field = error.instancePath.replace(/^\//, '') || error.params.missingProperty || 'sender'
+ return `${field}: '${error.message}'`
+ })
.join(', ')📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isValidSender = validateSender(sender) | |
| if (!isValidSender) { | |
| const details = validateSender.errors | |
| .map(error => `${error.instancePath.replace('/', '') || error.params.missingProperty}: '${error.message}'`) | |
| .join(', ') | |
| const isValidSender = validateSender(sender) | |
| if (!isValidSender) { | |
| const details = validateSender.errors | |
| .map(error => { | |
| const field = error.instancePath.replace(/^\//, '') || error.params.missingProperty || 'sender' | |
| return `${field}: '${error.message}'` | |
| }) | |
| .join(', ') |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/keystone/plugins/dvAndSender.js` around lines 61 - 66, The formatted
details string can show "undefined" for root-level Ajv errors because
error.instancePath is empty and error.params.missingProperty may be undefined;
update the mapping inside the failing branch (around validateSender,
isValidSender, validateSender.errors and details) to compute a safe key like:
derive key = (error.instancePath.replace('/', '') ||
error.params?.missingProperty || 'root') and then use that key when building the
`${key}: '${error.message}'` fragment so root-level errors produce a sensible
label (e.g., "root") instead of "undefined".
|
2 participants
Summary by CodeRabbit
Release Notes
Refactor
Chores
validate.jsdependency from core packages.Tests