[WIP] TSD-85 - Attempt #2 with Either and minimal #50

mdimjasevic · 2018-08-09T13:14:44Z

This implements error handling with an Either in function return types to reflect the possibility of failure.

…nto mdimjasevic/tsd-85-http-exceptions

…the moment

…nto mdimjasevic/tsd-85-http-exceptions

ksaric

Some fixes required.

ksaric · 2018-08-13T06:20:03Z

log-classifier.cabal

                     , transformers-base
                     , unliftio
-                     -- 
+                     --


Generally, it's best to leave unchanged lines so they don't confuse people.

This is not something I've done by hand. Rather, it was done automatically by a tool while I work on the project, I guess by the editor.

ksaric · 2018-08-13T06:21:22Z

src/DataSource/Http.hs

 import           Data.Aeson (parseJSON)
 import           Data.Aeson.Text (encodeToLazyText)
 import           Data.List (nub)
+import           Data.Text (pack)


You don't need pack, you can look at https://github.com/serokell/universum/blob/master/src/Universum/String/Conversion.hs

ksaric · 2018-08-13T06:23:36Z

src/HttpLayer.hs

+    :: Config
+    -> Text
+    -> Either String Request
+apiRequest Config{..} u = mapLeft show $ catches buildRequest handlerList


I don't think we need an Either here. All our URLs are "internal" and will fail if incorrect anyway.
I would otherwise consider this, but it seems to be difficult for you right now.

There are multiple external functions that get called there and they can fail with an HttpException or with a JSONException. We shouldn't be hoping it will always be a happy case.

Where in apiRequest does it say that?

There is a handlerList in the body of the function providing these two types of exceptions.

Indeed. And how can those exceptions reach this handlerList?

Maybe I'm getting something wrong, but there is this function call: catches buildRequest handlerList. In case buildRequest results in an exception given in the handlerList, it gets handled by a corresponding handler.

ksaric · 2018-08-13T06:23:53Z

src/HttpLayer.hs

+apiRequestAbsolute
+    :: Config
+    -> Text
+    -> Either String Request


What do you mean with "Same here"?

Same here, as in "think why you return the Either". Why do you return it?

I updated the implementation of this function. Now it is almost the same as the apiRequest function, hence it is clear why Either in the return type.

ksaric · 2018-08-13T06:24:35Z

src/HttpLayer.hs

 -- | Request PUT
-addJsonBody :: forall a. ToJSON a => a -> Request -> Request
-addJsonBody body req = setRequestBodyJSON body $ setRequestMethod "PUT" req
+addJsonBody :: forall a. ToJSON a => a -> Request -> Either String Request


As a general rule, if you just wrap Either with a single constructor (Right), you probably don't need it.
Can you rewrite this to catch any exceptions and in that case to return it?

I agree with your observation. However, an exception can occur and that's why there is a call to the catch function.

And where do you catch it?

It's caught right there, on the first line of addJsonBody.

And where in the type signature does it say that this can fail?

The type signature contains the return type Either String Request. The String part of the Either sum type represents its left side, which by convention represents a failure.

@mdimjasevic Think about what I'm asking here, don't just answer back - what place is that exception coming from? How do you know that this function needs to address this exception? What kind of signature is on this function, and does that tell you something about its behavior?
Think about exceptions - do they occur on pure functions? In which cases? A lot of things to think about, no?

All of these questions are easy to answer by peeking at the code. Functions from the HTTP library that are called in addJsonBody can throw exceptions, hence addJsonBody has a corresponding signature. But you must be right, these are great questions.

Functions from the HTTP library

Which ones?

The addJsonBody function's signature follows from your requirement that the HTTPNetworkLayer.hnlAddJsonBody field should have a type hnlAddJsonBody :: forall a. (ToJSON a) => a -> Request -> Either String Request: https://iohk.myjetbrains.com/youtrack/issue/TSD-85#comment=93-26226

ksaric · 2018-08-13T06:31:46Z

src/DataSource/Http.hs


    let url = showURL $ TicketCommentsURL tId
    let req = apiRequest cfg url
+    case req of


Chain computation?

ksaric · 2018-08-13T06:31:49Z

src/DataSource/Http.hs

    let req = apiRequest cfg url
-
-    apiCall parseJSON req
+    case req of


Chain computation?

ksaric · 2018-08-13T06:32:08Z

src/DataSource/Http.hs

-    let req = addJsonBody responseTicket (apiRequest cfg url)
-    void $ apiCall (pure . encodeToLazyText) req
+    let req = apiRequest cfg url
+    case req of


Chain computation?

ksaric · 2018-08-13T06:32:11Z

src/DataSource/Http.hs

-        case nextPage of
-            Just nextUrl -> go page0 nextUrl
-            Nothing      -> pure page0
+                case req' of


Chain computation?

ksaric · 2018-08-13T06:32:21Z

src/DataSource/Http.hs

-    apiCall <- asksHTTPNetworkLayer hnlApiCall
-
-    Just <$> apiCall parseTicket req
+    case req of


Chain computation?

Can you please explain what you mean with "Chain computation?"

Means, "use a Monad". You first need to address the other issues.

These computations are chained. If they were not, the code wouldn't compile.

Here we have the resulting monad m and the Either monad. They could be combined in a monad stack, but given that there is a single point where a failure could happen, a monad stack would be overkill. Don't you agree?

ksaric

Some small fixes.

ksaric · 2018-08-14T13:25:18Z

src/HttpLayer.hs

+    :: Config
+    -> Text
+    -> Either String Request
+apiRequest Config{..} u = mapLeft show $ catches buildRequest handlerList


Where in apiRequest does it say that?

ksaric · 2018-08-14T13:26:48Z

src/HttpLayer.hs

-
+-- | The mapLeft function takes a function and applies it to an Either value
+-- iff the value takes the form Left _.
+mapLeft :: (a -> c) -> Either a b -> Either c b


In most cases, you don't need such functions, if you need to collapse either you use the either function, otherwise you can case match. Introducing functions for common operations just brings confusion.

ksaric · 2018-08-14T13:27:22Z

src/HttpLayer.hs

 -- | Request PUT
-addJsonBody :: forall a. ToJSON a => a -> Request -> Request
-addJsonBody body req = setRequestBodyJSON body $ setRequestMethod "PUT" req
+addJsonBody :: forall a. ToJSON a => a -> Request -> Either String Request


And where do you catch it?

ksaric · 2018-08-14T15:10:41Z

src/DataSource/Http.hs

-    let req = apiRequest cfg url
-
-    apiCall parseJSON req
+    res <- runEitherT $ do


You can simplify if you want to use transformers here and remove transformers-either:

res <- runExceptT $ do req <- ExceptT . pure $ apiRequest cfg url ExceptT $ apiCall parseJSON req

But I don't think we will require this.

res <- runExceptT $ (ExceptT . pure $ apiRequest cfg url) >>= ExceptT . apiCall parseJSON

Putting it all into a single line doesn't seem like a good idea because it is harder to read.

Yes, I know. The reason it's there is because it has nice symmetry, the one that's relevant and tells you how you can get rid of transformers-either and where it says that we won't require this is one above.

Sure, I'll make the change you suggested above.

ksaric

Further explanations.

ksaric · 2018-08-16T07:07:58Z

src/HttpLayer.hs

+    :: Config
+    -> Text
+    -> Either String Request
+apiRequest Config{..} u = mapLeft show $ catches buildRequest handlerList


Indeed. And how can those exceptions reach this handlerList?

ksaric · 2018-08-16T07:11:03Z

src/HttpLayer.hs

    case parseEither parser v of
-        Right o -> pure o
+        Right o -> pure $ Right o
        Left e -> error $ "couldn't parse response "


What's the result of parseEither parser v?

ksaric · 2018-08-16T08:37:30Z

src/HttpLayer.hs

 -- | Request PUT
-addJsonBody :: forall a. ToJSON a => a -> Request -> Request
-addJsonBody body req = setRequestBodyJSON body $ setRequestMethod "PUT" req
+addJsonBody :: forall a. ToJSON a => a -> Request -> Either String Request


And where in the type signature does it say that this can fail?

ksaric

Fixes.

ksaric · 2018-08-20T09:20:59Z

src/DataSource/Http.hs

-    apiCall <- asksHTTPNetworkLayer hnlApiCall
-
-    Just <$> apiCall parseTicket req
+    case req of


Means, "use a Monad". You first need to address the other issues.

ksaric · 2018-08-20T09:24:01Z

src/HttpLayer.hs

 -- | Request PUT
-addJsonBody :: forall a. ToJSON a => a -> Request -> Request
-addJsonBody body req = setRequestBodyJSON body $ setRequestMethod "PUT" req
+addJsonBody :: forall a. ToJSON a => a -> Request -> Either String Request


@mdimjasevic Think about what I'm asking here, don't just answer back - what place is that exception coming from? How do you know that this function needs to address this exception? What kind of signature is on this function, and does that tell you something about its behavior?
Think about exceptions - do they occur on pure functions? In which cases? A lot of things to think about, no?

ksaric · 2018-08-20T09:25:01Z

src/HttpLayer.hs

+apiRequestAbsolute
+    :: Config
+    -> Text
+    -> Either String Request


Same here, as in "think why you return the Either". Why do you return it?

ksaric · 2018-08-20T09:26:05Z

src/HttpLayer.hs

    case parseEither parser v of
-        Right o -> pure o
+        Right o -> pure $ Right o
        Left e -> error $ "couldn't parse response "


Seems like that matches the return type of the function (hint, hint) 😏

ksaric · 2018-08-20T09:28:06Z

src/HttpLayer.hs

+    :: Config
+    -> Text
+    -> Either String Request
+apiRequest Config{..} u = showErr $ catches buildRequest handlerList


Same thing as #50 (comment)

ksaric · 2018-08-28T13:23:06Z

Closing in favor of #57

ksaric and others added 30 commits July 23, 2018 16:04

[TSD-71] Generalize HTTP functions to a layer.

8b34ad2

Adds an Aeson-related empty exception

1265e0e

Merge branch 'develop' of github.com:input-output-hk/log-classifier i…

62a5c1f

…nto mdimjasevic/tsd-85-http-exceptions

WIP: adds support for handling JSON parsing exceptions

6591ffd

WIP: Fixes some compilation errors wrt exceptions, introduces some new

8b56390

Resolves conflict after a merge

ec35aec

Resolves a few compile errors by adding the MonadThrow constraint

b846064

Fixes a few Maybe TicketInfo compilation errors

5ffc186

Fixes remaining compilation errors (MonadCatch constraint)

404322c

Minor polishing (data -> newtype)

7845bcd

Resolves a few merge conflicts

5f8c328

Merge branch 'develop' of github.com:input-output-hk/log-classifier i…

6c563ef

…nto mdimjasevic/tsd-85-http-exceptions

Changes the type of hnlAddJsonBody

3b0e885

Merge branch 'develop' of github.com:input-output-hk/log-classifier i…

835d4cc

…nto mdimjasevic/tsd-85-http-exceptions

WIP: Changed the type of the apiRequest function; doesn't compile at …

d6745be

…the moment

WIP: Resolved a number of compile errors, but still some left

d7cc048

WIP: exception handling

d9526cf

Merge branch 'develop' of github.com:input-output-hk/log-classifier i…

35e7821

…nto mdimjasevic/tsd-85-http-exceptions

Merge branch 'develop' of github.com:input-output-hk/log-classifier i…

0ab5f7b

…nto mdimjasevic/tsd-85-http-exceptions

WIP: replaces catch with catches

6ffb37e

WIP: adds parseUrlThrow instead of parseRequest

24cbedd

WIP: Some more progress with removing compilation errors

16c9584

WIP: Yet some more progress with removing compilation errors

8569110

WIP: Yet some more progress with removing compilation errors

35c278b

[TSD-85] - Additional imports require new cabal2nix.nix

afcbd80

WIP: almost compiles

c73bb57

WIP: It finally compiles

4862c0d

WIP: initial changes in HttpLayer.hs per Kristijan's feedback from today

91b057c

Removes dependency on the 'either' package

5e4a5de

Removes the MonadThrow constraint where possible

c721407

mdimjasevic added 2 commits August 10, 2018 15:42

Cleanes up left-over comments

c389a5f

Fixes formatting with stylish-haskell

3689e14

ksaric suggested changes Aug 13, 2018

View reviewed changes

mdimjasevic added 8 commits August 13, 2018 14:01

Replaces 'pack' with 'toText'

e829574

WIP: Introduces EitherT

749be31

WIP: Removes one nested either case checking

d492ded

WIP: Fixes another nesting of casing on Eithers

7eebc77

WIP: Fixes a few more nestings of casing on Eithers

cd9afb5

Removes the last nesting of Eithers

a2ea60d

Updates the auto-generated cabal2nix.nix file

f10d283

Minor cleaning up

dfb38a2

ksaric suggested changes Aug 14, 2018

View reviewed changes

ksaric reviewed Aug 14, 2018

View reviewed changes

ksaric reviewed Aug 16, 2018

View reviewed changes

mdimjasevic added 2 commits August 16, 2018 12:50

Implements Kristijan's feedback from today

edd9506

Implements latest Kristijan's feedback that he left in a PR review

61f0c35

ksaric suggested changes Aug 20, 2018

View reviewed changes

mdimjasevic added 2 commits August 20, 2018 14:34

Introduces a generalisation function apiRequestGeneral

13fb905

Addressed Kristijan's feedback on simplifying code

6538dab

ksaric closed this Aug 28, 2018

ksaric deleted the mdimjasevic/TSD-85-take-2 branch August 28, 2018 13:23

[WIP] TSD-85 - Attempt #2 with Either and minimal #50

[WIP] TSD-85 - Attempt #2 with Either and minimal #50

Uh oh!

Conversation

mdimjasevic commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ksaric left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ksaric Aug 14, 2018 • 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!

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!

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!

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!

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!

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!

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!

ksaric left a comment

Choose a reason for hiding this comment

Uh oh!

ksaric Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

mdimjasevic commented Aug 9, 2018 •

edited

Loading

ksaric Aug 14, 2018 •

edited

Loading

ksaric Aug 14, 2018 •

edited

Loading