add file upload and download endpoints to the test controller#60
add file upload and download endpoints to the test controller#60baronfel wants to merge 8 commits intofsprojects:masterfrom
Conversation
|
I've started trialing using Http.fs on this branch. I've converted the existing codebase over and been able to run all the tests with the exception of 4 that use the query string to send arrays of items, which Http.fs doesn't support. I've sent a PR lifting this restriction, at which point all the existing tests should work. However, this only works with some changes I made to the typeprovider starter pack. In With that branch added the code compiles and the tests mostly pass! Does any of that make sense to you? I was thinking about opening a PR to the TypeProvider Starter Pack repo seeing if anyone else wanted this. |
|
Latest commits have pointed the type provider files to my fork which has the union code added, so builds should now pass on CI. Tests however will mostly fail for the reasons described above. |
|
@baronfel thank you for all your effort here! You definitely could open PR to I am not sure about The primary reason behind
But we are not going The only runtime dependency that we really need is serializer and for not it is @baronfel In your opinion, how hard will be to implement our own http communication directly based on |
|
It's no big deal to not use HttpFs, that was mostly me trying to see if I understood how the project worked well enough to do such a drastic change. If we don't want to use HttpFs, which is perfectly reasonable as it does bring in hopac too :), then I'll just continue with implementing multipart uploads on FSharp.Data so that more than just us can use it. It shouldn't block us here because I can paket include my fork until it gets merged in. As a result we won't need the type provider union changes, but I'll open a PR for that as well to see what the community thinks. Thanks for the checkup and sanity check, Sergey! |
|
Sometimes I not super responsive on GitHub, but this topic is definitely interesting for me. |
|
Finally finished the extension to FSharp.Data.Http that adds support for Multipart form data, that's in fsprojects/FSharp.Data#1056. Once that is merged we can update paket references and continue this work. |
|
@baronfel why did you close this PR? |
|
Oh, I was doing some branch cleanup and I think GitHub did that
automatically. I'll fix it up.
…On Mon, Jun 12, 2017, 1:49 AM Sergey Tihon ***@***.***> wrote:
@baronfel <https://github.com/baronfel> why did you close this PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#60 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAjCG82IjeCUIwvcqMVN7pi9VqR64Sscks5sDN-KgaJpZM4LDs1n>
.
|
|
@sergey-tihon any tips on live-debugging type providers on mono? |
|
I did not do that on Mono. Only did attach to VS from another instance to VS. In theory, you could use VS Code to attach to some Ionide process (it should run fsautocomplete somewhere). Or attach from one instance of VS for Mac to another... @baronfel maybe you already found the solution how to do this? 😃 |
|
GOT IT! At freaking last. I cleaned up some of the operation code a bit to make it hopefully easier to follow, but I'll dump some comments inline. The tests all run and pass! |
paket.dependencies
Outdated
|
|
||
| github fsharp/FSharp.Data src/Net/UriUtils.fs | ||
| github fsharp/FSharp.Data src/Net/Http.fs | ||
| github baronfel/FSharp.Data:multipart_form_data src/Net/Http.fs |
There was a problem hiding this comment.
I'm still waiting on a merge for FSharp.Data at the moment
| open System | ||
| open System.Reflection | ||
|
|
||
| type FileWrapper(fileName: string, data: IO.Stream) = |
There was a problem hiding this comment.
I tried to make this a tuple of these two pieces of data but that failed for some reason.
| open System.Text.RegularExpressions | ||
| open System.IO | ||
|
|
||
| type BodyType = |
There was a problem hiding this comment.
modeling the body content like this made it easier to switch between formdata and multipart form data when we get parameters lists that have normal form data and files
| | Some ty -> defCompiler.CompileTy methodName "Response" ty true | ||
| | None -> typeof<unit> | ||
| | None -> typeof<unit>, false | ||
| | Some ty when ty = SchemaObject.File -> |
There was a problem hiding this comment.
this is sort of a hack because I didn't yet want to look at making the definitions compiler learn that the same swagger type (in this case File), could have different representations on the input and output side.
| let headers = | ||
| let jsonConsumable = op.Consumes |> Seq.exists (fun mt -> mt="application/json") | ||
| <@@ | ||
| <@ |
There was a problem hiding this comment.
generally got away from the untyped expression API as much as possible in this PR.
| | _ -> Some (FormData, <@@ (seq [name, (%var : string)]) @@>) | ||
| | Some (BodyForm f) -> | ||
| Some (BodyForm <@ Seq.append %f [name, %var()] @>) | ||
| | Some (BodyForm f) when param.Type = SchemaObject.File -> |
There was a problem hiding this comment.
if we get a file param, now we have to switch to multipart. for this we have to convert the previous entries into the contentTypefileNamedata structure that files have
| if isReturnFile | ||
| then box stream | ||
| else | ||
| let reader = new StreamReader(stream) |
There was a problem hiding this comment.
we can't use a use here because the finally branch goes all wonky.
| let value = | ||
| <@@ | ||
| let stream = (%result).ResponseStream | ||
| if isReturnFile |
There was a problem hiding this comment.
I don't like how I'm explicitly checking here.
I also don't like how I'm just returning a stream. I think I'd like to see if we can default some kind of file name, perhaps from the Content-Disposition header of the response if it exists?
|
|
||
| testCase "file Upload" <| fun _ -> | ||
| try | ||
| let result = store.UploadFile(6L, "thing", new FileWrapper("thing.jpg", new MemoryStream(Text.Encoding.UTF8.GetBytes("hello!")))) |
There was a problem hiding this comment.
FileWrapper is a leaky type, I'd like to not have to expose it. Thoughts?
| </dependentAssembly> | ||
| </assemblyBinding></runtime> | ||
| </configuration> No newline at end of file | ||
| </assemblyBinding></runtime></configuration> No newline at end of file |
There was a problem hiding this comment.
Binding redirect changes came from paket for some reason.
|
Ping! I've changed this to point back to official FSharp.Data sources now that the Http Multipart Data work was merged. |
|
@baronfel does it ready for review/merge? |
|
Hah! Apparently not. Let me see if I can fix that test. |
|
That's a good question, and one that gets harder if we support openapi 2 and 3 in this library. 3 drastically changed the file schema for the better, but 2 was very very particular in how items could be sent... It's something I want to get back to, just have to find the time. |
|
As I understand it now, Microsoft.OpenApi.Readers support |
|
That's great news! |
|
closing in favor of new PR |
3 participants
I had to just make some stubs and then register their correct output schema formats after checking to see how Swashbuckle itself does it for their testing. So it seems there's not a unified useful way to represent a 'file' yet.
but this at least generates the expected swagger formats in the doc, so it can be used as input when testing the provider.
See
swashbuckle file download controller
swashbuckle file upload controller
swashbuckle file upload registrations
swashbuckle file download registrations
I had to add more to our 'file download' registration because the key is to have the response be of type 'file' as well, so we can key Swagger Provider off of that.