swift: add conversion from Request to headers#288
Conversation
Adds converters to go from `Request` to a set of headers that upstream Envoy understands. This will be utilized by the request logic in the `Envoy` class. Signed-off-by: Michael Rebello <me@michaelrebello.com>
| /// - returns: Outbound headers to send with an HTTP request. | ||
| func makeOutboundHeaders() -> [String: String] { | ||
| var headers = self.headers | ||
| .filter { !$0.key.hasPrefix(kRestrictedHeaderPrefix) } |
There was a problem hiding this comment.
I'm guessing this will silently drop headers with the restricted prefix - did we decide to do this instead of throw?
Another option would be to simply abort the request and pass control to the onError handler.
There was a problem hiding this comment.
Ah, I see there's another PR which throws.
There was a problem hiding this comment.
Though I just read the comment on that one that we don't want to have to try the builder. What's our current thinking?
There was a problem hiding this comment.
We discussed further over Slack, and decided that it wasn't worth making the builder for adding headers throwable since this would greatly reduce the ergonomics of using it. URLSession allows for setting these headers silently, which is what we prefer to do here as well
There was a problem hiding this comment.
Another option would be to simply abort the request and pass control to the onError handler.
We could do this, though personally I'd prefer to keep it simple and strip those headers for the time being
There was a problem hiding this comment.
Yeah, this was a super annoying realization because Swift/ObjC doesn't have a kinder runtime exception concept which this would be a good use case for since it'll be a way for us to complain to the user that they did something unexpected. While I do think that we should throw for this case, I feel it's worse to make the user handle errors for which they have already accounted for.
The alternatives here is to compromise on the builder object:
- Breaking the builder pattern by returning a pair (builder + error)
- Have the build return a pair (request + error)
- Straight crashing the application (the nuclear rocket version in Swift/ObjC of a runtime exception)
There was a problem hiding this comment.
That all makes sense. I think silently dropping them is reasonable for now.
Signed-off-by: Michael Rebello <me@michaelrebello.com>
Mirrors: #288 Adds converters to go from Request to a set of headers that upstream Envoy understands. This will be utilized by the request logic in the Envoy class. For an explanation of how to fill out the fields, please see the relevant section in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/master/PULL_REQUESTS.md) Description: kotlin: add conversion from Request to headers Risk Level: low Testing: unit Docs Changes: n/a Release Notes: n/a [Optional Fixes #Issue] [Optional Deprecated:]
Mirrors: envoyproxy/envoy-mobile#288 Adds converters to go from Request to a set of headers that upstream Envoy understands. This will be utilized by the request logic in the Envoy class. For an explanation of how to fill out the fields, please see the relevant section in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/master/PULL_REQUESTS.md) Description: kotlin: add conversion from Request to headers Risk Level: low Testing: unit Docs Changes: n/a Release Notes: n/a [Optional Fixes #Issue] [Optional Deprecated:] Signed-off-by: JP Simard <jp@jpsim.com>
Mirrors: envoyproxy/envoy-mobile#288 Adds converters to go from Request to a set of headers that upstream Envoy understands. This will be utilized by the request logic in the Envoy class. For an explanation of how to fill out the fields, please see the relevant section in [PULL_REQUESTS.md](https://github.com/envoyproxy/envoy/blob/master/PULL_REQUESTS.md) Description: kotlin: add conversion from Request to headers Risk Level: low Testing: unit Docs Changes: n/a Release Notes: n/a [Optional Fixes #Issue] [Optional Deprecated:] Signed-off-by: JP Simard <jp@jpsim.com>
Adds converters to go from
Requestto a set of headers that upstream Envoy understands.This will be utilized by the request logic in the
Envoyclass.Previous consideration: #275
Signed-off-by: Michael Rebello me@michaelrebello.com