fix(Predictions): Callback is not triggered with URLError #896

618coffee · 2020-11-17T19:05:16Z

Description of changes:
PR is a fix for #678

When using schema with unsupported region, which means the host generated cannot be reached and causes a url failure.
When the device is not connected to Internet, URLError is also going to be returned

In the above cases, the Callback with error info is not triggered because it is skipping the error check.

The fix is:

Check what type of error return back and avoid calling the next listen() if is of type "NSURLErrorDomain".
Trigger callback in failure case.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

codecov · 2020-11-17T19:15:28Z

Codecov Report

Merging #896 (81ec2e5) into main (d29ecdd) will increase coverage by 0.06%.
The diff coverage is 75.80%.

@@            Coverage Diff             @@
##             main     #896      +/-   ##
==========================================
+ Coverage   67.29%   67.35%   +0.06%     
==========================================
  Files         895      895              
  Lines       35389    35448      +59     
==========================================
+ Hits        23814    23875      +61     
+ Misses      11575    11573       -2

Flag	Coverage Δ
API_plugin_unit_test	`62.56% <ø> (+0.07%)`	⬆️
Analytics_plugin_unit_test	`72.38% <ø> (ø)`
Auth_plugin_unit_test	`48.62% <ø> (ø)`
DataStore_plugin_unit_test	`83.03% <ø> (ø)`
Predictions_plugin_unit_test	`50.43% <75.80%> (+0.73%)`	⬆️
Storage_plugin_unit_test	`74.74% <ø> (ø)`
build_test_amplify	`63.08% <ø> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...gin/Support/Internal/NativeWebSocketProvider.swift	`15.87% <0.00%> (+2.53%)`	⬆️
...sPlugin/Support/Utils/PredictionsErrorHelper.swift	`40.62% <61.90%> (+5.95%)`	⬆️
...ctionsTest/PredictionsServiceTranscribeTests.swift	`92.70% <81.25%> (-1.52%)`	⬇️
...Predictions/AWSPredictionsService+Transcribe.swift	`82.35% <88.88%> (+27.35%)`	⬆️
...ocks/Service/MockTranscribeStreamingBehavior.swift	`92.59% <100.00%> (+17.59%)`	⬆️
...Hub/DefaultPluginTests/DefaultHubPluginTests.swift	`83.72% <0.00%> (-4.66%)`	⬇️
.../DataStore/Model/Internal/Schema/ModelSchema.swift	`85.29% <0.00%> (-2.95%)`	⬇️
...s/AWSHubPlugin/Internal/HubChannelDispatcher.swift	`94.11% <0.00%> (-2.95%)`	⬇️
...lugin/Operation/AWSAPIOperation+APIOperation.swift	`83.78% <0.00%> (+5.40%)`	⬆️
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d29ecdd...f5f3689. Read the comment docs.

.../Predictions/AWSPredictionsPlugin/Service/Predictions/AWSPredictionsService+Transcribe.swift

618coffee · 2020-11-17T22:05:51Z

AmplifyPlugins/Predictions/AWSPredictionsPlugin/Support/Internal/NativeWebSocketProvider.swift

                    self.clientDelegate.connectionStatusDidChange(status, withError: error)
                }
+
+                guard nsError?.domain != NSURLErrorDomain else {


If the error is of type NSURLErrorDomain, we do not want to keep calling listen(), we should just return.

Probably

nsError?.code == -1003

should also be checked? https://stackoverflow.com/questions/27258702/error-domain-nsurlerrordomain-code-1003-a-server-with-the-specified-hostname-c

Let's not use -1003 as a literal, though. Use the appropriate error code

.../Predictions/AWSPredictionsPlugin/Service/Predictions/AWSPredictionsService+Transcribe.swift

AmplifyPlugins/Predictions/AWSPredictionsPlugin/Support/Internal/NativeWebSocketProvider.swift

palpatim

Comments below. Do you have unit tests to cover this case?

.../Predictions/AWSPredictionsPlugin/Service/Predictions/AWSPredictionsService+Transcribe.swift

palpatim · 2020-11-18T00:10:05Z

AmplifyPlugins/Predictions/AWSPredictionsPlugin/Support/Internal/NativeWebSocketProvider.swift

            switch result {
            case .failure(let error):
                let status = AWSTranscribeStreamingClientConnectionStatus.closed
+                let nsError = error as NSError?


Error isn't nil, so you should be able to do let nsError = error as NSError, and do away with the optional chaining. My recollection is that Swift.Error always successfully casts to NSError

Removing ? produces

'Error?' is not convertible to 'NSError'; did you mean to use 'as!' to force downcast?

and force cast is also not allowed.

if you can't, i would go with

if let error = error as NSError? { }

palpatim · 2020-11-18T00:10:47Z

AmplifyPlugins/Predictions/AWSPredictionsPlugin/Support/Internal/NativeWebSocketProvider.swift

                    self.clientDelegate.connectionStatusDidChange(status, withError: error)
                }
+
+                guard nsError?.domain != NSURLErrorDomain else {


Let's not use -1003 as a literal, though. Use the appropriate error code

.../Predictions/AWSPredictionsPlugin/Service/Predictions/AWSPredictionsService+Transcribe.swift

lawmicha · 2020-11-19T18:25:45Z

AmplifyPlugins/Predictions/AWSPredictionsPlugin/Support/Internal/NativeWebSocketProvider.swift

            switch result {
            case .failure(let error):
                let status = AWSTranscribeStreamingClientConnectionStatus.closed
+                let nsError = error as NSError?


if you can't, i would go with

if let error = error as NSError? { }

...ns/AWSPredictionsPluginTests/Service/PredictionsTest/PredictionsServiceTranscribeTests.swift

AmplifyPlugins/Predictions/AWSPredictionsPlugin/Support/Utils/PredictionsErrorHelper.swift

...ns/AWSPredictionsPluginTests/Service/PredictionsTest/PredictionsServiceTranscribeTests.swift

…PredictionsErrorHelper.swift Co-authored-by: Michael Law <1365977+lawmicha@users.noreply.github.com>

...ns/AWSPredictionsPluginTests/Service/PredictionsTest/PredictionsServiceTranscribeTests.swift

.../Predictions/AWSPredictionsPlugin/Service/Predictions/AWSPredictionsService+Transcribe.swift

lawmicha · 2020-12-03T23:24:25Z

AmplifyPlugins/Predictions/AWSPredictionsPlugin/Support/Utils/PredictionsErrorHelper.swift

        }
    }

+    static func mapError(_ error: NSError) -> PredictionsError {


case NSURLErorrDomain: guard let urlError = error as? URLError else { return defaultError } return .network(description, recovery, urlError)

lawmicha · 2020-12-03T23:25:01Z

...ns/AWSPredictionsPluginTests/Service/PredictionsTest/PredictionsServiceTranscribeTests.swift

+            case .completed(let result):
+                XCTFail("Should not produce result: \(result)")
+            case .failed(let error):
+                XCTAssertNotNil(error, "Should produce an error")


minor: assert it is a network error, then assert underlying error is .cannotFindHost

lawmicha · 2020-12-08T23:16:58Z

AmplifyPlugins/Predictions/AWSPredictionsPlugin/Support/Utils/PredictionsErrorHelper.swift

            }
            return AWSTranscribeStreamingErrorMessage.map(errorType) ?? defaultError
+        case NSURLErrorDomain:
+            return mapError(error)


can cast to URLError now that you know it is NSURLErrorDomain, then call a specific method that takes in URLError as a parameter

lawmicha · 2020-12-08T23:17:52Z

AmplifyPlugins/Predictions/AWSPredictionsPlugin/Support/Utils/PredictionsErrorHelper.swift

        }
    }

+    static func mapError(_ nsError: NSError) -> PredictionsError {


this is more like mapURLError(_ urlError: URLError)

618coffee self-assigned this Nov 17, 2020

618coffee requested review from diegocstn, drochetti, lawmicha, palpatim, royjit and wooj2 November 17, 2020 21:55

618coffee commented Nov 17, 2020

View reviewed changes

.../Predictions/AWSPredictionsPlugin/Service/Predictions/AWSPredictionsService+Transcribe.swift Outdated Show resolved Hide resolved

618coffee commented Nov 17, 2020

View reviewed changes

lawmicha reviewed Nov 17, 2020

View reviewed changes

palpatim reviewed Nov 18, 2020

View reviewed changes

618coffee marked this pull request as draft November 18, 2020 19:32

lawmicha reviewed Nov 19, 2020

View reviewed changes

618coffee added 4 commits November 25, 2020 15:04

initialize fix for transcribe callback

06c9f83

added error code checking & added a test

6c2d438

trying to resolve another bug, offline should return error

b1765fa

rebase from main

b571032

618coffee force-pushed the fix/transcribewithunsupportedregion branch from 005deea to b571032 Compare November 30, 2020 18:03

618coffee added 4 commits November 30, 2020 10:27

revert podfile change

d821084

revert podfile change

9f6a042

Updated unit tests

4c9951d

removed connectionError

89d5da7

lawmicha reviewed Dec 2, 2020

View reviewed changes

618coffee and others added 2 commits December 2, 2020 09:02

addressed some PR comments

08da2b9

Update AmplifyPlugins/Predictions/AWSPredictionsPlugin/Support/Utils/…

7145b8b

…PredictionsErrorHelper.swift Co-authored-by: Michael Law <1365977+lawmicha@users.noreply.github.com>

lawmicha reviewed Dec 2, 2020

View reviewed changes

...ns/AWSPredictionsPluginTests/Service/PredictionsTest/PredictionsServiceTranscribeTests.swift Outdated Show resolved Hide resolved

Updated a unit test

2150c54

618coffee marked this pull request as ready for review December 2, 2020 18:02

lawmicha reviewed Dec 3, 2020

View reviewed changes

.../Predictions/AWSPredictionsPlugin/Service/Predictions/AWSPredictionsService+Transcribe.swift Outdated Show resolved Hide resolved

lawmicha reviewed Dec 3, 2020

View reviewed changes

updated an iterated issue

81ec2e5

618coffee changed the title ~~fix(Predictions): Callback is not triggered with unsupported region~~ fix(Predictions): Callback is not triggered with URLError Dec 4, 2020

lawmicha reviewed Dec 8, 2020

View reviewed changes

updated PredictionsErrorHelper

f5f3689

lawmicha approved these changes Dec 9, 2020

View reviewed changes

618coffee merged commit 17285d1 into main Dec 9, 2020

618coffee deleted the fix/transcribewithunsupportedregion branch December 9, 2020 00:41

fix(Predictions): Callback is not triggered with URLError #896

fix(Predictions): Callback is not triggered with URLError #896

Uh oh!

Conversation

618coffee commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

618coffee Nov 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

palpatim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lawmicha Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

618coffee commented Nov 17, 2020 •

edited

Loading

codecov bot commented Nov 17, 2020 •

edited

Loading

618coffee Nov 17, 2020 •

edited

Loading

lawmicha Dec 3, 2020 •

edited

Loading