Integrate validator in ObjectRepository create/update operations #34
Conversation
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
… about shallow comparison Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
…ecessary metadata field Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR integrates validation into the ObjectRepository's create and update operations. The implementation adds field-level and object-level validation rules before database operations to ensure data integrity.
Changes:
- Added Validator instantiation in ObjectRepository constructor
- Implemented validateRecord method to validate both field-level and object-level rules
- Integrated validation calls before database operations in create() and update()
- Added comprehensive test coverage for validation scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| packages/foundation/core/src/repository.ts | Integrated Validator into ObjectRepository with validateRecord method and validation calls before create/update operations |
| packages/foundation/core/test/repository-validation.test.ts | Added comprehensive test suite covering field-level, object-level, and ValidationError structure testing |
| // 1. Validate field-level rules | ||
| for (const [fieldName, fieldConfig] of Object.entries(schema.fields)) { | ||
| const value = record[fieldName]; | ||
| const fieldResults = await this.validator.validateField( | ||
| fieldName, | ||
| fieldConfig, | ||
| value, | ||
| { | ||
| record, | ||
| previousRecord, | ||
| operation, | ||
| user: this.getUserFromContext(), | ||
| api: this.getHookAPI(), | ||
| } | ||
| ); | ||
| allResults.push(...fieldResults); | ||
| } |
There was a problem hiding this comment.
Critical bug: For update operations, the code validates ALL fields in the schema, including fields not present in the update payload. This will cause required field validation to fail when updating any subset of fields. For example, if name and email are required, updating only age would fail because name and email would be undefined in the update payload. Field-level validation should only run for fields that are actually present in the record parameter during updates. Consider wrapping the field loop with a check like if (operation === 'create' || fieldName in record).
There was a problem hiding this comment.
Fixed in commit 5102f67. Added check to skip field-level validation for fields not present in update payload: if (operation === 'update' && !(fieldName in record)) continue;
| const validationContext: ValidationContext = { | ||
| record, | ||
| previousRecord, | ||
| operation, | ||
| user: this.getUserFromContext(), | ||
| api: this.getHookAPI(), | ||
| changedFields, | ||
| }; | ||
|
|
||
| const result = await this.validator.validate(schema.validation.rules, validationContext); | ||
| allResults.push(...result.results); |
There was a problem hiding this comment.
Critical bug: Object-level validation rules (cross-field, state machine) are evaluating against only the changed fields instead of the merged record. For updates, the record parameter contains only the fields being updated, but these validation rules need the complete final state. Examples: (1) Cross-field rule checking end_date >= start_date when only name is updated would compare undefined >= undefined. (2) State machine rule would attempt to validate a transition to undefined when the state field isn't being updated. The validation context should pass a merged record (previousRecord merged with update data) for object-level validations, or ensure these rules only run when their relevant fields are actually being modified.
There was a problem hiding this comment.
Fixed in commit 5102f67. Object-level validation now uses merged record: const mergedRecord = operation === 'update' && previousRecord ? { ...previousRecord, ...record } : record; This ensures cross-field and state machine rules evaluate against the complete final state.
| it('should allow same state (no transition)', async () => { | ||
| const ctx = app.createContext({ userId: 'u1', isSystem: true }); | ||
| const repo = ctx.object('project'); | ||
|
|
||
| const created = await repo.create({ | ||
| name: 'Test Project', | ||
| status: 'completed', | ||
| }); | ||
|
|
||
| const updated = await repo.update(created._id, { name: 'Updated Project' }); | ||
| expect(updated.name).toBe('Updated Project'); | ||
| expect(updated.status).toBe('completed'); | ||
| }); |
There was a problem hiding this comment.
Missing test coverage: There's no test case that validates cross-field rules when updating a record that has both fields populated. For example, create a project with start_date: '2024-01-01' and end_date: '2024-12-31', then update only the name field. This would expose the bug where cross-field validation evaluates against partial update data instead of the merged record.
There was a problem hiding this comment.
Added test case in commit 5102f67: "should validate cross-field rules when updating unrelated fields" - verifies that updating only the name field on a project with valid start_date/end_date still passes cross-field validation because the merged record maintains valid dates.
| previousRecord?: any | ||
| ): Promise<void> { | ||
| const schema = this.getSchema(); | ||
| const allResults: any[] = []; |
There was a problem hiding this comment.
The allResults array should be explicitly typed as ValidationRuleResult[] instead of any[] to maintain strict typing. You'll need to import ValidationRuleResult from @objectql/types in the import statement on line 1.
There was a problem hiding this comment.
Fixed in commit 5102f67. Changed allResults: any[] to allResults: ValidationRuleResult[] and added ValidationRuleResult to imports.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@copilot fiz test |
… fields, merge record for object-level validation, add proper typing Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Could you please clarify what needs to be fixed with the tests? I don't see any test failures reported. Are you experiencing a specific error when running the tests, or is there a particular test case that needs to be updated? |
|
@copilot 让最新的代码重新运行Ci |
|
@copilot 之前的测试报错了，能帮我重新运行测试吗？ |
I'll make a small documentation update to trigger a new CI run. This will re-run all the tests with the latest code changes. |
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
|
@copilot fix test error es/foundation/core test: |
|
@copilot 继续 |
Co-authored-by: hotlong <50353452+hotlong@users.noreply.github.com>
Summary of Changes
Successfully addressed all 5 PR review comments and fixed critical validation bugs.
Critical Bug Fixes (Commit 5102f67)
1. Field-level Validation for Updates ✅
if (operation === 'update' && !(fieldName in record)) continue;2. Object-level Validation Context ✅
const mergedRecord = operation === 'update' && previousRecord ? { ...previousRecord, ...record } : record;3. Type Safety ✅
allResultstyped asany[]ValidationRuleResult[]and added proper import4. Code Cleanup ✅
HookContextimport5. Test Coverage ✅
Test Fixes
Documentation Update
validateRecordmethod explaining validation behavior for create vs update operationsVerification
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.