API Client のエラー処理の改善 by shoe116 · Pull Request #91 · BuffettCode/buffett-code-api-client-excel

shoe116 · 2022-07-17T03:28:40Z

to close #51

shoe116

@akiomik お願いします

shoe116 · 2022-07-17T04:14:31Z

-                    {
-                        case (int)HttpStatusCode.Forbidden:
-                            throw new InvalidAPIKeyException($"request={request}");
-                        case (int)HttpStatusCode.NotFound:
-                            throw new ResourceNotFoundException($"request={request}");
-                        case 429: // Quota Error
-                            throw new QuotaException($"request={request}");
-                        default:
-                            throw new BuffettCodeApiClientException($"request={request}");
-                    }
+                    GetErrorResponseHandler.Handle(request, response);


getのエラーレスポンスの処理はこいつに任せます

ちょっと名前が直感的じゃなく感じた。たぶん Get がどこにかかっているのかわからないから、「エラーレスポンスのハンドラ」を取得するように見える

「GETリクエストのエラー時のハンドラ」なら、 GetReqeustErrorHandler がわかりやすいかなと思うんだけどどうだろう (同じ問題はあるけど、 GetRequest という言葉はあるからまだましかなと)

わかる　GetRequestError にするわ

rename 07c27bb

shoe116 · 2022-07-17T04:15:39Z

+        Forbidden = HttpStatusCode.Forbidden,
+        NotFound = HttpStatusCode.NotFound,
+        BadRequest = HttpStatusCode.BadRequest,
+        ExceedingApiGatewayQuota = 429,


こいつは今使ってる HttpStatusCode に未定義でした、新しいやつには入ってるっぽいので .NET あげたら対応します
https://docs.microsoft.com/en-us/dotnet/api/system.net.httpstatuscode?view=net-6.0

どのみち新しい.netframework入れたら TooManyRequests にするんだろうから、素直に TooManyRequests という名前にした方がいいと思う。もし ExceedingApiGatewayQuota を使いたいなら、 Forbidden や BadRequest をapplication specificな名前に変更した方がいいと思う

ああ、たしかに。

shoe116 · 2022-07-17T04:16:05Z

+                        case "{\"message\":\"Forbidden\"}":
+                            throw new InvalidAPIKeyException($"request={request}");


403 かつこのメッセージの時のみ InvalidAPIKeyException

shoe116 · 2022-07-17T04:16:23Z

+                    if (TestApiTokenErrorMessage.KnownErrorMessages.Contains(errorResponse.Content.ReadAsStringAsync().Result))
+                    {


メッセージ完全一致にしました

CSむずかしすぎる… ReadAsStringAsync() ってどうasyncなの？
普通のプログラミング言語なら、コールバックを使うなり await するなりでまたなきゃいけないと思うけど、呼び出しを見ると同期してるように見える

これまじで・・・もちろん await しないと使えないんだけど、 Result すると Task じゃなくて string がそのまま返ってくるのよね（詳細は追ってない

なるほど

shoe116 · 2022-07-17T04:18:16Z

+                    }
+                    else
+                    {
+                        throw new BuffettCodeApiClientException($"request={request}");


知らないメッセージの 400 は BuffettCodeApiClientException

akiomik · 2022-07-17T05:38:48Z

+            switch (errorResponse.Content.ReadAsStringAsync().Result)
+            {
+                case "{\"message\":\"Forbidden\"}":
+                    return new InvalidAPIKeyException($"request={request}");


ネストしてるやつは return するようにしたのね

ですです

shoe116 · 2022-07-17T06:38:16Z

 Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "BuffettCodeAddinRibbonTests", "BuffettCodeAddinRibbonTests\BuffettCodeAddinRibbonTests.csproj", "{00A6A593-A87D-4DD2-8B06-8A36CFC6C3AA}"
 EndProject
 Project("{930C7802-8A8C-48F9-8165-68863BCCD9DD}") = "SetupAddinRibbon32", "SetupAddinRibbon32\SetupAddinRibbon32.wixproj", "{E1016C59-0448-4BFA-B89D-BB3C960F1F16}"
+	ProjectSection(ProjectDependencies) = postProject


build が不安定だったので（成果物がないって言われる）、 Setup 系の build は各テストが終わってからにします

見てる感じ、これでだいぶ安定した（なんでや・・・

shoe116 · 2022-07-17T06:38:35Z

+            switch (errorResponse.Content.ReadAsStringAsync().Result)
+            {
+                case "{\"message\":\"Forbidden\"}":
+                    return new InvalidAPIKeyException($"request={request}");


ですです

shoe116 · 2022-07-17T06:40:20Z

+                return new TestAPIConstraintException($"request={request}");
+            }
+            else
+            {
+                return new BuffettCodeApiClientException($"request={request}");


こっちも return するようにしてあります

shoe116 · 2022-07-17T06:41:53Z

+
+    static class InvalidAPIKeyErrorMessage
+    {
+        public const string ApiGatewayDefault = "{\"message\":\"Forbidden\"}";


名前にまよったけど、一番現実に即した名前を付けてあります

shoe116 · 2022-07-17T06:50:30Z

    }

-    public class QuotaException : BuffettCodeApiClientException
+    public class DailyQuotaException : BuffettCodeApiClientException


AWS Gateway の daily quota によるものなので、 DailyQuotaException に rename しました

shoe116 · 2022-07-17T06:50:41Z

+            else if (bce is DailyQuotaException)
            {
-                message = "APIの実行回数が上限に達しました";
+                message = "今日のAPIの実行回数が上限に達しました";


「今日の」をつけた

akiomik

色々対応ありがとうございました 🙏

shoe116 commented Jul 17, 2022

View reviewed changes