Skip to content

Expose InnerResponse, Http properties#1291

Merged
dsyme merged 1 commit into
fsprojects:masterfrom
piaste:httpWebResponse
Jan 7, 2020
Merged

Expose InnerResponse, Http properties#1291
dsyme merged 1 commit into
fsprojects:masterfrom
piaste:httpWebResponse

Conversation

@piaste

@piaste piaste commented Oct 22, 2019

Copy link
Copy Markdown
Contributor

The current FSharp.Data.WebResponse wrapper does not easily allow the user to access the original response.

In particular, the WebResponse will normally (always?) be a HttpWebResponse, which exposes critical properties like StatusCode, which are currently buried pretty deep under type tests (and I might not have found them at all without debugging and looking at the source):

function 
| Error (e : Net.WebException) ->
    match e.InnerException with
    | :? Net.WebException as webExn ->
       match webExn.Response with
       | :? Net.HttpWebResponse r when r.StatusCode = Net.HttpStatusCode.Conflict -> 
         
            Log.Informationf("`Resource %s already exists, no operation needed", resourceName)
            Ok ()

This PR adds to FSharp.Data.WebResponse:

  • a set of properties that wrap those in HttpWebResponse, returning None if the wrapped response isn't a HttpWebResponse

  • an InnerResponse property that exposes the original WebResponse, for anything else a user might need

The current `FSharp.Data.WebResponse` wrapper does not allow the user to access the original response.

In particular, the `WebResponse` will normally (always?) be a `HttpWebResponse`, which exposes critical properties like `StatusCode`, which are currently inaccessible save through parsing the added line in `Message`.

This PR adds:

 - a set of properties that wrap those in [HttpWebResponse](https://docs.microsoft.com/it-it/dotnet/api/system.net.httpwebresponse?view=netframework-4.8), returning `None` if the wrapped response isn't a `HttpWebResponse` 

 - an `InnerResponse` property that exposes the original `WebResponse`, for anything a user might need.

@dsyme dsyme left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great. Request adding comments. Adding a test wouldn't go astray either....

Comment thread src/Net/Http.fs
override x.GetResponseStream () = responseStream :> Stream
member x.ResetResponseStream () = responseStream.Position <- 0L

member x.CharacterSet = httpProperty (fun r -> r.CharacterSet)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you put a /// comment on each of these? I know these are missing on the rest but we may as well not make the problem worse :)

@dsyme dsyme merged commit 15e010c into fsprojects:master Jan 7, 2020
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.

2 participants