ADP-291: Document endpoints error codes in the API documentation #2258

hasufell · 2020-10-19T15:26:56Z

Issue Number

Implementation approach

This is all best-effort. There are no static checks to ensure we didn't miss anything. There's ongoing work at servant to make this possible: haskell-servant/servant#841
But even that would require a major refactor.

Testing all possible errors is also an option, but seems quite excessive.

The workflow for figuring out the error codes is roughly:

follow the entry point to the the Handler () function e.g.

listTransactions
    :: forall ctx s t k n. (ctx ~ ApiLayer s t k)
    => ctx
...
    -> Handler [ApiTransaction n]

check all liftHandler calls, which have functions with ExceptT as argument, e.g.:

listTransactions
    :: forall ctx s k t.
        ( HasDBLayer s k ctx
        , HasNetworkLayer t ctx
        )
    => ctx
...
    -> ExceptT ErrListTransactions IO [TransactionInfo]

find all the LiftHandler instance, e.g.

instance LiftHandler ErrListTransactions where
    handler = \case
        ErrListTransactionsNoSuchWallet e -> handler e
        ErrListTransactionsStartTimeLaterThanEndTime e -> handler e
        ErrListTransactionsMinWithdrawalWrong ->
            apiError err400 MinWithdrawalWrong
            "The minimum withdrawal value must be at least 1 Lovelace."
        ErrListTransactionsPastHorizonException e -> handler e

follow the recursive handlers to resolve all errors and add client error types to swagger.yaml. The error code is the 3rd ApiErrorCode argument to apiError.
Some errors are implicit, e.g. failed parameter parsing etc. These are also in ApiErrorCode. Also see the special instance instance LiftHandler (Request, ServerError)
repeat until exhaustion

Overview

Remarks

I did not double check the byron endpoints. Many of their endpoints share the same types in swagger.yaml, so I assumed they have the same behavior wrt error codes.
This will generally be hard to maintain, since yaml anchors aren't enough to express the overlaps of error codes and group them sensibly. It would need something like dhall to do that.
I removed 405 errors, because the HTTP method is part of the very spec. If you follow the spec, you can't get 405.

lib/core/src/Cardano/Wallet.hs

specifications/api/swagger.yaml

hasufell · 2020-10-29T14:06:34Z

lib/core/test/unit/Cardano/Wallet/Api/TypesSpec.hs

+                errStr = case res of
+                    Error s -> s
+                    _ -> ""
+            in counterexample errStr $ res == Success SchemaApiErrorCode


This shows a nice error message:

Cardano.Wallet.Api.Types Api Errors Every constructor from ApiErrorCode has a corresponding type in the schema FAILED [1] Failures: test/unit/Cardano/Wallet/Api/TypesSpec.hs:937:9: 1) Cardano.Wallet.Api.Types, Api Errors, Every constructor from ApiErrorCode has a corresponding type in the schema Falsifiable (after 1 test): Missing ApiErrorCode constructors for: ["RejectedTip","InvalidDelegationDiscovery","NotImplemented","WalletNotResponding","AddressAlreadyExists","UtxoTooSmall","WithdrawalNotWorth","PastHorizon","UnableToAssignInputOutput"] Each of these need a corresponding swagger type of the form: x-errConstructorName To rerun use: --match "/Cardano.Wallet.Api.Types/Api Errors/Every constructor from ApiErrorCode has a corresponding type in the schema/" Randomized with seed 1387792531

…n which they may occur. Also, I've re-organized a bit their declarations so that they are listed in the same order as the branches on their corresponding sum-type in Haskell, so it's easier to map from one to another.

hasufell · 2020-10-30T10:41:02Z

bors r+

2258: ADP-291: Document endpoints error codes in the API documentation r=hasufell a=hasufell # Issue Number ADP-291 # Implementation approach This is all best-effort. There are no static checks to ensure we didn't miss anything. There's ongoing work at servant to make this possible: haskell-servant/servant#841 But even that would require a major refactor. Testing all possible errors is also an option, but seems quite excessive. The workflow for figuring out the error codes is roughly: 1. follow the entry point to the the `Handler ()` function e.g. ```hs listTransactions :: forall ctx s t k n. (ctx ~ ApiLayer s t k) => ctx ... -> Handler [ApiTransaction n] ``` 2. check all `liftHandler` calls, which have functions with `ExceptT` as argument, e.g.: ```hs listTransactions :: forall ctx s k t. ( HasDBLayer s k ctx , HasNetworkLayer t ctx ) => ctx ... -> ExceptT ErrListTransactions IO [TransactionInfo] ``` 3. find all the LiftHandler instance, e.g. ```hs instance LiftHandler ErrListTransactions where handler = \case ErrListTransactionsNoSuchWallet e -> handler e ErrListTransactionsStartTimeLaterThanEndTime e -> handler e ErrListTransactionsMinWithdrawalWrong -> apiError err400 MinWithdrawalWrong "The minimum withdrawal value must be at least 1 Lovelace." ErrListTransactionsPastHorizonException e -> handler e ``` 4. follow the recursive handlers to resolve all errors and add client error types to `swagger.yaml`. The error code is the 3rd `ApiErrorCode` argument to `apiError`. 5. Some errors are implicit, e.g. failed parameter parsing etc. These are also in `ApiErrorCode`. Also see the special instance `instance LiftHandler (Request, ServerError)` 6. repeat until exhaustion # Overview - [x] Wallets - [x] Addresses - [x] Coin Selections - [x] Transactions - [x] Migrations - [x] Stake Pools - [x] Utils - [x] Network - [x] Proxy - [x] Settings - [ ] Byron-specific endpoints # Remarks 1. I did not double check the byron endpoints. Many of their endpoints share the same types in `swagger.yaml`, so I assumed they have the same behavior wrt error codes. 2. This will generally be hard to maintain, since yaml anchors aren't enough to express the overlaps of error codes and group them sensibly. It would need something like [dhall](https://github.com/dhall-lang/dhall-lang) to do that. 3. I removed 405 errors, because the HTTP method is part of the very spec. If you follow the spec, you can't get 405.  Co-authored-by: Julian Ospald <julian.ospald@iohk.io> Co-authored-by: KtorZ <matthias.benkort@gmail.com>

piotr-iohk

Good work on documenting error messages. I've left few comments.

piotr-iohk · 2020-10-30T10:03:31Z

specifications/api/swagger.yaml

+  properties:
+    message:
+      type: string
+      description: May occur when trying to forget a transaction that is not pending.


Just to note this will change after https://github.com/input-output-hk/cardano-wallet/pull/2262/files#diff-a2398827a22606930a9b4edc67dfc9c99196a94cdb56c56ef0ef5f92512d42e0R2467 merged. Also expired transaction will be "forgettable".

piotr-iohk · 2020-10-30T10:10:47Z

specifications/api/swagger.yaml

+        schema:
+          oneOf:
+            - <<: *errBadRequest
+            - <<: *errTransactionIsTooBig


I think this should be only in 403?

ErrSelectForPaymentTxTooLarge maxSize maxN -> apiError err400 TransactionIsTooBig $ mconcat [ "I am afraid that the transaction you're trying to submit is " , "too large! The network allows transactions only as large as " , pretty maxSize, "s! As it stands, the current transaction only " , "allows me to select up to ", showT maxN, " inputs. Note " , "that I am selecting inputs randomly, so retrying *may work* " , "provided I end up choosing bigger inputs sufficient to cover " , "the transaction cost. Alternatively, try sending to less " , "recipients or with smaller metadata." ]

it is 400

piotr-iohk · 2020-10-30T10:25:50Z

specifications/api/swagger.yaml

+    content:
+      application/json:
+        schema:
+          <<: *errNoSuchWallet


Same 410 and errNoSuchWallet.

It is 410

ErrSignPaymentNoSuchWallet e -> (handler e) { errHTTPCode = 410 , errReasonPhrase = errReasonPhrase err410 }

piotr-iohk · 2020-10-30T10:36:14Z

specifications/api/swagger.yaml

@@ -1908,19 +2401,12 @@ x-responsesErr404: &responsesErr404
      application/json:
        schema: *responsesErr

-x-responsesErr405: &responsesErr405


Do we think it is irrelevant to highlight in API doc? Basically every endpoint may return this...

I don't think so. The spec includes which method to use. If you follow the spec, 405 will never be returned.

Then 406 and 415 should be removed also I guess, no?

415 maybe, but the swagger spec doesn't document enough for 406 to not be a concern afais.

piotr-iohk · 2020-10-30T10:41:14Z

specifications/api/swagger.yaml

+    content:
+      application/json:
+        schema:
+          <<: *errNoSuchWallet


Same 410 and errNoSuchWallet.

ErrSignPaymentNoSuchWallet e -> (handler e) { errHTTPCode = 410 , errReasonPhrase = errReasonPhrase err410 }

piotr-iohk · 2020-10-30T10:41:41Z

specifications/api/swagger.yaml

+    content:
+      application/json:
+        schema:
+          <<: *errNoSuchWallet


This doesn't feel right to me. I think we should use 404 on 'no such wallet'. Especially looking at some endpoints where:

This PR isn't really about refactoring or fixing our error codes though. I just added documentation to the existing error handlers. I changed nothing.

ACK. It revealed those small inconsistencies though (I spotted 410 and 403 only).
Could be fixed in this go if it's not a lot of work, but don't have to of course.

piotr-iohk · 2020-10-30T10:42:53Z

specifications/api/swagger.yaml

+    content:
+      application/json:
+        schema:
+          <<: *errNoSuchWallet


Same 410 and errNoSuchWallet.

ErrSignPaymentNoSuchWallet e -> (handler e) { errHTTPCode = 410 , errReasonPhrase = errReasonPhrase err410 }

hasufell · 2020-10-30T10:51:27Z

bors r-

iohk-bors · 2020-10-30T10:51:29Z

Canceled.

hasufell · 2020-11-02T09:35:48Z

bors r+

iohk-bors · 2020-11-02T11:08:39Z

Build succeeded:

2293: Revise API error codes r=piotr-iohk a=piotr-iohk # Issue Number ADP-291 # Overview - f533091 Revise error codes: - 404 when wallet not found - 403 when transaction is too big - 430a31f Update swagger - e90b264 Update integration tests # Comments Whie reviewing #2258 I noticed that some error codes are a bit inconsistent, e.g.: - 410 or 404 is returned when the wallet is not found. - 400 or 403 - when transaction is too big. This pr is about to revise it and: - return 404 on wallet not found (removing 410 altogether) - return 403 on transaction too big Co-authored-by: Piotr Stachyra <piotr.stachyra@iohk.io>

hasufell self-assigned this Oct 19, 2020

hasufell marked this pull request as draft October 19, 2020 15:27

hasufell changed the title ~~Document endpoints error codes in the API documentation~~ [WIP] Document endpoints error codes in the API documentation Oct 19, 2020

hasufell changed the title ~~[WIP] Document endpoints error codes in the API documentation~~ [WIP] ADP-291: Document endpoints error codes in the API documentation Oct 19, 2020

hasufell force-pushed the hasufell/ADP-291/document-api-endpoints branch 3 times, most recently from d2aee15 to aa7650a Compare October 20, 2020 09:40

hasufell marked this pull request as ready for review October 20, 2020 09:41

hasufell changed the title ~~[WIP] ADP-291: Document endpoints error codes in the API documentation~~ ADP-291: Document endpoints error codes in the API documentation Oct 20, 2020

hasufell force-pushed the hasufell/ADP-291/document-api-endpoints branch from aa7650a to ff52b1f Compare October 20, 2020 09:48

KtorZ reviewed Oct 20, 2020

View reviewed changes

lib/core/src/Cardano/Wallet.hs Show resolved Hide resolved

specifications/api/swagger.yaml Show resolved Hide resolved

specifications/api/swagger.yaml Outdated Show resolved Hide resolved

hasufell force-pushed the hasufell/ADP-291/document-api-endpoints branch from ff52b1f to 623af75 Compare October 26, 2020 12:20

hasufell requested a review from KtorZ October 26, 2020 12:20

hasufell force-pushed the hasufell/ADP-291/document-api-endpoints branch from 623af75 to 1203859 Compare October 26, 2020 16:13

KtorZ force-pushed the hasufell/ADP-291/document-api-endpoints branch from 1203859 to 1ddd32b Compare October 26, 2020 17:17

hasufell commented Oct 29, 2020

View reviewed changes

KtorZ approved these changes Oct 30, 2020

View reviewed changes

Julian Ospald and others added 6 commits October 30, 2020 11:12

Remove unused ErrSelectForDelegationUnableToAssignInputs

d66e0ba

Document endpoints error codes in the API documentation

d98c0af

Apply review suggestions

1404bd1

remove unused error codes and error definitions from the server.

9e3e895

Add automated testing for 'ApiErrorCode'

7225066

hasufell force-pushed the hasufell/ADP-291/document-api-endpoints branch from c215dce to 7225066 Compare October 30, 2020 10:38

piotr-iohk reviewed Oct 30, 2020

View reviewed changes

iohk-bors bot merged commit 9c56a88 into master Nov 2, 2020

iohk-bors bot deleted the hasufell/ADP-291/document-api-endpoints branch November 2, 2020 11:08

This was referenced Nov 4, 2020

Revise API error codes #2293

Merged

ADP-494 - Add POST endpoint for script address #2253

Merged

Anviking mentioned this pull request Nov 9, 2020

Flaky test - various DB properties causing timeout #2292

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ADP-291: Document endpoints error codes in the API documentation #2258

ADP-291: Document endpoints error codes in the API documentation #2258

hasufell commented Oct 19, 2020 •

edited

Loading

hasufell Oct 29, 2020

hasufell commented Oct 30, 2020

piotr-iohk left a comment

piotr-iohk Oct 30, 2020

piotr-iohk Oct 30, 2020

hasufell Oct 30, 2020

piotr-iohk Oct 30, 2020

hasufell Oct 30, 2020

piotr-iohk Oct 30, 2020

hasufell Oct 30, 2020

piotr-iohk Oct 30, 2020

hasufell Oct 30, 2020

piotr-iohk Oct 30, 2020

hasufell Oct 30, 2020

piotr-iohk Oct 30, 2020

hasufell Oct 30, 2020

piotr-iohk Oct 30, 2020

piotr-iohk Oct 30, 2020

hasufell Oct 30, 2020

hasufell commented Oct 30, 2020

iohk-bors bot commented Oct 30, 2020

hasufell commented Nov 2, 2020

iohk-bors bot commented Nov 2, 2020

ADP-291: Document endpoints error codes in the API documentation #2258

ADP-291: Document endpoints error codes in the API documentation #2258

Conversation

hasufell commented Oct 19, 2020 • edited Loading

Issue Number

Implementation approach

Overview

Remarks

Choose a reason for hiding this comment

hasufell commented Oct 30, 2020

piotr-iohk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hasufell commented Oct 30, 2020

iohk-bors bot commented Oct 30, 2020

hasufell commented Nov 2, 2020

iohk-bors bot commented Nov 2, 2020

hasufell commented Oct 19, 2020 •

edited

Loading