Repository navigation
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1827 +/- ##
==========================================
+ Coverage 83.37% 83.73% +0.36%
==========================================
Files 95 96 +1
Lines 4709 4723 +14
Branches 881 884 +3
==========================================
+ Hits 3926 3955 +29
+ Misses 529 511 -18
- Partials 254 257 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
arthurschreiber
left a comment
There was a problem hiding this comment.
Thanks, this follows the existing InputError convention nicely. The main suggestion is to add the row index, as the TVP path already does, since the column name alone doesn't help much on large loads. A null row still fails with a context-free error, and one integration test lost its cause check. The rest are small test and style points.
Generated by Claude Code
cheenamalhotra
left a comment
There was a problem hiding this comment.
A few things worth addressing:
-
Breaking change for error matching. Bulk load validation errors now have a different message (
Column 'x' could not be validated at row index Ninstead of e.g.Invalid date.), and serialization errors gainat row index N. Code matching onerr.messagewill break. Since release notes come from the squash commit, consider adding a footer so it's flagged:BREAKING CHANGE: Bulk load validation and serialization errors are now `InputError`s with messages like "Column 'x' could not be validated at row index N". The original error is in `err.cause`. Rows that are not arrays or objects now throw instead of loading as NULLs.@arthurschreiber — would you treat this as breaking, or just call it out in the notes?
-
Rows that aren't objects now fail. A string or number row used to load silently as all NULLs. Good fix, but it's a behavior change. Please add a test with a primitive row (e.g.
'abc',42). -
Sensitive value claim.
writeDecimal(src/data-types/decimal-common.ts) throwsValue ${value} is out of range..., which ends up incause. Either drop the value from that message or adjust the claim in the description. -
Description is out of date. It doesn't mention the row index or the new row check. Please update the examples and the Compatibility section.
When a value in a bulk load fails type validation, the error doesn't say which column it belongs to. With many columns and many rows, the only clue is a generic message such as
Invalid date., so the bad value is hard to find.Change
Validation failures in
BulkLoadPayloadare now wrapped in anInputErrorthat names the failing column. The original validator error is kept ascause:This follows the convention Tedious already uses for the same kind of failure:
Input parameter '<name>' could not be validated(feat: provide better error messages when input param is invalid #1603)Column '<name>' could not be serialized(fix: wrap bulk load serialization errors in InputError #1772)Bulk load validation was the remaining path that still threw the validator's error directly.
Before
After
Sensitive values
The issue originally suggested attaching the rejected value to the error. This PR doesn't do that: the value could be a password, token or personal data, and errors usually end up in logs.
cause. Tedious's built-in validators use generic messages (Invalid date.,Invalid number.), so they don't include the value. A custom data type that puts the value in its own error message would still carry it incause.Compatibility
InputErrorextendsTypeError, so existinginstanceof TypeErrorchecks still pass.Column '<name>' could not be validated. The original text is still available inerror.cause.message.Tests
Unit (
test/unit/bulk-load-test.ts): new tests for both row shapes:objName) differs from the column name, so the test can check that the error names the database column rather than the propertyEach test checks the error type and the message, that
causeis the original error, and that the rejected value appears neither in the error's stack nor as a property on the error.Integration (
test/integration/bulk-load-test.ts): two existing streaming validation tests expected the bareInvalid date.error. They now expect the wrappedInputErrorwith the original error incause.