Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ability to add custom headers to requests in integration framework #122

Closed
wants to merge 3 commits into from

Conversation

piotr-iohk
Copy link
Contributor

Issue Number

#94

Overview

  • I have made possible add custom headers to requests in integration framework in order to test destructive scenarios

Comments

Relates to #56.

@KtorZ KtorZ mentioned this pull request Mar 26, 2019
@piotr-iohk piotr-iohk force-pushed the piotr/94/integration_framework branch from 76094c0 to ad5d1d2 Compare March 26, 2019 22:58
@KtorZ KtorZ self-requested a review March 27, 2019 07:16
@@ -129,31 +133,34 @@ request'
)
=> (Method, Text)
-- ^ HTTP method and request path
-> Maybe RequestHeaders
-- ^ Request headers
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest to use a Map HeaderName Header or [RequestHeaders] here instead and perform a union with the base / default headers later: requestHeaders = baseHeaders <> extraHeaders. This will allow overwriting the base headers when necessary and/or provide extra ones still fairly easily, or none using mempty.

What do you think @piotr-iohk ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if I follow. RequestHeaders is just [Header] (and Header is (HeaderName, ByteString)) so why use [ResuestHeaders]?

Also how would it be possible to overwrite the baseHeaders? (I understand that providing mempty or [] (if I'll change to [Header]) would be having just base (as a result of base <> mempty), providing some extraHeaders would add base and extra but how to overwrite base then?)

Copy link
Member

Choose a reason for hiding this comment

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

Using a Map and doing the union between two maps, we'll the values associated to keys present in the second map overriding the one corresponding to keys present in the first map (if present).

So:

>>> let m1 = Map.fromList [("a", 1), ("b", 2)]
>>> let m2 = Map.fromList [("a", 14), ("c", 42)]
>>> m1 <> m2
Map.fromList [("a", 14), ("b", 2), ("c", 42)]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm. Well, in fact it is:

Prelude> import qualified Data.Map as Map
Prelude Map> let m1 = Map.fromList [("a", 1), ("b", 2)]
Prelude Map> let m2 = Map.fromList [("a", 14), ("c", 42)]
Prelude Map> m1 <> m2
fromList [("a",1),("b",2),("c",42)]

, but that's ok as the arguments can be flipped :) :
... still it won't allow something like providing no headers at all...

Copy link
Member

Choose a reason for hiding this comment

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

... still it won't allow something like providing no headers at all...

True. Aaaaand, okay. You've convinced me ^^

@piotr-iohk piotr-iohk force-pushed the piotr/94/integration_framework branch from ad5d1d2 to 8e43304 Compare March 27, 2019 16:58
@KtorZ
Copy link
Member

KtorZ commented Mar 27, 2019

Merged into #128.

@KtorZ KtorZ closed this Mar 27, 2019
@piotr-iohk piotr-iohk deleted the piotr/94/integration_framework branch March 28, 2019 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants