fix(polymorphic): align lossy array recovery with BetterCodable policy - #29
Conversation
Sorry, something went wrong.
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@ElonPark, you've reached your PR review limit, so we couldn't start this review. Next review available in: 43 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR. 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: 66cbbb2b-8968-442f-9f1d-4b5c7ca2a87d 📥 CommitsReviewing files that changed from the base of the PR and between c2283fa and b337468. 📒 Files selected for processing (3)
WalkthroughPolymorphic 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: Bug, Improvement, Docs 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
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. ❤️ ShareComment @coderabbitai help to get the list of available commands. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agentsVerify 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.
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
📥 CommitsReviewing files that changed from the base of the PR and between 5842b66 and c2283fa.
📒 Files selected for processing (9)- Sources/KarrotCodableKit/PolymorphicCodable/DefaultEmptyPolymorphicArrayValue.swift
- Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+OptionalPolymorphicLossyArrayValue.swift
- Sources/KarrotCodableKit/PolymorphicCodable/Extensions/KeyedDecodingContainer+PolymorphicLossyArrayValue.swift
- Sources/KarrotCodableKit/PolymorphicCodable/Extensions/UnkeyedDecodingContainer+LossyPolymorphicElements.swift
- Sources/KarrotCodableKit/PolymorphicCodable/OptionalPolymorphicLossyArrayValue.swift
- Sources/KarrotCodableKit/PolymorphicCodable/PolymorphicLossyArrayValue.swift
- Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/LossyArrayDecodeIfPresentTests.swift
- Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/LossyArrayRecoveryPolicyTests.swift
- Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/PolymorphicLossyArrayValueResilientTests.swift
Sorry, something went wrong.
| 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-L111
- Tests/KarrotCodableKitTests/PolymorphicCodable/ArrayValue/LossyArrayDecodeIfPresentTests.swift#L132-L135
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.
Sorry, something went wrong.
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.
Sorry, something went wrong.
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?
Sorry, something went wrong.
There was a problem hiding this comment.
Update: pushed b337468 addressing the review feedback on this PR.
- decodeLossyPolymorphicElements(of:) now returns a LossyPolymorphicElements value that always carries the decoded element array, and the per-element Result array only in DEBUG builds.
- Release builds decode a lossy array with a single collection allocation — the intermediate [Result] and the compactMap pass are gone.
- The decoding loop stays single-sourced; DEBUG/release only diverge at the accumulation points, mirroring how BetterCodable's @LossyArray accumulates. Net change: +53/−26 across three files.
- No behavior change: swift test passes in debug (303) and release (295), and the DEBUG outcome/results surface is identical.
The stacked formatting PR #30 has been rebased on top of this commit accordingly.
Sorry, something went wrong.
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
Background
The lossy array wrappers scattered their recovery logic across the KeyedDecodingContainer overloads, so behavior depended on which entry point decoded the value:
This violated the wrappers' own documented contract ("defaults to [] if … not a valid JSON array") and diverged from the BetterCodable wrappers (@LossyArray recovers everything inside init(from:)), which this library treats as the reference policy.
Changes
Align both lossy array wrappers with BetterCodable's @LossyArray:
Tests
Notes
https://claude.ai/code/session_017kn3Hf2khxiFy1TqimTGx9
Summary by CodeRabbit
Bug Fixes
Tests