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

"Skipping image analysis because ImageMagick or Vips doesn't support the file" when using open-uri to import images #189

Open
olbrich opened this issue Apr 13, 2023 · 3 comments

Comments

@olbrich
Copy link

olbrich commented Apr 13, 2023

When using OpenURI to import an image from a URL, I get the error
"Skipping image analysis because ImageMagick or Vips doesn't support the file".

I think this only happens when using Vips as an image processor.

When trying to open urls for large images, OpenURI will return an IO object pointing to a tempfile. That tempfile does not have an extension indicating the type of file, which prevents the code from accurately determining the file type.

For some reason, there is a check in this code...

image = if image_processor == :vips && defined?(Vips) && Vips::get_suffixes.include?(File.extname(tempfile.path).downcase)
Vips::Image.new_from_file(tempfile.path)
elsif defined?(MiniMagick)
MiniMagick::Image.new(tempfile.path)
end
else
file_path = read_file_path
image = if image_processor == :vips && defined?(Vips) && Vips::get_suffixes.include?(File.extname(file_path).downcase)
Vips::Image.new_from_file(file_path)
elsif defined?(MiniMagick)
MiniMagick::Image.new(file_path)
end
end

...that makes sure that the file is a type that Vips can handle before letting ImageMagick try to handle it. Since no extension exists on the tempfile, this always fails. In my experiments, Vips is actually able to properly determine the file type from the contents, so that check may not be needed (although this could be dependent on the version of Vips).

Also, it is possible to do this..

remote_image = URI.parse(url).open
profile.image.attach(io: remote_image, filename: "file.jpg", content_type: "image/jpg")

But the validator ignores the filename and content_type hints unless the IO is a StringIO.

Suggested Fixes:

  1. Respect the passed-in filename option.
  2. Skip the check to see if the file is of a type that Vips processes and otherwise handles errors for bad file contents.

When using OpenURI for this kind of thing it is possible for the contents of the IO to be something like an HTML error page instead of an image, so relying on the extension to determine types may not be reliable.

Related to #176 and #154

@olbrich
Copy link
Author

olbrich commented Apr 13, 2023

To further complicate testing, since the gem declares both ruby-vips and minimagick as development dependencies, the elsif fallback will work during tests, but not in a system that doesn't include both vips and minimagick.

Probably this could be done by removing that dependency from the gemspec and making one gemfile for ruby-vips and one for minimagick for all the possible combinations.

FWIW, this is something that https://github.com/thoughtbot/appraisal is good for.

@jorge-d
Copy link

jorge-d commented Jul 14, 2023

Hi there,
Having the same issue on our side, for anyone stumbling upon this issue our quickfix for now is to generate a Tempfile with the correct extension, copy the content of the file into it and use it to attach:

# we need to generate a tempfile with the correct extension for active_storage_validation dimension analysis to work
uri = URI(url)
filename = uri.path.split('/').last

response = uri.open(open_timeout: 10.seconds, read_timeout: 10.seconds)

file_extension = File.extname(filename).downcase.presence
file_extension ||= Rack::Mime::MIME_TYPES.invert[response.content_type]

file = Tempfile.new([File.basename(filename.to_s, '.*'), file_extension])

begin
  File.open(file, 'wb') { |f| f << response.read }

  our_object.name_of_our_attachment.attach(io: file, filename: attachment_name, content_type: response.content_type)
ensure
  file.close
  file.unlink
end

and voilà, would be happy to remove those unnecessary steps if anyone has a better solution that works out of the box?

@jaredmoody
Copy link

A slightly simpler workaround is to make sure you provide a StringIO since the validator respects the passed file name if the io is_a?(StringIO)

string_io = StringIO.new(URI.parse(url).read)
profile.image.attach(io: string_io, filename: "file.jpg")

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

3 participants