fix(polymorphic): align lossy array recovery with BetterCodable policy#29
Conversation
Lossy array wrappers scattered their recovery logic across the KeyedDecodingContainer overloads, which made behavior depend on the entry point: - decode(_:forKey:) recovered a non-array value to an empty array while decodeIfPresent(_:forKey:) propagated the error, and their release null outcomes disagreed (.valueWasNil vs .decodedSuccessfully). - OptionalPolymorphicLossyArrayValue threw on a non-array value even through plain @OptionalPolymorphicLossyArray properties, violating its lossy contract. - Decoding the wrappers directly (e.g., nested in other collections) bypassed the overloads entirely, so nothing was recovered. Align both wrappers with BetterCodable's @LossyArray: - Move null / non-array recovery into init(from:) so every entry point (decode, decodeIfPresent, direct decoding) behaves identically. The container overloads now handle only the missing-key case. - OptionalPolymorphicLossyArrayValue is now the exact optional variant of PolymorphicLossyArrayValue: it recovers a non-array value to [] and keeps nil reserved for a missing key or an explicit null. - Element failures are recorded in the decoding outcome as an ArrayDecodingError and reported per element to the resilient error reporter in DEBUG builds, matching @LossyArray. - Extract the shared lossy element loop into UnkeyedDecodingContainer.decodeLossyPolymorphicElementResults(of:). - Rewrite both wrappers' documentation to describe the actual policy. Claude-Session: https://claude.ai/code/session_017kn3Hf2khxiFy1TqimTGx9
… docs The doc comment claimed that any invalid element causes the whole decoding to fail with a thrown error, but init(from:) catches every error and falls back to an empty array with a .recoveredFrom outcome. It also claimed errors are logged via print, while they are reported to the resilient decoding error reporter in DEBUG builds. Align the docs with the actual behavior and wrap the comment block to the 120-column limit. Claude-Session: https://claude.ai/code/session_017kn3Hf2khxiFy1TqimTGx9
|
Warning Review limit reached
Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughPolymorphic lossy array decoding now centralizes per-element recovery, records array decoding errors, and delegates keyed decoding behavior to wrapper initializers. Tests cover missing, null, invalid, partial, and direct-decoding cases. ChangesLossy array recovery
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.
Actionable comments posted: 2
🤖 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
`@Sources/KarrotCodableKit/PolymorphicCodable/Extensions/UnkeyedDecodingContainer`+LossyPolymorphicElements.swift:
- Around line 17-33: The unkeyed decoding loop in the lossy polymorphic elements
extension unnecessarily stores a full Result array before callers compact it.
Preserve Result collection and error reporting only under DEBUG; in release
builds, append each successfully decoded PolymorphicValue directly to the
returned expected-type array while continuing past failures, avoiding the
intermediate allocation.
In
`@Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/LossyArrayRecoveryPolicyTests.swift`:
- Around line 124-127: Rename OptionalLossyArrayDummyResponse to
LossyArrayDummyResponse at all three affected sites:
LossyArrayRecoveryPolicyTests.swift lines 124-127, and
LossyArrayDecodeIfPresentTests.swift lines 108-111 and 132-135. Update each
reference consistently while preserving the non-optional [any DummyNotice]
property and existing test behavior.
🪄 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: Path: .coderabbit.yml
Review profile: ASSERTIVE
Plan: Pro
Run ID: bdd557a9-041b-469d-bbef-ddd6249b76c9
📒 Files selected for processing (9)
Sources/KarrotCodableKit/PolymorphicCodable/DefaultEmptyPolymorphicArrayValue.swiftSources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+OptionalPolymorphicLossyArrayValue.swiftSources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+PolymorphicLossyArrayValue.swiftSources/KarrotCodableKit/PolymorphicCodable/Extensions/UnkeyedDecodingContainer+LossyPolymorphicElements.swiftSources/KarrotCodableKit/PolymorphicCodable/OptionalPolymorphicLossyArrayValue.swiftSources/KarrotCodableKit/PolymorphicCodable/PolymorphicLossyArrayValue.swiftTests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/LossyArrayDecodeIfPresentTests.swiftTests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/LossyArrayRecoveryPolicyTests.swiftTests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/PolymorphicLossyArrayValueResilientTests.swift
| let result = try JSONDecoder().decode( | ||
| OptionalLossyArrayDummyResponse.self, | ||
| from: Data(jsonData.utf8) | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for LossyArray dummy responses to find the correct non-optional struct name.
rg 'struct .*LossyArrayDummyResponse' Tests/KarrotCodableKitTests/Repository: daangn/KarrotCodableKit
Length of output: 442
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Definitions =="
rg -n "struct (OptionalLossyArrayDummyResponse|OptionalPolymorphicLossyArrayDummyResponse)|`@PolymorphicLossyArray`|`@OptionalPolymorphicLossyArray`|notices1|decodeResult" Tests/KarrotCodableKitTests/PolymorphicCodable/TestDoubles/PolymorphicValueCodableDummy.swift Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/LossyArrayRecoveryPolicyTests.swift Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/LossyArrayDecodeIfPresentTests.swift
echo
echo "== Relevant file excerpts =="
sed -n '1,220p' Tests/KarrotCodableKitTests/PolymorphicCodable/TestDoubles/PolymorphicValueCodableDummy.swift
echo
sed -n '1,220p' Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/LossyArrayRecoveryPolicyTests.swift
echo
sed -n '1,220p' Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/LossyArrayDecodeIfPresentTests.swiftRepository: daangn/KarrotCodableKit
Length of output: 19032
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,220p' Tests/KarrotCodableKitTests/PolymorphicCodable/TestDoubles/PolymorphicValueCodableDummy.swiftRepository: daangn/KarrotCodableKit
Length of output: 3963
Rename the non-optional lossy-array dummy response. OptionalLossyArrayDummyResponse wraps a non-optional [any DummyNotice], so the name suggests an optional property where there isn’t one. A non-optional name like LossyArrayDummyResponse would make these three test sites read consistently with OptionalPolymorphicLossyArrayDummyResponse.
📍 Affects 2 files
Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/LossyArrayRecoveryPolicyTests.swift#L124-L127(this comment)Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/LossyArrayDecodeIfPresentTests.swift#L108-L111Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/LossyArrayDecodeIfPresentTests.swift#L132-L135
🤖 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
`@Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/LossyArrayRecoveryPolicyTests.swift`
around lines 124 - 127, Rename OptionalLossyArrayDummyResponse to
LossyArrayDummyResponse at all three affected sites:
LossyArrayRecoveryPolicyTests.swift lines 124-127, and
LossyArrayDecodeIfPresentTests.swift lines 108-111 and 132-135. Update each
reference consistently while preserving the non-optional [any DummyNotice]
property and existing test behavior.
There was a problem hiding this comment.
Fair point on the name — OptionalLossyArrayDummyResponse does wrap a non-optional array. However, it is a pre-existing test double from main; this PR only adds new usages of it. Renaming it here would pull unrelated existing test files into this PR's diff and create rebase conflicts with the formatting-only stack in #30.
Will do the rename as a small follow-up after this stack merges.
There was a problem hiding this comment.
@ElonPark, that makes sense — thanks for the context on the stack ordering with #30. Deferring the rename to avoid unrelated diff noise and rebase conflicts is the right call here.
Would you like me to open a follow-up issue to track the OptionalLossyArrayDummyResponse → LossyArrayDummyResponse rename (and its usages in LossyArrayDecodeIfPresentTests.swift) so it doesn't get lost after this stack merges?
There was a problem hiding this comment.
Update: pushed b337468 addressing the review feedback on this PR.
decodeLossyPolymorphicElements(of:)now returns aLossyPolymorphicElementsvalue that always carries the decoded element array, and the per-elementResultarray only in DEBUG builds.- Release builds decode a lossy array with a single collection allocation — the intermediate
[Result]and thecompactMappass are gone. - The decoding loop stays single-sourced; DEBUG/release only diverge at the accumulation points, mirroring how BetterCodable's
@LossyArrayaccumulates. Net change: +53/−26 across three files. - No behavior change:
swift testpasses in debug (303) and release (295), and the DEBUGoutcome/resultssurface is identical.
The stacked formatting PR #30 has been rebased on top of this commit accordingly.
Address CodeRabbit feedback on #29: the lossy element helper built a full [Result] array and callers built a second array via compactMap, doubling transient storage in release builds where per-element results are unobservable. Return a LossyPolymorphicElements value instead, which carries the element array always and the per-element results only in DEBUG builds, so release decodes a lossy array with a single collection allocation. Behavior is unchanged in both configurations. Includes house-style formatting (trailing commas, redundant self) that the format hook applied to the touched files. Claude-Session: https://claude.ai/code/session_017kn3Hf2khxiFy1TqimTGx9
Address CodeRabbit feedback on #29: the lossy element helper built a full [Result] array and callers built a second array via compactMap, doubling transient storage in release builds where per-element results are unobservable. Return a LossyPolymorphicElements value instead, which carries the element array always and the per-element results only in DEBUG builds, so release decodes a lossy array with a single collection allocation. Behavior is unchanged in both configurations. Includes house-style formatting (trailing commas, redundant self) that the format hook applied to the touched files. Claude-Session: https://claude.ai/code/session_017kn3Hf2khxiFy1TqimTGx9
d70a3aa to
b337468
Compare
1 participant
Background
The lossy array wrappers scattered their recovery logic across the
KeyedDecodingContaineroverloads, so behavior depended on which entry point decoded the value:@PolymorphicLossyArray@OptionalPolymorphicLossyArray(before)"notices": "oops"(non-array) via property[]outcomereported.decodedSuccessfullyThis violated the wrappers' own documented contract ("defaults to
[]if … not a valid JSON array") and diverged from the BetterCodable wrappers (@LossyArrayrecovers everything insideinit(from:)), which this library treats as the reference policy.Changes
Align both lossy array wrappers with BetterCodable's
@LossyArray:init(from:)so every entry point (decode,decodeIfPresent, direct decoding) behaves identically. The container overloads now handle only the missing-key case.OptionalPolymorphicLossyArrayValueis now the exact optional variant ofPolymorphicLossyArrayValue: it recovers a non-array value to[], andnilstays reserved for a missing key or an explicitnull.outcomeas anArrayDecodingErrorand reported per element to the resilient error reporter in DEBUG builds, matching@LossyArray.UnkeyedDecodingContainer.decodeLossyPolymorphicElementResults(of:).DefaultEmptyPolymorphicArrayValuedocs that claimed element failures throw (they recover to[]).Tests
LossyArrayRecoveryPolicyTestspins the policy: non-array recovery through properties and through direct decoding, plus element-failure outcomes (written red-first against the old behavior).LossyArrayDecodeIfPresentTestscovers the previously untesteddecodeIfPresentcontainer path (missing / null / present / non-array).swift testpasses in debug (303) and release (295) configurations.Notes
https://claude.ai/code/session_017kn3Hf2khxiFy1TqimTGx9
Summary by CodeRabbit
Bug Fixes
nilwhere appropriate.Tests