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

Transform: Silent Failures which don't output file result in crash later on attempted File copy. #87

Open
holsee opened this issue Sep 6, 2021 · 1 comment

Comments

@holsee
Copy link

holsee commented Sep 6, 2021

Environment

  • Elixir version (elixir -v): OTP 24 Elixir 1.12.2
  • Waffle version (mix deps): 1.1.5
  • Waffle dependencies when applicable (mix deps): n/a
  • Operating system: OS X

Expected behaviour

An error should be returned when a file failed to be transformed (or at least when the output file does not exist).
The silent failure due to the 0 return code, means the actual issue is no obvious.
A change along the lines of the following would check that the expected output file exists before continuing.

Waffle.Transform.Convert @ apply/4

    result = System.cmd(program, args_list(args), stderr_to_stdout: true)

    case result do
      {_, 0} ->
        # Check that the transform output a new file to `new_path`
        if File.exists?(new_path) do
          {:ok, %Waffle.File{file | path: new_path, is_tempfile?: true}}
        else
          failed_cmd = Enum.join([program | arg_list(args)], " ")
          {:error, {"Transform failed silently as no file was output.", failed_cmd}}
        end

      {error_message, _exit_code} ->
        {:error, error_message}
    end

Actual behaviour

Throwing error as the output file does not exist due to silent conversion failure by transform function.

This issue was discovered when trying to convert an animated gif with image magick without the correct arguments to select the first frame during the conversion process. This I would suggest is not an uncommon occurrence as gif files are whitelisted in the generate default template and the transform will fail silently for animated versions.

@achempion
Copy link
Member

Generally, I think checking result code is enough as we presume called program has correct behaviour, i.e. it should not return 0 if file generation fails. Although you can just call ls as transform function which will return zero code as well without outputting any file.

Also, you noted in Actual behaviour section what we currently raise the exception because of file missing, so we still catching this type of issues.

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

No branches or pull requests

2 participants