Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/5-internal/hlint-federator
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix non-controversial HLint issues in federator to improve code quality
4 changes: 2 additions & 2 deletions services/federator/src/Federator/ExternalServer.hs
Original file line number Diff line number Diff line change
Expand Up @@ -107,14 +107,14 @@ parseRequestData req = do
when (Wai.requestMethod req /= HTTP.methodPost) $
throw InvalidRoute
-- No query parameters are allowed
when (not . BS.null . Wai.rawQueryString $ req) $
unless (BS.null . Wai.rawQueryString $ req) $
throw InvalidRoute
-- check that the path has the expected form
(componentSeg, rpcPath) <- case Wai.pathInfo req of
["federation", comp, rpc] -> pure (comp, rpc)
_ -> throw InvalidRoute

when (not (Text.all isAllowedRPCChar rpcPath)) $
unless (Text.all isAllowedRPCChar rpcPath) $
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this change, but I doubt that it's non-controversial. :) Let's see if anybody complains.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They seem to be exactly the opposite of each other:

when p s = if p then s else pure ()

unless p s =  if p then pure () else s

So, I'm wondering how this could be controversial. 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's clearly still correct, just a question of style: is code easier to read if with fewer keywords, or with less complexity in boolean expressions? i'm also leaning towards unless, and i think we're way too deep into a bike shedding thing again here, so just ignore me. if somebody feels strongly, they can make a PR with an hlint rule that rolls this back, then we can bike shed. :)

throw InvalidRoute

when (Text.null rpcPath) $
Expand Down
2 changes: 1 addition & 1 deletion services/federator/src/Federator/InternalServer.hs
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ parseRequestData req = do
when (Wai.requestMethod req /= HTTP.methodPost) $
throw InvalidRoute
-- No query parameters are allowed
when (not . BS.null . Wai.rawQueryString $ req) $
unless (BS.null . Wai.rawQueryString $ req) $
throw InvalidRoute
-- check that the path has the expected form
(domain, componentSeg, rpcPath) <- case Wai.pathInfo req of
Expand Down
4 changes: 2 additions & 2 deletions services/federator/src/Federator/MockServer.hs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ import Wire.API.Federation.Domain

-- | Thrown in IO by mock federator if the server could not be started after 10
-- seconds.
data MockTimeout = MockTimeout Warp.Port
newtype MockTimeout = MockTimeout Warp.Port
deriving (Eq, Show, Typeable)

instance Exception MockTimeout
Expand Down Expand Up @@ -159,7 +159,7 @@ withTempMockFederator headers resp action = do
frBody = rdBody
}
)
embed @IO $ modifyIORef remoteCalls $ (<> [fedRequest])
embed @IO $ modifyIORef remoteCalls (<> [fedRequest])
body <-
fromException @MockException
. handle (throw . handleException)
Expand Down
2 changes: 1 addition & 1 deletion services/federator/src/Federator/Monitor/Internal.hs
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ data WatchedPath
deriving stock (Eq, Ord, Show, Generic)
deriving (Arbitrary) via (GenericUniform WatchedPath)

mergePaths :: [WatchedPath] -> (Set WatchedPath)
mergePaths :: [WatchedPath] -> Set WatchedPath
mergePaths = Set.fromList . merge . sort
where
merge [] = []
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,13 +62,14 @@ spec env = do
resp <-
runTestSem
. assertNoError @RemoteError
$ inwardBrigCallViaIngress "get-user-by-handle" $
$ inwardBrigCallViaIngress
"get-user-by-handle"
(Aeson.fromEncoding (Aeson.toEncoding hdl))
liftIO $ do
bdy <- streamingResponseStrictBody resp
let actualProfile = Aeson.decode (toLazyByteString bdy)
responseStatusCode resp `shouldBe` HTTP.status200
actualProfile `shouldBe` (Just expectedProfile)
actualProfile `shouldBe` Just expectedProfile

-- @SF.Federation @TSFI.RESTfulAPI @S2 @S3 @S7
--
Expand Down Expand Up @@ -96,7 +97,9 @@ spec env = do
r <-
runTestSem
. runError @RemoteError
$ inwardBrigCallViaIngressWithSettings tlsSettings "get-user-by-handle" $
$ inwardBrigCallViaIngressWithSettings
tlsSettings
"get-user-by-handle"
(Aeson.fromEncoding (Aeson.toEncoding hdl))
liftIO $ case r of
Right _ -> expectationFailure "Expected client certificate error, got response"
Expand Down
15 changes: 7 additions & 8 deletions services/federator/test/unit/Test/Federator/Client.hs
Original file line number Diff line number Diff line change
Expand Up @@ -131,13 +131,12 @@ testClientStreaming = withInfiniteMockServer $ \port -> do
ceFederator = Endpoint "127.0.0.1" (fromIntegral port)
}
let c = clientIn (Proxy @StreamingAPI) (Proxy @(FederatorClient 'Brig))
runCodensity (runFederatorClientToCodensity env c) $ \eout ->
case eout of
Left err -> assertFailure $ "Unexpected error: " <> displayException err
Right out -> do
let expected = mconcat (replicate 500 "Hello")
actual <- takeSourceT (fromIntegral (LBS.length expected)) (fmap Text.encodeUtf8 out)
actual @?= expected
runCodensity (runFederatorClientToCodensity env c) $ \case
Left err -> assertFailure $ "Unexpected error: " <> displayException err
Right out -> do
let expected = mconcat (replicate 500 "Hello")
actual <- takeSourceT (fromIntegral (LBS.length expected)) (fmap Text.encodeUtf8 out)
actual @?= expected

testClientFailure :: IO ()
testClientFailure = do
Expand Down Expand Up @@ -232,7 +231,7 @@ withInfiniteMockServer k = bracket (startMockServer Nothing app) fst (k . snd)
app _ respond = respond $
Wai.responseStream HTTP.ok200 mempty $ \write flush ->
let go n = do
when (n == 0) $ flush
when (n == 0) flush
write (byteString "Hello\n") *> go (if n == 0 then 100 else n - 1)
in go (1000 :: Int)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,8 @@ tests :: TestTree
tests =
testGroup
"Federate"
[ testGroup "with remote" $
[ testGroup
"with remote"
[ federatedRequestSuccess,
federatedRequestFailureAllowList
]
Expand Down
7 changes: 3 additions & 4 deletions services/federator/test/unit/Test/Federator/Util.hs
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,9 @@ testRequest tr = do
pure . flip Wai.setPath (trPath tr) $
Wai.defaultRequest
{ Wai.requestMethod = trMethod tr,
Wai.requestBody = atomicModifyIORef refChunks $ \bss ->
case bss of
[] -> ([], mempty)
x : y -> (y, x),
Wai.requestBody = atomicModifyIORef refChunks $ \case
[] -> ([], mempty)
x : y -> (y, x),
Wai.requestHeaders =
[("X-SSL-Certificate", HTTP.urlEncode True h) | h <- toList (trCertificateHeader tr)]
<> [(originDomainHeaderName, h) | h <- toList (trDomainHeader tr)]
Expand Down
9 changes: 6 additions & 3 deletions services/federator/test/unit/Test/Federator/Validation.hs
Original file line number Diff line number Diff line change
Expand Up @@ -60,12 +60,15 @@ mockDiscoveryFailure = Polysemy.interpret $ \case

tests :: TestTree
tests =
testGroup "Validation" $
[ testGroup "federateWith" $
testGroup
"Validation"
[ testGroup
"federateWith"
[ federateWithAllowListSuccess,
federateWithAllowListFail
],
testGroup "validateDomain" $
testGroup
"validateDomain"
[ validateDomainAllowListFailSemantic,
validateDomainAllowListFail,
validateDomainAllowListSuccess,
Expand Down