[CBR-26] Error Handling Improvements #3429

KtorZ · 2018-08-17T14:50:31Z

Description

I've resurrected this branch from the abyss.
In theory, everything in there have been already reviewed but the rebase took me a few hours to complete with quite a lot of conflicts. @parsonsmatt, I'll have an extra look at the github diff, but I'd appreciate you have one too :)

Linked issue

[CBR-26]

Type of change

🐞 Bug fix (non-breaking change which fixes an issue)
🛠 New feature (non-breaking change which adds functionality)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)
🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
🔨 New or improved tests for existing code
⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

I have read the style guide document, and my code follows the code style of this project.
If my code deals with exceptions, it follows the guidelines.
I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

I have added tests to cover my changes.
All new and existing tests passed.

QA Steps

Screenshots (if available)

KtorZ · 2018-08-17T15:31:27Z

wallet-new/src/Cardano/Wallet/API/Response.hs

+            fail "Incorrect JSON encoding for JSONValidationError"
+
+    parseJSON invalid =
+        typeMismatch "JSONValidationError" invalid


No usage of generics here, to have consistent approach with the work recently done on Errors.

Note that withObject does the pattern match/note dance for you

KtorZ · 2018-08-17T15:31:59Z

wallet-new/src/Cardano/Wallet/API/V1/Errors.hs

-import qualified Pos.Core.Attributes as Core
-import qualified Pos.Crypto.Hashing as Crypto
-import           Pos.Util.Util (aesonError)
-


Everything moved to V1/Types.hs

KtorZ · 2018-08-17T15:39:47Z

wallet-new/src/Cardano/Wallet/API/V1/Generic.hs

 import           Data.List ((!!))
-import           Pos.Util.Util (aesonError)
-
--


Now that WalletError JSON instances are defined by hand, we don't need all the sop-generic stuff. I'd be more than happy to re-introduce generics here if we can find a way to do so without relying on partial field accessors.

KtorZ · 2018-08-17T15:40:19Z

wallet-new/src/Cardano/Wallet/API/V1/LegacyHandlers/Transactions.hs

+    V0.SignedTxInvalidSignature txt ->
+        SignedTxSubmitError $ sformat build (V0.SignedTxInvalidSignature txt)
+    V0.GeneralTxError txt ->
+        UnknownError txt


Move from API/V1/Errors.hs because used only here.

KtorZ · 2018-08-17T15:40:56Z

wallet-new/src/Cardano/Wallet/API/V1/Migration/Types.hs

+            fail "Incorrect JSON encoding for MigrationError"
+
+    parseJSON invalid =
+        typeMismatch "MigrationError" invalid


idem JSONValidationError

KtorZ · 2018-08-17T15:41:45Z

wallet-new/src/Cardano/Wallet/API/V1/Swagger.hs

+    , mkRow errToDescription AddressNotFound
+    , mkRow errToDescription $ MissingRequiredParams (("wallet_id", "walletId") :| [])
+    , mkRow errToDescription $ WalletIsNotReadyToProcessPayments sampleSyncProgress
+    , mkRow errToDescription $ NodeIsStillSyncing (mkSyncPercentage 42)


Meh. This lacks some of the available errors. Would be nice to have a generic way to generate those, or if not, something that will crash at runtime upon starting if one of them is missing though lazyness doesn't serve us well here.

KtorZ · 2018-08-17T15:43:27Z

wallet-new/src/Cardano/Wallet/API/V1/Types.hs

+            quantity <- parseJSON quantityO
+            return . NodeIsStillSyncing $ mkSyncPercentage quantity
+        | otherwise = fail "Incorrect JSON encoding for WalletError"
+    parseJSON invalid = typeMismatch "WalletError" invalid


The only changes compare to the base branch here are:

Dropped usage of Pos.Util.Util.aesonError in favor of fail which is doing pretty-much the same thing without 454325 level of indirections.

Remove the MigrationError & JSONValidationError which are now standalone types.

KtorZ · 2018-08-17T15:43:57Z

wallet-new/test.md

+sklfjs fsdkfjsd fsdkfjsdfksdf sdfkljsdfs dfksdjfsdfsd fsldfmsdf sfksdflksdf
+sdfksdjfklsd fsdkfjsdklfs fsdkfsdkfsd fksdjfsdf flksjfksd fsfkjsdfkd fkdjopqpw
+poxvn slakfps masmpxx lsfjn dfjdj aod aal do akdnf aod da pd diq didbdh die  oa
+aidbna dla sja da diand saa dsoa aodjs akisopa dka' 


parsonsmatt · 2018-08-17T21:10:14Z

I have "reviewed" it by implementing all the changes I would have done.

KtorZ · 2018-08-18T07:14:08Z

wallet-new/src/Cardano/Wallet/API/Response.hs

+                err <- diag .: "validationError"
+                pure $ JSONValidationFailed err
+            _ ->
+                fail "Incorrect JSON encoding for JSONValidationError"


ruhatch

LGTM!

* [CO-319] Fix account index swagger example * [CO-319] Add roundtrip tests * [CO-319] Fix recursive buildable instances * [CO-319] Use strongly typed error * [CO-319] Remove duplication in 'renderAccountIndexError' * [CO-319] Distangle V1/Errors This makes it now possible to import V1/Errors from the V1/Types module and leverage errors from this module. One thing is still unclear to me: Why Errors isn't defined in V1/Types already? There's a circular dependency between V1/Response and V1/Types if we go this way, as well as between V1/Migration and V1/Types. Nevertheless, it would make sense to have three data-types here: - WalletError (defined in V1/Types) - MigrationError (defined in V1/Types) - JSONParsingError (defined in Response) This way, we could remove the conflicting constructor from WalletError and remove the need for an extra module here. It will also makes thing clearer * [CO-319] Make V1/Error part of V1/Types To realize this, we had to extract JSONValidationFailed and MigrationFailed constructor from WalletError. They're now defined as constructor in different data-types (resp. JSONValidationError and MigrationError). * [CO-319] Solve rebase conflicts * [CO-319] Correctly format (jsend) newtype errors This is rather ugly and could probably be achieved nicely with a better understanding of the Generics.SOP library. As far as I could tell, there's no easy way to retrieve 'Tag' for single constructor (cf: 'For a datatype with a single constructor we do not need to tag values with their constructor; but for a datatype with multiple constructors we do. ')

We used to rely on this code solely for 'WalletError' instances. Though, we now declare those instances by hand using manual encoding. We might want to re-introduce generics is possible in the future. Note that we switch for a manual implementation because the current generics implementation relied on partial field accessors in the underlying types which we try to avoid. Another approach using generics without the need for partial field accessors would make everyone happier

KtorZ · 2018-08-18T17:00:30Z

wallet-new/src/Cardano/Wallet/API/V1/Generic.hs

+    gDiagnosticToJSON k (K1 c) = object [ k .= c ]
+
+instance GDiagnosticToJSON U1 where
+    gDiagnosticToJSON _ _ = object mempty


Gotta trust the tests on that one I guess.
The diff is better visualized when looking at commits:

first, the old implementation using json-sop was removed (1bd80be)

and later, a new implementation which doesn't rely on partial field accessor was done (72a608e)

KtorZ · 2018-08-18T17:01:05Z

wallet-new/src/Cardano/Wallet/API/V1/Swagger.hs

-                  -> Declare (Definitions Schema) NamedSchema
-fromExampleJSON (_ :: proxy a) = do
-    let (randomSample :: a) = genExample
-    return $ NamedSchema (Just $ fromString $ show $ typeOf randomSample) (sketchSchema randomSample)


Moved into /Swagger/Example.hs

KtorZ · 2018-08-18T17:01:50Z

wallet-new/src/Cardano/Wallet/API/V1/Swagger.hs

+          Core.mkAttributes $ Core.AddrAttributes Nothing Core.BootstrapEraDistr
+      , Core.addrType =
+          Core.ATPubKey
+      }


We need to define this as a proper Example

KtorZ · 2018-08-18T17:03:19Z

wallet-new/test/WalletNewGen.hs

-    pure $ SyncProgress
-               (mkEstimatedCompletionTime estCompT)
-               (mkSyncThroughput $ BlockCount blockCount)
-               (mkSyncPercentage pct)


This was removed in favor of proper Arbitrary instances. We were using Arbitrary instances as Example and defining these Gen whatever to generate actual arbitrary data. Non-sense.

KtorZ · 2018-08-18T17:03:35Z

wallet-new/test/WalletNewJson.hs

-roundTripWalletError :: Property
-roundTripWalletError =
-    eachOf 5000 genWalletError roundTripsAesonShow
-


Duplicate with tests in MarshalingSpec

- This put the lights on a few breaking changes - 'NodeIsStillSyncing' error is now properly serialized to a JSend structure (was raw diagnostic before that) - 'WalletIsNotReadyToProcess' error is now properly serialized to a JSend structure - 'WalletAlreadyExists' error now contains the underlying wallet Id - I've extended the golden test suite with all (rather) recently introduced errors - Remove the 'WalletNewGen' in favor of a proper 'Arbitrary' instance defined in API/Types and the round-trip tests defined in MarshalingSpec - Aligned a couple of sample / example references, there's work to do on many types though

Turns out that some of them were actually wrongly migrated (and therefore, fixing them to what they ought to be isn't a breaking change as mentioned before. This concerns the NodeIsStillSyncing and WalletIsNotReadyToProcessPayment branches). This rewrite also makes thing more readable, and now I just want to refactor that to use generics.....

This is done through the combination of 'SOP.Generic', 'Generic' and 'HasDiagnostic' classe. Basically, what we miss from the old implementation are the names of all the partial reccord fields that were used to wrap the diagnostic part of the JSEnd response. These names are now provided by the 'HasDiagnostic' class that need to be implemented for each JSEnd error. From there, we provide two generic functions to convert a type 'a' to a JSON value and respectively, to parse any value to a type a. The main trick for the parsing is to leverage the 'Generic' instance and to construct a JSON value as Aeson would have serialized it in a first place. The serialization becomes quite straighforward once we can serialize the diagnostic which is done via an internal Generic class, working with a few type representations (not all, we don't need them). This approach is much more readable and maintainable than previously and will save us some errors due to typos or wrong formatting.

KtorZ added the wip label Aug 17, 2018

KtorZ requested a review from parsonsmatt August 17, 2018 14:50

KtorZ force-pushed the Squad1/CBR-26/improve-error-handling branch 2 times, most recently from f8351c9 to dc6670e Compare August 17, 2018 15:30

KtorZ commented Aug 17, 2018

View reviewed changes

KtorZ force-pushed the Squad1/CBR-26/improve-error-handling branch from dc6670e to 8965dd4 Compare August 17, 2018 15:38

KtorZ commented Aug 17, 2018

View reviewed changes

KtorZ force-pushed the Squad1/CBR-26/improve-error-handling branch 2 times, most recently from 55fe38f to 63ae68a Compare August 17, 2018 16:07

parsonsmatt approved these changes Aug 17, 2018

View reviewed changes

KtorZ commented Aug 18, 2018

View reviewed changes

ruhatch approved these changes Aug 18, 2018

View reviewed changes

parsonsmatt and others added 8 commits August 18, 2018 18:54

[CBR-254] Implement wallet ID in error

472d678

[CBR-254] Stylish Haskell

1d4e75e

[CBR-26] Update CHANGELOG.md

4a5a327

[CBR-26] Re-generate pkgs/default.nix

1555993

[CBR-26] Implement review suggestions

6387a41

[CBR-26] Fix tests

85d63ab

KtorZ force-pushed the Squad1/CBR-26/improve-error-handling branch from 7c94a28 to 72a608e Compare August 18, 2018 16:55

KtorZ commented Aug 18, 2018

View reviewed changes

KtorZ force-pushed the Squad1/CBR-26/improve-error-handling branch from 72a608e to 9b6e6a5 Compare August 18, 2018 17:09

KtorZ added 3 commits August 18, 2018 19:22

KtorZ force-pushed the Squad1/CBR-26/improve-error-handling branch from 9b6e6a5 to 4bb60d3 Compare August 18, 2018 17:23

[CBR-26] Make API breaking-change explicit in CHANGELOG

e7f76f5

KtorZ force-pushed the Squad1/CBR-26/improve-error-handling branch from 4bb60d3 to e7f76f5 Compare August 18, 2018 22:36

KtorZ removed the wip label Aug 18, 2018

KtorZ merged commit e81cabe into develop Aug 18, 2018

KtorZ deleted the Squad1/CBR-26/improve-error-handling branch September 13, 2018 04:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CBR-26] Error Handling Improvements #3429

[CBR-26] Error Handling Improvements #3429

KtorZ commented Aug 17, 2018 •

edited

Loading

KtorZ Aug 17, 2018

parsonsmatt Aug 17, 2018

KtorZ Aug 17, 2018

KtorZ Aug 17, 2018

KtorZ Aug 17, 2018

KtorZ Aug 17, 2018

KtorZ Aug 17, 2018 •

edited

Loading

KtorZ Aug 17, 2018

KtorZ Aug 17, 2018

KtorZ Aug 17, 2018

KtorZ Aug 17, 2018

parsonsmatt commented Aug 17, 2018

KtorZ Aug 18, 2018

ruhatch left a comment

KtorZ Aug 18, 2018

KtorZ Aug 18, 2018

KtorZ Aug 18, 2018

KtorZ Aug 18, 2018

KtorZ Aug 18, 2018

[CBR-26] Error Handling Improvements #3429

[CBR-26] Error Handling Improvements #3429

Conversation

KtorZ commented Aug 17, 2018 • edited Loading

Description

Linked issue

Type of change

Developer checklist

Testing checklist

QA Steps

Screenshots (if available)

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

KtorZ Aug 17, 2018 • edited Loading

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

parsonsmatt commented Aug 17, 2018

Choose a reason for hiding this comment

ruhatch 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

KtorZ commented Aug 17, 2018 •

edited

Loading

KtorZ Aug 17, 2018 •

edited

Loading