Implement multipartI modifier

[fluffynukeit]

This branch implements multipart input to go along with the multipart output combinator. I have tested it with curl and it seems to work fine. There also looks to be no impact on my json GET/POST resources, so I don't think it breaks anything. I did not test or assess the impact on rest-gen for docs/clients.

I have the following concerns about my implementation. Not sure how problematic they might be.

• The multipart boundary is carried around in the MultipartFormat data contructor. This makes Format no longer an Enum or a Bounded.
• The BodyPart type is not re-exported from Rest.Dictionary.Combinators, so using mutlipartI requires importing Network.Multipart to get access to the type. I don't know what the usual practice is for this kind of thing, so I left it alone.
• In this implementation, the body parts are all read as bytestrings and it is up to the handler to inspect headers in the BodyParts and parse the bytestrings as appropriate. This requires either very complicated handling code or some kind of convention on the endpoint for significance and order of BodyParts (for instance, in my test code I check the number of body parts and throw UnsupportedFormat if the number is wrong). I suspect there might be a smarter design that puts the header inspection/parsing into the backend code and out of the handler.

Thanks! I am really loving the rest library.

[hesselink]

Thanks for the PR! The general concept looks good, I'll try to look into it in more detail this week. Feel free to ping me after that if you have no response yet :)

[fluffynukeit]

Any update on whether this will get pulled in or if there are any mods I need to make? :)

[hesselink]

Thanks for reminding me! I just reviewed it, and found one problem (in parseContentType). I'll put the actual problem as a comment on the relevant source line, and I'll also add some nitpicky formatting comments to keep coding style consistent. But overall, it looks good, and nicely mirrors the multipart output.

Regarding your second and third point: when I initially added multipart output, I did do only for the multi-GET endpoint (you can batch up multiple GETs and send them to the top level API endpoint). It wasn't really meant for direct use in handlers. I could imagine using multipart input in the same way, for example to implement a multi-anything handler that is a POST at the top level. For actual use in handlers, I'd define a type containing both things and just use a normal xml or json serialization. Sometimes that's not possible, e.g. with file inputs. We're going to redo the input/dictionary stuff at some point, and that's one of the things we want to fix. What's the use-case you have for multipart input?

[hesselink]

The colons here should be aligned.

[hesselink]

OK, I think that's all :)

[fluffynukeit]

Thanks for the feedback. I should have paid closer attention to the style. I'll fix these up and try out the multipart package for parsing headers when I get an opportunity, although I'm not currently using multipartI for my own project anymore so it might be a while. Or do you think it's even worth trying to fix this up if you guys are planning to redo the dictionary system anyway? I tried to squeeze multipartI into the system that was already there.

My use case at the time was that I wanted to upload a file and json metadata about the file at the same time. There are other ways to structure that kind of service, like uploading the file and getting a URL in the response that you can POST the metadata to, but I didn't want to structure my service like that (I don't like the partially complete interim state).

Putting up a file and data about the file at the same time was my use case generally. Specifically, all I wanted to add was the file name, and I have since learned that the Content-Disposition header is the canonical way to do this, so I use that instead of multipartI presently. For that I made a custom addFilenameH Modifier:

addFilenameH = addHeader $ Header ["Content-Disposition"] $ \case
        [] -> Left . ParseError $ "Problem processing Content-Disposition header."
        Nothing:_ -> Left . MissingField $ "Content-Disposition header not found in request."
        Just str:_ ->
          case takeWhile (/='"') <$> (stripPrefix "attachment;filename=\"" $ filter (/=' ') str) of
            Nothing -> Left . UnsupportedFormat $ "Content-Disposition header improperly formatted."
            Just fn -> let vFn = (isValid tFn, hasExtension tFn && length (takeExtension tFn) > 1)
                           tFn = takeFileName fn
                       in case vFn of
                            (False, _    ) -> Left . ParseError $ "File name \"" ++ fn ++ "\" is invalid."
                            (_    , False) -> Left . ParseError $ "File name \"" ++ fn ++ "\" is missing extension."
                            (True , True ) -> Right tFn

As you can see it's pretty fragile. Following your suggestion, the multipart package might offer some utility here, but it looks like the functionality is meant for parsing a particular header out of a list, and in this case the header is already found; it's just the header content that needs parsing into more useful components.

Ok, that was a bit of a tangent but I thought it helped to answer the use case question. Thanks a lot for taking a look.

[hesselink]

The "file and metadata" use case is something where we could actually use this as well. We currently have the two-stage setup, but it's not ideal as you mention. Additionally, I'd want to use this for a top-level multi-POST, as I mentioned. So if you have the time, I definitely think it's worth it to fix it up and merge it. The rewrite of the dictionary stuff is pretty vague for now, so who knows when we'll get to that.

[bergmark]

Is this still a good base for getting this implemented?

[hesselink]

I think so, if someone fixes the comments above this could still work. I'm still working on the dictionary rewrite, but that's nowhere near finished.

[fluffynukeit]

Hey, gents. I'm pretty far removed from this PR now doing other things, but at the very least I can fix up all the cosmetic nits above, and optionally rebase onto a branch of your choosing. The fix to use multipart instead of the broken string parsing will probably need to come from you guys, though. Does that work for you both?

[bergmark]

Thanks for letting us know Daniel!

Any help is much appreciated, rebasing is best made on top of master.

[fluffynukeit]

Ok, I implemented all the formatting and style updates, I think. I rebased on master. My build environment changed a lot so I was actually unable to build and test the changes, but I suspect they are ok and any required fixes to be minor. Let me know if there's anything else I can help with.

[hesselink]

@fluffynukeit Thanks!

@bergmark Shall I look into the parseContentType/multipart stuff, or will you?

[bergmark]

@hesselink if you have time I'd appreciate it! If we're able to merge this soonish we can include it in the pending major release.

[hesselink]

I'll try to have a look tonight.

[bergmark]

@hesselink any chance you'll be able to take a look at this soon? I think everything else is ready for release now.

[hesselink]

Sorry, forgot to reply to this one. Turns out I'm pretty busy :( Feel free to do it ;)

[bergmark]

Ok, I think I'll just release what we have now. I'm eager to get it out the door!