fix(stack): bulk-model id split corrupts field names containing hyphens#548
Conversation
The bulk model helpers key per-field operation results by
`${modelIndex}-${fieldKey}` ids and reconstructed models with a naive
key.split('-'), truncating any field key containing a hyphen
(some-field → some): the value landed under the truncated key and the
real field silently vanished. Duplicated across all four multi-model
reconstruction sites since f1248d5.
Extract one fieldsForModelIndex helper that splits at the FIRST hyphen
only and use it at all four sites. Offline regression tests (mocked
protect-ffi) prove hyphenated and multi-hyphen field names survive
bulkEncryptModels / bulkDecryptModels — both fail against the naive
split.
Flagged in the consolidated review on #547 (pre-existing, out of that
PR's scope).
|
📝 WalkthroughWalkthroughAdds a ChangesHyphenated field key fix
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant bulkEncryptModels
participant fieldsForModelIndex
participant ProtectFFI
Client->>bulkEncryptModels: bulk encrypt request with hyphenated fields
bulkEncryptModels->>ProtectFFI: encryptBulk(payload)
ProtectFFI-->>bulkEncryptModels: encryptedData keyed by modelIndex-fieldKey
bulkEncryptModels->>fieldsForModelIndex: fieldsForModelIndex(encryptedData, modelIndex)
fieldsForModelIndex-->>bulkEncryptModels: fieldKey map (first-hyphen split preserved)
bulkEncryptModels-->>Client: reconstructed model with hyphenated keys intact
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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.
🧹 Nitpick comments (1)
packages/stack/__tests__/bulk-model-hyphen-fields.test.ts (1)
78-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for lock-context variants.
Only
bulkEncryptModels/bulkDecryptModelsare tested. The samefieldsForModelIndexfix also applies tobulkEncryptModelsWithLockContext/bulkDecryptModelsWithLockContext(model-helpers.ts lines 809, 872), which remain untested by this regression suite.🤖 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/stack/__tests__/bulk-model-hyphen-fields.test.ts` around lines 78 - 118, Add regression coverage for the lock-context bulk helpers because the hyphenated-field fix in fieldsForModelIndex also affects bulkEncryptModelsWithLockContext and bulkDecryptModelsWithLockContext. Extend the bulk-model-hyphen-fields test suite with cases that call those two methods, using the same hyphenated keys assertions as the existing bulkEncryptModels and bulkDecryptModels tests. Keep the checks focused on the named helpers so the suite verifies hyphenated field names remain intact in both lock-context and non-lock-context paths.
🤖 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.
Nitpick comments:
In `@packages/stack/__tests__/bulk-model-hyphen-fields.test.ts`:
- Around line 78-118: Add regression coverage for the lock-context bulk helpers
because the hyphenated-field fix in fieldsForModelIndex also affects
bulkEncryptModelsWithLockContext and bulkDecryptModelsWithLockContext. Extend
the bulk-model-hyphen-fields test suite with cases that call those two methods,
using the same hyphenated keys assertions as the existing bulkEncryptModels and
bulkDecryptModels tests. Keep the checks focused on the named helpers so the
suite verifies hyphenated field names remain intact in both lock-context and
non-lock-context paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ddbb1afc-1167-4fe1-b91a-3cbee7f6661b
📒 Files selected for processing (2)
packages/stack/__tests__/bulk-model-hyphen-fields.test.tspackages/stack/src/encryption/helpers/model-helpers.ts
1 participant
Summary
The bulk model helpers key per-field operation results by
${modelIndex}-${fieldKey}ids, and reconstruction split that id with a naivekey.split('-')— truncating any field key containing a hyphen (some-field→some): the encrypted/decrypted value landed under the truncated key and the real field silently vanished from the model. The bug was duplicated byte-identically across all four multi-model reconstruction sites (bulk encrypt/decrypt, with and without lock context) sincef1248d5f.Fix
One
fieldsForModelIndexhelper that splits at the first hyphen only, used at all four sites (also removes the 4× duplication).Tests
Offline regression suite (mocked protect-ffi): hyphenated (
some-field) and multi-hyphen (multi-part-name) column names survivebulkEncryptModels/bulkDecryptModels, with explicit assertions that no truncated keys appear. Verified both tests fail against the naive split before the fix.Provenance
Flagged in the consolidated review on #547 (🔴 "bulk-encrypt id
${idx}-${key}corrupts field names containing-") — pre-existing onmain, so fixed here rather than in that stacked PR.Summary by CodeRabbit
some-fieldormulti-part-name.