Issue #26: use a more precise type for a list of errors

juspay · Nov 25, 2019 · dbd6538 · dbd6538
1 parent 9cb3563
commit dbd6538
Show file tree

Hide file tree

Showing 4 changed files with 40 additions and 35 deletions.
diff --git a/lib/Fencer/Main.hs b/lib/Fencer/Main.hs
@@ -13,6 +13,7 @@ where
 import BasePrelude
 
 import Control.Concurrent.STM (atomically)
+import qualified Data.List.NonEmpty as NE
 import System.FilePath ((</>))
 import qualified System.Logger as Logger
 import System.Logger (Logger)
@@ -77,27 +78,26 @@ reloadRules logger settings appState = do
         Logger.msg ("Loading rules from " ++ configDir)
 
     -- Read and parse the rules
-    ruleDefinitionsVal :: Either [LoadRulesError] [DomainDefinition] <-
-        loadRulesFromDirectory
-            (#rootDirectory $ settingsRoot settings)
-            (#subDirectory $ settingsSubdirectory settings </> "config")
-            (#ignoreDotFiles (settingsIgnoreDotFiles settings))
-    case ruleDefinitionsVal of
-        Left fs ->
-            Logger.err logger $
-                Logger.msg ("error loading new configuration from runtime: " ++
-                            prettyPrintErrors fs)
-        Right ruleDefinitions -> do
-            Logger.info logger $
-                Logger.msg ("Parsed rules for domains: " ++
-                    show (map (unDomainId . domainDefinitionId) ruleDefinitions))
+    loadRulesFromDirectory
+        (#rootDirectory $ settingsRoot settings)
+        (#subDirectory $ settingsSubdirectory settings </> "config")
+        (#ignoreDotFiles (settingsIgnoreDotFiles settings))
+    >>= \case
+    Left fs ->
+        Logger.err logger $
+            Logger.msg ("error loading new configuration from runtime: " ++
+                        prettyPrintErrors (NE.toList fs))
+    Right ruleDefinitions -> do
+        Logger.info logger $
+            Logger.msg ("Parsed rules for domains: " ++
+                show (map (unDomainId . domainDefinitionId) ruleDefinitions))
 
-            -- Recreate 'appStateRules'
-            --
-            -- There is no need to remove old rate limiting rules
-            atomically $
-                -- See the documentation of 'setRules' for details on what
-                -- happens with counters during rule reloading.
-                setRules appState (map domainToRuleTree ruleDefinitions)
-            Logger.info logger $
-                Logger.msg (Logger.val "Applied new rules")
+        -- Recreate 'appStateRules'
+        --
+        -- There is no need to remove old rate limiting rules
+        atomically $
+            -- See the documentation of 'setRules' for details on what
+            -- happens with counters during rule reloading.
+            setRules appState (map domainToRuleTree ruleDefinitions)
+        Logger.info logger $
+            Logger.msg (Logger.val "Applied new rules")
diff --git a/lib/Fencer/Rules.hs b/lib/Fencer/Rules.hs
@@ -20,8 +20,9 @@ import BasePrelude
 import Control.Applicative (liftA2)
 import Control.Monad.Extra (partitionM, concatMapM, ifM)
 import Data.Either (partitionEithers)
-import Data.Maybe (catMaybes)
 import qualified Data.HashMap.Strict as HM
+import Data.List.NonEmpty (NonEmpty)
+import Data.Maybe (catMaybes)
 import qualified Data.List.NonEmpty as NE
 import Named ((:!), arg)
 import System.Directory (listDirectory, doesFileExist, doesDirectoryExist, getPermissions, pathIsSymbolicLink, readable)
@@ -60,7 +61,7 @@ loadRulesFromDirectory
     :: "rootDirectory" :! FilePath
     -> "subDirectory" :! FilePath
     -> "ignoreDotFiles" :! Bool
-    -> IO (Either [LoadRulesError] [DomainDefinition])
+    -> IO (Either (NonEmpty LoadRulesError) [DomainDefinition])
 loadRulesFromDirectory
     (arg #rootDirectory -> rootDirectory)
     (arg #subDirectory -> subDirectory)
@@ -123,9 +124,9 @@ loadRulesFromDirectory
 -- of @lyft/ratelimit@.
 validatePotentialDomains
   :: [Either LoadRulesError (Maybe DomainDefinition)]
-  -> Either [LoadRulesError] [DomainDefinition]
+  -> Either (NonEmpty LoadRulesError) [DomainDefinition]
 validatePotentialDomains res = case partitionEithers res of
-  (errs@(_:_), _    ) -> Left errs
+  (errs@(_:_), _    ) -> Left $ NE.fromList errs
   ([]        , mRules) -> do
     -- check if there are any duplicate domains
     let

diff --git a/test/Fencer/Rules/Test.hs b/test/Fencer/Rules/Test.hs
@@ -9,6 +9,7 @@ module Fencer.Rules.Test
 
 import           BasePrelude
 
+import qualified Data.List.NonEmpty as NE
 import qualified Data.Yaml as Yaml
 import qualified System.Directory as Dir
 import           System.FilePath ((</>))
@@ -138,7 +139,7 @@ test_rulesLoadRulesException =
         , simpleRuleFile "faultyDomain.yaml" faultyDomain
         ]
       )
-      (#result $ Left
+      (#result $ Left $ NE.fromList
          [LoadRulesParseError "faultyDomain.yaml" $
            Yaml.AesonException
              "Error in $.descriptors[1]: key \"key\" not present"])
@@ -183,7 +184,8 @@ test_rulesLoadRulesDuplicateDomain =
         , simpleRuleFile "two.yaml" domainDescriptorKeyValueText
         ]
       )
-      (#result $ Left [LoadRulesDuplicateDomain $ DomainId "domain1"])
+      (#result $
+         Left $ NE.fromList [LoadRulesDuplicateDomain $ DomainId "domain1"])
 
 -- | test that 'loadRulesFromDirectory' loads a configuration file in
 -- presence of another configuration file without read permissions.

diff --git a/test/Fencer/Rules/Test/Helpers.hs b/test/Fencer/Rules/Test/Helpers.hs
@@ -11,6 +11,8 @@ where
 
 import           BasePrelude
 
+import           Data.List.NonEmpty (NonEmpty)
+import qualified Data.List.NonEmpty as NE
 import qualified Data.Text.IO as TIO
 import           Named ((:!), arg)
 import qualified System.Directory as Dir
@@ -46,7 +48,7 @@ writeAndLoadRules
   :: "ignoreDotFiles" :! Bool
   -> "root" :! FilePath
   -> "files" :! [RuleFile]
-  -> IO (Either [LoadRulesError] [DomainDefinition])
+  -> IO (Either (NonEmpty LoadRulesError) [DomainDefinition])
 writeAndLoadRules
   (arg #ignoreDotFiles -> ignoreDotFiles)
   (arg #root -> root)
@@ -66,7 +68,7 @@ writeAndLoadRules
 expectLoadRules
   :: "ignoreDotFiles" :! Bool
   -> "files" :! [RuleFile]
-  -> "result" :! Either [LoadRulesError] [DomainDefinition]
+  -> "result" :! Either (NonEmpty LoadRulesError) [DomainDefinition]
   -> Assertion
 expectLoadRules
   (arg #ignoreDotFiles -> ignoreDotFiles)
@@ -84,11 +86,11 @@ expectLoadRules
             assertFailure "Expected failures, got domain definitions!"
           Left expectedErrs ->
             assertBool ("Exceptions differ! Expected: " ++
-                        (prettyPrintErrors expectedErrs) ++ "\nGot: " ++
-                        (prettyPrintErrors errs))
+                        (prettyPrintErrors $ NE.toList expectedErrs) ++ "\nGot: " ++
+                        (prettyPrintErrors $ NE.toList errs))
               (((==) `on` (fmap showError))
-               (sortBy (compare `on` showError) (trimPath <$> expectedErrs))
-               (sortBy (compare `on` showError) (trimPath <$> errs)))
+               (NE.sortBy (compare `on` showError) (trimPath <$> expectedErrs))
+               (NE.sortBy (compare `on` showError) (trimPath <$> errs)))
       Right definitions -> assertBool "unexpected definitions"
         (((==) `on` show)
         (sortOn domainDefinitionId <$> result)