Skip to content

Fix broken errors thrown by JSONDecoder when parsing decimal as Int #1606

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hiragram wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix broken errors thrown by JSONDecoder when parsing decimal as Int #1606

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions Sources/FoundationEssentials/JSON/JSONDecoder.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1000,13 +1000,29 @@ extension JSONDecoderImpl: Decoder {
}

static private func _slowpath_unwrapFixedWidthInteger<T: FixedWidthInteger>(as type: T.Type, json5: Bool, numberBuffer: BufferView<UInt8>, fullSource: BufferView<UInt8>, digitBeginning: BufferViewIndex<UInt8>, for codingPathNode: _CodingPathNode, _ additionalKey: (some CodingKey)?) throws -> T {
// Helper function to create number conversion error
func createNumberConversionError() -> DecodingError {
#if FOUNDATION_FRAMEWORK
Copy link
Contributor

@itingliu itingliu Nov 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to preserve this existing (broken) behavior for FOUNDATION_FRAMEWORK as long as underlyingError's error code is untouched... @kperryua what do you think?

Copy link
Author

@hiragram hiragram Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@itingliu Thanks for your review. Just to confirm, are you saying that underlyingError should always be
nil on all platforms in this case?

There is existing code that switches the presence of underlyingError based on the FOUNDATION_FRAMEWORK flag, which I used as a reference. I don't fully understand the intent behind it, but I think it would be better to keep the behavior consistent.

https://github.com/hiragram/swift-foundation/blob/fix-jsondecoder/Sources/FoundationEssentials/JSON/JSONDecoder.swift#L388-L393

Copy link
Contributor

@kperryua kperryua Jan 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In retrospect, a JSONError seems incorrect here to begin with, as we've already pre-validated the buffer and successfully parsed some a number. Hence, it's valid JSON. (If we fail to parse, we call validateNumber afterwards, which gives us a JSONError that tells us exactly what's wrong.)

We should just be throwing the dataCorrupted error with no underlying error at all, on all platforms.

Copy link
Author

@hiragram hiragram Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kperryua Should that be handled in this Pull Request? Personally, I think it's better to first fix the specific broken behaviors and have the discussion about what the "grand design" should look like in a separate thread.

Copy link
Contributor

@kperryua kperryua Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn’t consider this a “grand design” change. It’s a simplification of this PR by virtue of realigning this particular error throwing instance with the correct layering of errors. Nothing about the PR needs to change except eliding the creation of the underlying error (and the corresponding test code).

let underlyingError: Error? = JSONError.numberIsNotRepresentableInSwift(
parsed: String(decoding: numberBuffer, as: UTF8.self)
).nsError
#else
let underlyingError: Error? = nil
#endif
return DecodingError.dataCorrupted(DecodingError.Context(
codingPath: codingPathNode.path(byAppending: additionalKey),
debugDescription: "Parsed JSON number <\(String(decoding: numberBuffer, as: UTF8.self))> does not fit in \(T.self).",
underlyingError: underlyingError
))
}

// This is the slow path... If the fast path has failed. For example for "34.0" as an integer, we try to parse as either a Decimal or a Double and then convert back, losslessly.
if let double = Double(prevalidatedBuffer: numberBuffer) {
// T.init(exactly:) guards against non-integer Double(s), but the parser may
// have already transformed the non-integer "1.0000000000000001" into 1, etc.
// Proper lossless behavior should be implemented by the parser.
guard let value = T(exactly: double) else {
throw JSONError.numberIsNotRepresentableInSwift(parsed: String(decoding: numberBuffer, as: UTF8.self))
throw createNumberConversionError()
}

// The distance between Double(s) is >=2 from ±2^53.
Expand All @@ -1021,7 +1037,7 @@ extension JSONDecoderImpl: Decoder {
let decimalParseResult = Decimal._decimal(from: numberBuffer, matchEntireString: true).asOptional
if let decimal = decimalParseResult.result {
guard let value = T(decimal) else {
throw JSONError.numberIsNotRepresentableInSwift(parsed: String(decoding: numberBuffer, as: UTF8.self))
throw createNumberConversionError()
}
return value
}
Expand Down
116 changes: 116 additions & 0 deletions Tests/FoundationEssentialsTests/JSONEncoderTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -901,6 +901,122 @@ private struct JSONEncoderTests {
_testDecodeFailure(of: DecodeFailure.self, data: toDecode.data(using: .utf8)!)
}

@Test("Decimal to Int decoding error message regression test for issue #1604")
func decimalToIntDecodingErrorMessage() throws {
struct Object: Decodable {
var foo: Int
}

let json = """
{
"foo": 123.45
}
""".data(using: .utf8)!

do {
_ = try JSONDecoder().decode(Object.self, from: json)
Issue.record("Expected decoding error but decoding succeeded")
} catch let error as DecodingError {
guard case .dataCorrupted(let context) = error else {
Issue.record("Expected dataCorrupted error, got \(error)")
return
}

// Verification 1: Check if codingPath is correctly set
// Expected: ["foo"]
#expect(context.codingPath.count == 1, "codingPath should contain one element, got \(context.codingPath)")
if let key = context.codingPath.first {
#expect(key.stringValue == "foo", "codingPath should be ['foo'], got '\(key.stringValue)'")
}

// Verification 2: Check if debugDescription is appropriate
// Expected: A specific message like "Parsed JSON number <123.45> does not fit in Int."
#expect(
context.debugDescription.contains("123.45"),
"debugDescription should mention the number 123.45, got: \(context.debugDescription)"
)
#expect(
context.debugDescription.contains("Int") || context.debugDescription.contains("fit"),
"debugDescription should mention Int or fit, got: \(context.debugDescription)"
)

// Verification 3 (macOS only): Check if underlyingError is provided
#if FOUNDATION_FRAMEWORK
#expect(context.underlyingError != nil, "macOS should provide underlyingError, got nil")

if let nsError = context.underlyingError as? NSError {
#expect(nsError.domain == NSCocoaErrorDomain, "underlyingError domain should be NSCocoaErrorDomain")
#expect(nsError.code == CocoaError.propertyListReadCorrupt.rawValue, "underlyingError code should match propertyListReadCorrupt")

// Verify the error message mentions the number
if let debugDesc = nsError.userInfo[NSDebugDescriptionErrorKey] as? String {
#expect(debugDesc.contains("123.45"), "underlyingError should mention the number 123.45")
}
}
#endif
} catch {
Issue.record("Expected DecodingError, got \(type(of: error)): \(error)")
}
}

@Test("Negative to UInt decoding error message regression test for issue #1604")
func negativeToUIntDecodingErrorMessage() throws {
struct Object: Decodable {
var foo: UInt
}

let json = """
{
"foo": -123
}
""".data(using: .utf8)!

do {
_ = try JSONDecoder().decode(Object.self, from: json)
Issue.record("Expected decoding error but decoding succeeded")
} catch let error as DecodingError {
guard case .dataCorrupted(let context) = error else {
Issue.record("Expected dataCorrupted error, got \(error)")
return
}

// Verification 1: Check if codingPath is correctly set
// Expected: ["foo"]
#expect(context.codingPath.count == 1, "codingPath should contain one element, got \(context.codingPath)")
if let key = context.codingPath.first {
#expect(key.stringValue == "foo", "codingPath should be ['foo'], got '\(key.stringValue)'")
}

// Verification 2: Check if debugDescription is appropriate
// Expected: A specific message like "Parsed JSON number <-123> does not fit in UInt."
#expect(
context.debugDescription.contains("-123"),
"debugDescription should mention the number -123, got: \(context.debugDescription)"
)
#expect(
context.debugDescription.contains("UInt") || context.debugDescription.contains("fit"),
"debugDescription should mention UInt or fit, got: \(context.debugDescription)"
)

// Verification 3 (macOS only): Check if underlyingError is provided
#if FOUNDATION_FRAMEWORK
#expect(context.underlyingError != nil, "macOS should provide underlyingError, got nil")

if let nsError = context.underlyingError as? NSError {
#expect(nsError.domain == NSCocoaErrorDomain, "underlyingError domain should be NSCocoaErrorDomain")
#expect(nsError.code == CocoaError.propertyListReadCorrupt.rawValue, "underlyingError code should match propertyListReadCorrupt")

// Verify the error message mentions the number
if let debugDesc = nsError.userInfo[NSDebugDescriptionErrorKey] as? String {
#expect(debugDesc.contains("-123"), "underlyingError should mention the number -123")
}
}
#endif
} catch {
Issue.record("Expected DecodingError, got \(type(of: error)): \(error)")
}
}

@Test func repeatedFailedNilChecks() {
struct RepeatNilCheckDecodable : Decodable {
enum Failure : Error {
Expand Down