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

files function capturing empty input-file field. #396

Open
tusharad opened this issue Apr 25, 2024 · 7 comments
Open

files function capturing empty input-file field. #396

tusharad opened this issue Apr 25, 2024 · 7 comments

Comments

@tusharad
Copy link
Contributor

Situation

In my scotty application. I have a HTML form that takes an input file, which is a optional field. I am using files function to capture the file. I assumed that if user doesn't upload a file, the files function would return an empty list. But it is returning a list containing FileInfo type with fileName as "\"\"".

Question:

Is this the correct behaviour? My workaround is if the fileContent is empty, I will reject the file or can there be any better solution for files?

When the list is empty:

Screenshot from 2024-04-25 18-48-48

Checkout full project code:

  1. files function code: https://github.com/tusharad/HaskRead/blob/25cdbe72fe2e8ba7297526de7644d0a503b22e3b/src/ScottyCrud/Handler.hs#L45

  2. HTML Form code: https://github.com/tusharad/HaskRead/blob/25cdbe72fe2e8ba7297526de7644d0a503b22e3b/src/ScottyCrud/HTML/Core.hs#L93

@ocramz ocramz added bug test needed info needed More information is needed labels Apr 25, 2024
@ocramz
Copy link
Collaborator

ocramz commented Apr 25, 2024

Looks like a bug, but could you share first what scotty version is running?

@tusharad
Copy link
Contributor Author

tusharad commented Apr 26, 2024

Hi, it's 0.22

@ocramz
Copy link
Collaborator

ocramz commented Apr 27, 2024

@tusharad could you please check whether previous versions of scotty have the same behaviour? I suspect not because the changes introduced in 0.22 move the files parsing behaviour to wai-extra.

We don't even have a test for the "no files" case, fwiw.

@tusharad
Copy link
Contributor Author

tusharad commented Apr 27, 2024

Hi @ocramz ,
I used this example.
Even after using the older version (0.12.1). On submitting without uploading file, It is given us list containing elements with filename as "\"\"".
image

post upload code:

    post "/upload" $ do
      fileList <- files
      liftIO $ print fileList
      text **"done"**

@ocramz
Copy link
Collaborator

ocramz commented Apr 27, 2024

Thanks @tusharad for checking! This is odd and unintuitive behaviour, but I'm at least glad that it didn't appear in 0.22 .

Question now is, what's the cause of this, and how do we fix it?

@tusharad
Copy link
Contributor Author

Hi @ocramz,

Upon investigating the files and filesOpts functions, I discovered that both rely on the formParamsAndFilesWith function. This function, in turn, utilizes getFormParamsAndFilesAction, which ultimately utilizes sinkRequestBodyEx from wai-extra. Location of sinkRequestBodyEx function.

I've provided two images illustrating the output of wholebody and fs at this location:

  • Screenshot when I upload the file style.css
    Screenshot from 2024-04-28 15-57-40

  • Screenshot when I don't upload a file.
    Screenshot from 2024-04-28 15-58-03

It appears that sinkRequestBodyEx parses the wholebody bytestring, extracting the filename and file content. However, I don't believe the issue lies with sinkRequestBodyEx itself, as it simply performs parsing.

To address this, I propose a simple solution: apply a filter function after sinkRequestBodyEx to exclude files with an empty filename ("\"\""). This solution has worked effectively in my testing.

Here's the proposed modification within the getFormParamsAndFilesAction function:

  bs <- getBodyAction bodyInfo opts
  let
    wholeBody = BL.toChunks bs
  (formparams, fs) <- parseRequestBodyExBS istate prbo wholeBody (W.getRequestBodyType req) `catches` handleWaiParseSafeExceptions
  -- changed here
  let fs' = filter emptyFile fs
  return (convertBoth <$> formparams, convertKey <$> fs')
    where emptyFile (_,fInfo) = ("\"\"" /= (W.fileName fInfo)) 

Please review and let me know if this approach aligns with your expectations. If approved, I'll proceed with creating a PR.

Thank you!

@ocramz
Copy link
Collaborator

ocramz commented Apr 29, 2024

@tusharad Yes, this fix seems sensible but it's also a breaking change that should be documented as such.

Happy to review your PR!

tusharad added a commit to tusharad/scotty that referenced this issue Apr 29, 2024
tusharad added a commit to tusharad/scotty that referenced this issue Apr 29, 2024
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

2 participants