Expose file path when upload file exceeds maximum size

[nayibor]

this fix returns the file path for an uploaded file when the file being uploaded is too big this is so various actions can be done on the partially uploaded file instead of an {error, Reason} where Reason={error, io_lib:format("~p is too large",[State#upload.param_name])} being returned a tuple of the form {error, Reason} where Reason = {error_file_path,Error_file_path} where Error_file_path = upld file path . When running the yaws_multipart:read_multipart_form(Arg, Options) an option error_file_expose must be passed which will cause the function to return the tuple {error_file_path,Error_file_path} .

[vinoski]

Thanks for this contribution. Before we review it in detail, please do the following:

• add testing for this feature, see testsuite/multipart_post_parsing_SUITE.erl
• squash all commits in this branch down to a single commit
• make sure the commit message is a good one
• rename the error_file_expose atom to return_error_file_path
• check indentation and make sure it's the same as surrounding code

[nayibor]

noted . will work on the suggestions and commit .

[nayibor]

have worked on the changes outlined except the test cases . working on the test case and will squash and recommit.

[nayibor]

hello , the travis ci check is failing on erlang 18.2. its giving these error messages below.

(ct@travis-job-klacke-yaws-462906135)1> {error_logger,{{2018,12,3},{17,14,59}},"Error in process ~p with exit value:~n~p~n",[<0.2590.0>,{badarg,[{code_server,call,2,[{file,"code_server.erl"},{line,146}]},{test_server_sup,framework_call,4,[{file,"test_server_sup.erl"},{line,754}]},{test_server_ctrl,init_tester,10,[{file,"test_server_ctrl.erl"},{line,1153}]}]}]}
{"Kernel pid terminated",application_controller,"{application_start_failure,yaws,{{shutdown,{failed_to_start_child,yaws_server,{badbind,[{yaws_server,start_group,2,[{file,\"yaws_server.erl\"},{line,261}]},{lists,filtermap,2,[{file,\"lists.erl\"},{line,1316}]},{yaws_server,init2,5,[{file,\"yaws_server.erl\"},{line,245}]},{gen_server,init_it,6,[{file,\"gen_server.erl\"},{line,328}]},{proc_lib,init_p_do_apply,3,[{file,\"proc_lib.erl\"},{line,240}]}]}}},{yaws_app,start,[normal,[]]}}}"}
============================================================================
make[2]: *** [common_test] Error 1
make[2]: Leaving directory `/home/travis/build/klacke/yaws/testsuite'
make[1]: *** [check-am] Error 2
make[1]: Leaving directory `/home/travis/build/klacke/yaws/testsuite'
make: *** [check-recursive] Error 1
The command "make check -j4 V=1" exited with 2.

was wondering i could also get some help with some test cases for this pull ?

[vinoski]

I restarted the TravisCI check on 18.2 and it passed this time.

[nayibor]

thanks steve. will finish up the test case and upload it up.

[nayibor]

hi steve,

was trying to setup the test case for the multipart and i came up with this test case in the multipart_post_parsing_SUITE suite . wanted to know if the direction is good.

return_error_file_path(_)->
    Body = <<"------WebKitFormBoundaryUkx0KS47IKKfcF4z\r\n",
             "Content-Disposition: form-data; name=upfile; filename=test.txt\r\n",
             "\r\n",
             "Hello world12313123132133423421341dda2341234123412341241241223412421\n",
             "\r\n",
             "------WebKitFormBoundaryUkx0KS47IKKfcF4z\r\n",
             "Content-Disposition: form-data; name=note\r\n",
             "\r\n",
             "testing\r\n",
             "------WebKitFormBoundaryUkx0KS47IKKfcF4z--\r\n">>,
    Arg = #arg{headers=#headers{content_type="multipart/form-data; boundary=----WebKitFormBoundaryUkx0KS47IKKfcF4z"},
               req=#http_request{method='POST'},clidata=Body},
    {error,FilePath} = yaws_multipart:read_multipart_form(Arg,[list,return_error_file_path,{max_file_size,0.0001}]),
    FileSize =  filelib:file_size(FilePath),
    ct:pal("~nfile path is ~p,~nfilesize is ~p",[FilePath,FileSize]),
    ?assertEqual(true ,0.1 < FileSize).

[vinoski]

    {error,FilePath} = yaws_multipart:read_multipart_form(Arg,[list,return_error_file_path,{max_file_size,0.0001}]),

Here, is there a good reason to use such a small value for max_file_size? What if we made it 10?

    ct:pal("~nfile path is ~p,~nfilesize is ~p",[FilePath,FileSize]),

I assume this line is temporary, just for debugging, and it won't be part of the final test?

    ?assertEqual(true ,0.1 < FileSize).

If a value like 10 were supplied for max_file_size above, this could instead be ?assertEqual(10, FileSize).

[nayibor]

Here, is there a good reason to use such a small value for max_file_size? What if we made it 10?

no no reason. was just using that for the examples. the value should not matter technically as long as the file size is bigger than the max_file_size.

I assume this line is temporary, just for debugging, and it won't be part of the final test?

yes it is for debugging purposes. will remove it from the final test case .

If a value like 10 were supplied for max_file_size above, this could instead be ?assertEqual(10, FileSize).

this one is a bit of a problem. seems the file is truncated at a size less than the max file size so using the maxfile size does not seem to work. e.g. if the max file size is 10 meg it seems to get truncated at 9.6 meg also sometimes seems if the max file size is smaller than what is read into memory it just goes straight to the error segment without writing anything to the temp file. eg. if the max file size is 10 bytes and file size is 103 bytes it seems nothing is written to the temp file. is checking for the existance of the temp file good enough ? or use below because the size of the temp file is always less than the max file size

?assertEqual(true ,10 >= FileSize).

[nayibor]

hi steve ,

I have modified the test case for the pull and also rebased with master.

[vinoski]

Thanks, looking at it.

[vinoski]

This is now on master, thanks for your contributions. I made a few changes before committing it, including returning error tuples for any errors from multipart upload file operations instead of just for exceeding the max file size, returning the error file path not only for temp filenames generated by Yaws but also for those passed in from the caller, adding some documentation, and tweaking the unit test slightly.

[nayibor]

thanks for the updates!!.