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

Add To/FromFormMultiPart classes #24

Open
fizruk opened this issue Jan 20, 2016 · 7 comments
Open

Add To/FromFormMultiPart classes #24

fizruk opened this issue Jan 20, 2016 · 7 comments

Comments

@fizruk
Copy link
Owner

fizruk commented Jan 20, 2016

Maybe something like this:

data Part
  = Param ByteString
  | File FileInfo

data FileInfo = FileInfo FilePath ContentType

class ToFormMultiPart a where
  toFormMultiPart :: a -> HashMap Text Part

class FromFormMultiPart a where
  parseFormMultiPart :: HashMap Text Part -> Either Text a
@fizruk
Copy link
Owner Author

fizruk commented Feb 19, 2018

Related work

Server-side:

Client-side:

EDIT: added yesod-form.

@fizruk
Copy link
Owner Author

fizruk commented Feb 21, 2018

yesod-form also has support for file upload (see Cookbook file upload saving files to server).
File upload field is created using fileAFormReq. File info is represented with FileInfo:

data FileInfo = FileInfo
    { fileName        :: !Text
    , fileContentType :: !Text
    , fileSourceRaw   :: !(ConduitT () ByteString (ResourceT IO) ())
    , fileMove        :: !(FilePath -> IO ())
    }

They also seem to have 3 options for handling file uploads (on the server):

data FileUpload
  = FileUploadMemory !(NWP.BackEnd L.ByteString)
  | FileUploadDisk !(InternalState -> NWP.BackEnd FilePath)
  | FileUploadSource !(NWP.BackEnd (ConduitT () ByteString (ResourceT IO) ()))

Note the last option which is not available with servant-multipart AFAICT.

@phadej
Copy link
Collaborator

phadej commented Feb 21, 2018 via email

@fizruk
Copy link
Owner Author

fizruk commented Feb 21, 2018

The more I think about it the more it seems like we should just provide common data types and not use type classes 🙃

For instance we could just provide something like this:

data MultiPartFormData file = MultiPartFormData
  { mpfdInputs :: Form
  , mpfdFiles :: [FileData file]
  }

data FileData file = FileData
  { fdInputName :: Text  -- input field name
  , fdFileName :: Text  -- user's file name
  , fdFileHeaders :: [Header] -- additional file headers
  , fdContentType :: Text
  , fdFileContents :: file
  }

-- | Locally saved file.
newtype LocalFilePath = LocalFilePath FilePath

-- | File contents loaded into memory.
newtype FileContents = FileContents ByteString

-- | Some file that can be read directly or moved (saved) somewhere.
-- Particularly useful for server-side handlers to ignore how server handles uploads.
data SomeFile = SomeFile
  { someFileSorceRaw :: ConduitT () ByteString (ResourceT IO) () -- I don't really know how to convert this into a simpler type
  , someFileMove :: FilePath -> IO ()
  }

Now we could provide classes like

class ToMultiPartFormData file a where
  toMultiPartFormData :: a -> MultiPartFormData file

class FromMultiPartFormData file a where
  parseMultiPartFormData :: MultiPartFormData file -> Either Text a

However those are multi parameter and without functional dependencies so are likely to only cause confusion.

Alternatively we could limit file for client/server:

class ToMultiPartFormData a where
  toMultiPartFormData :: a -> MultiPartFormData (IO ByteString)

class FromMultiPartFormData file a where
  parseMultiPartFormData :: MultiPartFormData SomeFile -> Either Text a

However those look pretty weird to me, especially with IO actions in IO ByteString and SomeFile.

So for now I think it would be sufficient to just have common datatypes and functions and not introduce type classes for now.

@alpmestan @phadej do you have any thoughts on this?

@alpmestan
Copy link

The more I think about it the more it seems like we should just provide common data types and not use type classes 🙃

Surely you'll reconsider this once your head is not upside down anymore. =)

More seriously: I do think typeclasses are useful here, as with captures, query params and what not. The handlers are then allowed to only care about the user's "business types" (e.g data User = User { name :: String, profilePic :: FilePath }, where the picture is uploaded with our machinery and the name sent along in the form). I won't fight people to death on this, of course not, but it would be consistent with everything else. Also, do the From/ToMultipart classes have to be parametrized by what's effectively the backend (file) ? Could we not have:

class ToMultiPartFormData a where
  toMultiPartFormData :: forall backend. KnownBackend backend => a -> MultiPartFormData backend

class FromMultiPartFormData a where
  parseMultiPartFormData :: forall backend. KnownBackend backend => MultiPartFormData backend -> Either Text a

where KnownBackend would be something similar to what we have in servant-multipart ? Again, I'm not too attached to these suggestions, feel free to dismiss them. The most important thing here for me is to have a story for migrating servant-multipart over to whatever we end up settling on by the time it is ready. :)

@fizruk
Copy link
Owner Author

fizruk commented Feb 21, 2018

@alpmestan I think the problem with KnownBackend is that you can't easily write

data User = User
  { name :: String
  , profilePic :: FilePath
  }

instance FromMultiPartFormData User where
  parseMultiPartFormData :: KnownBackend backend =>
    MultiPartFormData backend -> Either Text User
  parseMultiPartFormData = ... -- what goes here?

At least I can't figure out how to do that with servant-multipart's class:

class MultipartBackend tag where
    type MultipartResult tag :: *
    type MultipartBackendOptions tag :: *

    backend :: Proxy tag
            -> MultipartBackendOptions tag
            -> InternalState
            -> ignored1
            -> ignored2
            -> IO SBS.ByteString
            -> IO (MultipartResult tag)

    defaultBackendOptions :: Proxy tag -> MultipartBackendOptions tag

If I understand it correctly, MultipartBackend instances specify how to obtain MultipartResult tag values. However you can't really use backend to implement a backend-agnostic FromMultipart instance (because you have to know how to break down MultipartResult tag, but you only know how to make one with backend).

@alpmestan
Copy link

Hmm, correct, nevermind my suggestion then!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants