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

[olbrich]

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...

active_storage_validations/lib/active_storage_validations/metadata.rb

Lines 78 to 90 in b970f95

	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]

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]

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]

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")

[Mth0158]

Hi @olbrich, @jorge-d and @jaredmoody
This looked like an issue that could be solved. I made a PR about it (#253). It should handle the case when OpenUri returns a Tempfile for Vips. I am not proficient enough with Vips to decide if we could remove completely the check, so I have decided to add another edge case.
Feel free to tell me if metadata.rb changes are ok with you.