From 2aedc9a8c8321c0a8e2410c8ec610f07800a6b50 Mon Sep 17 00:00:00 2001 From: Tito Ciuro Date: Fri, 19 May 2023 14:08:17 -0700 Subject: [PATCH 1/4] Fix error generation when status is .internal - when the error received is of type `.internal`, the current code rewrites the message with `descriptionForErrorCode`. This is pretty bad because any contextual information we might have received from Firebase is destroyed. - this fix attempts to extract the message from the response payload, preserving the original value. - if message is not found, the message is set to the value returned by `descriptionForErrorCode`, as was done before. --- FirebaseFunctions/Sources/FunctionsError.swift | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/FirebaseFunctions/Sources/FunctionsError.swift b/FirebaseFunctions/Sources/FunctionsError.swift index 6a2c52e1f74..698f20f5895 100644 --- a/FirebaseFunctions/Sources/FunctionsError.swift +++ b/FirebaseFunctions/Sources/FunctionsError.swift @@ -225,18 +225,18 @@ internal func FunctionsErrorForResponse(status: NSInteger, if let status = errorDetails["status"] as? String { code = FunctionsErrorCode.errorCode(forName: status) + if let message = errorDetails["message"] as? String { + description = message + } else { + description = code.descriptionForErrorCode + } + // If the code in the body is invalid, treat the whole response as malformed. guard code != .internal else { - return code.generatedError(userInfo: nil) + return code.generatedError(userInfo: [NSLocalizedDescriptionKey: description]) } } - - if let message = errorDetails["message"] as? String { - description = message - } else { - description = code.descriptionForErrorCode - } - + details = errorDetails["details"] as AnyObject? if let innerDetails = details { // Just ignore the details if there an error decoding them. From 5fb298ff2172a009dad1e76cbb5377205cdb6b07 Mon Sep 17 00:00:00 2001 From: Tito Ciuro Date: Fri, 19 May 2023 14:55:57 -0700 Subject: [PATCH 2/4] Run the style.sh script --- FirebaseFunctions/Sources/FunctionsError.swift | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/FirebaseFunctions/Sources/FunctionsError.swift b/FirebaseFunctions/Sources/FunctionsError.swift index 698f20f5895..cd205351e19 100644 --- a/FirebaseFunctions/Sources/FunctionsError.swift +++ b/FirebaseFunctions/Sources/FunctionsError.swift @@ -230,13 +230,13 @@ internal func FunctionsErrorForResponse(status: NSInteger, } else { description = code.descriptionForErrorCode } - + // If the code in the body is invalid, treat the whole response as malformed. guard code != .internal else { return code.generatedError(userInfo: [NSLocalizedDescriptionKey: description]) } } - + details = errorDetails["details"] as AnyObject? if let innerDetails = details { // Just ignore the details if there an error decoding them. From 89adc2a82388070352219cc7d81e35087998050d Mon Sep 17 00:00:00 2001 From: Tito Ciuro Date: Sun, 21 May 2023 15:23:03 -0700 Subject: [PATCH 3/4] Update unit tests to reflect the new logic proposal --- .../Tests/Integration/IntegrationTests.swift | 8 ++++---- .../Tests/ObjCIntegration/FIRIntegrationTests.m | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/FirebaseFunctions/Tests/Integration/IntegrationTests.swift b/FirebaseFunctions/Tests/Integration/IntegrationTests.swift index 089eafd926d..d2961b7a787 100644 --- a/FirebaseFunctions/Tests/Integration/IntegrationTests.swift +++ b/FirebaseFunctions/Tests/Integration/IntegrationTests.swift @@ -442,7 +442,7 @@ class IntegrationTests: XCTestCase { } catch { let error = error as NSError XCTAssertEqual(FunctionsErrorCode.internal.rawValue, error.code) - XCTAssertEqual("INTERNAL", error.localizedDescription) + XCTAssertNotEqual("INTERNAL", error.localizedDescription) expectation.fulfill() return } @@ -473,7 +473,7 @@ class IntegrationTests: XCTestCase { } catch { let error = error as NSError XCTAssertEqual(FunctionsErrorCode.internal.rawValue, error.code) - XCTAssertEqual("INTERNAL", error.localizedDescription) + XCTAssertNotEqual("INTERNAL", error.localizedDescription) } } } @@ -498,7 +498,7 @@ class IntegrationTests: XCTestCase { } catch { let error = error as NSError XCTAssertEqual(FunctionsErrorCode.internal.rawValue, error.code) - XCTAssertEqual("INTERNAL", error.localizedDescription) + XCTAssertNotEqual("INTERNAL", error.localizedDescription) expectation.fulfill() return } @@ -528,7 +528,7 @@ class IntegrationTests: XCTestCase { } catch { let error = error as NSError XCTAssertEqual(FunctionsErrorCode.internal.rawValue, error.code) - XCTAssertEqual("INTERNAL", error.localizedDescription) + XCTAssertNotEqual("INTERNAL", error.localizedDescription) } } } diff --git a/FirebaseFunctions/Tests/ObjCIntegration/FIRIntegrationTests.m b/FirebaseFunctions/Tests/ObjCIntegration/FIRIntegrationTests.m index 124f2eb9044..320768c79f7 100644 --- a/FirebaseFunctions/Tests/ObjCIntegration/FIRIntegrationTests.m +++ b/FirebaseFunctions/Tests/ObjCIntegration/FIRIntegrationTests.m @@ -226,7 +226,7 @@ - (void)testUnknownError { completion:^(FIRHTTPSCallableResult *_Nullable result, NSError *_Nullable error) { XCTAssertNotNil(error); XCTAssertEqual(FIRFunctionsErrorCodeInternal, error.code); - XCTAssertEqualObjects(@"INTERNAL", error.localizedDescription); + XCTAssertNotEqualObjects(@"INTERNAL", error.localizedDescription); [expectation fulfill]; }]; [self waitForExpectations:@[ expectation ] timeout:10]; From f094a904cccc98ccc4b53505e4ec0334bd5e66a4 Mon Sep 17 00:00:00 2001 From: Paul Beusterien Date: Tue, 23 May 2023 15:11:16 -0700 Subject: [PATCH 4/4] Update tests for exposing internal error description --- FirebaseFunctions/Backend/index.js | 2 +- FirebaseFunctions/CHANGELOG.md | 3 +++ .../Tests/Integration/IntegrationTests.swift | 8 ++++---- .../Tests/ObjCIntegration/FIRIntegrationTests.m | 2 +- 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/FirebaseFunctions/Backend/index.js b/FirebaseFunctions/Backend/index.js index 00d5d725bce..0098bf4e864 100644 --- a/FirebaseFunctions/Backend/index.js +++ b/FirebaseFunctions/Backend/index.js @@ -78,7 +78,7 @@ exports.unknownErrorTest = functions.https.onRequest((request, response) => { response.status(400).send({ error: { status: 'THIS_IS_NOT_VALID', - message: 'this should be ignored', + message: 'invalid response', }, }); }); diff --git a/FirebaseFunctions/CHANGELOG.md b/FirebaseFunctions/CHANGELOG.md index c156c6daffd..2ae2e887ade 100644 --- a/FirebaseFunctions/CHANGELOG.md +++ b/FirebaseFunctions/CHANGELOG.md @@ -1,3 +1,6 @@ +# 10.11.0 +- [fixed] Provide more detail for internal errors. (#11310) + # 10.10.0 - [fixed] Fixed potential memory leak of Functions instances. (#11248) - [added] Callable functions can now opt in to using limited-use App Check diff --git a/FirebaseFunctions/Tests/Integration/IntegrationTests.swift b/FirebaseFunctions/Tests/Integration/IntegrationTests.swift index d2961b7a787..da2d420f195 100644 --- a/FirebaseFunctions/Tests/Integration/IntegrationTests.swift +++ b/FirebaseFunctions/Tests/Integration/IntegrationTests.swift @@ -442,7 +442,7 @@ class IntegrationTests: XCTestCase { } catch { let error = error as NSError XCTAssertEqual(FunctionsErrorCode.internal.rawValue, error.code) - XCTAssertNotEqual("INTERNAL", error.localizedDescription) + XCTAssertEqual("INTERNAL", error.localizedDescription) expectation.fulfill() return } @@ -473,7 +473,7 @@ class IntegrationTests: XCTestCase { } catch { let error = error as NSError XCTAssertEqual(FunctionsErrorCode.internal.rawValue, error.code) - XCTAssertNotEqual("INTERNAL", error.localizedDescription) + XCTAssertEqual("INTERNAL", error.localizedDescription) } } } @@ -498,7 +498,7 @@ class IntegrationTests: XCTestCase { } catch { let error = error as NSError XCTAssertEqual(FunctionsErrorCode.internal.rawValue, error.code) - XCTAssertNotEqual("INTERNAL", error.localizedDescription) + XCTAssertEqual("invalid response", error.localizedDescription) expectation.fulfill() return } @@ -528,7 +528,7 @@ class IntegrationTests: XCTestCase { } catch { let error = error as NSError XCTAssertEqual(FunctionsErrorCode.internal.rawValue, error.code) - XCTAssertNotEqual("INTERNAL", error.localizedDescription) + XCTAssertEqual("invalid response", error.localizedDescription) } } } diff --git a/FirebaseFunctions/Tests/ObjCIntegration/FIRIntegrationTests.m b/FirebaseFunctions/Tests/ObjCIntegration/FIRIntegrationTests.m index 320768c79f7..a6fff65c04e 100644 --- a/FirebaseFunctions/Tests/ObjCIntegration/FIRIntegrationTests.m +++ b/FirebaseFunctions/Tests/ObjCIntegration/FIRIntegrationTests.m @@ -226,7 +226,7 @@ - (void)testUnknownError { completion:^(FIRHTTPSCallableResult *_Nullable result, NSError *_Nullable error) { XCTAssertNotNil(error); XCTAssertEqual(FIRFunctionsErrorCodeInternal, error.code); - XCTAssertNotEqualObjects(@"INTERNAL", error.localizedDescription); + XCTAssertEqualObjects(@"invalid response", error.localizedDescription); [expectation fulfill]; }]; [self waitForExpectations:@[ expectation ] timeout:10];