From 2bb8b7e1b9bb664eb5ce97f03f06dfabb579e06c Mon Sep 17 00:00:00 2001 From: Kieran Eglin Date: Sat, 2 May 2020 14:48:55 -0700 Subject: [PATCH 1/6] Updated deps for rails tests --- external/activestorage_ex_rails/Gemfile | 2 +- external/activestorage_ex_rails/Gemfile.lock | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/external/activestorage_ex_rails/Gemfile b/external/activestorage_ex_rails/Gemfile index 6e5da64..ecdab62 100644 --- a/external/activestorage_ex_rails/Gemfile +++ b/external/activestorage_ex_rails/Gemfile @@ -5,7 +5,7 @@ ruby '2.4.2' gem 'rails', '5.2.2.1' # Database -gem 'pg' +gem 'pg', '~> 0.18' # Server gem 'puma', '~> 3.11' diff --git a/external/activestorage_ex_rails/Gemfile.lock b/external/activestorage_ex_rails/Gemfile.lock index b677e0c..de420a5 100644 --- a/external/activestorage_ex_rails/Gemfile.lock +++ b/external/activestorage_ex_rails/Gemfile.lock @@ -102,7 +102,7 @@ GEM nio4r (2.3.1) nokogiri (1.10.3) mini_portile2 (~> 2.4.0) - pg (1.1.4) + pg (0.21.0) public_suffix (3.0.3) puma (3.12.1) rack (2.0.7) @@ -199,7 +199,7 @@ DEPENDENCIES interactive_editor listen (>= 3.0.5, < 3.2) mini_magick (~> 4.8) - pg + pg (~> 0.18) puma (~> 3.11) rails (= 5.2.2.1) sass-rails (~> 5.0) From a8dc4f45908ebb4c90a117e4cb5e6ca03f9990fe Mon Sep 17 00:00:00 2001 From: Kieran Eglin Date: Sat, 2 May 2020 14:59:22 -0700 Subject: [PATCH 2/6] Updated Service.Upload to accept string paths --- lib/service/disk_service.ex | 32 +++++++++++++++++--------- lib/service/s3_service.ex | 36 +++++++++++++++++++----------- test/service/disk_service_test.exs | 14 +++++++++++- test/service/s3_service_test.exs | 12 +++++++++- 4 files changed, 68 insertions(+), 26 deletions(-) diff --git a/lib/service/disk_service.ex b/lib/service/disk_service.ex index b9ec470..9fe87a2 100644 --- a/lib/service/disk_service.ex +++ b/lib/service/disk_service.ex @@ -62,29 +62,27 @@ defmodule ActivestorageEx.DiskService do Saves an `%Image{}` to disk, as determined by a given `%Blob{}` or `%Variant{}` key ## Parameters - - `image`: A `%Mogrify.Image{}` that isn't persisted + - `image`: A `%Mogrify.Image{}` that isn't persisted _or_ a String.t() path to an image - `key`: The blob or variant's key. File location will be based off this. Directories _will_ be created ## Examples Uploading an `%Image{}` to disk from a `%Blob{}` key ``` - image = %Mogrify.Image{} + image = %Mogrify.Image{} | String.t() blob = %Blob{} DiskService.upload(image, blob.key) # %Mogrify.Image{} ``` """ - def upload(image, key) do - with :ok <- make_path_for(key) do - image - |> Mogrify.save() - |> rename_image(key) + def upload(%Mogrify.Image{} = image, key) do + do_upload(image, key) + end - :ok - else - {:error, err} -> {:error, err} - end + def upload(image_path, key) when is_binary(image_path) do + image_path + |> Mogrify.open() + |> do_upload(key) end @doc """ @@ -195,6 +193,18 @@ defmodule ActivestorageEx.DiskService do |> File.exists?() end + def do_upload(%Mogrify.Image{} = image, key) do + with :ok <- make_path_for(key) do + image + |> Mogrify.save() + |> rename_image(key) + + :ok + else + {:error, err} -> {:error, err} + end + end + defp make_path_for(key) do key |> path_for() diff --git a/lib/service/s3_service.ex b/lib/service/s3_service.ex index 82ff590..0ba9bd0 100644 --- a/lib/service/s3_service.ex +++ b/lib/service/s3_service.ex @@ -21,20 +21,14 @@ defmodule ActivestorageEx.S3Service do {:ok, filepath} end - def upload(image, key) do - saved_image = image |> Mogrify.save() - - with {:ok, image_io} <- File.read(saved_image.path), - {:ok, _} <- put_object_for(key, image_io) do - remove_temp_file(saved_image.path) - - :ok - else - {:error, err} -> - remove_temp_file(saved_image.path) + def upload(%Mogrify.Image{} = image, key) do + do_upload(image, key) + end - {:error, err} - end + def upload(image_path, key) when is_binary(image_path) do + image_path + |> Mogrify.open() + |> do_upload(key) end def delete(key) do @@ -74,6 +68,22 @@ defmodule ActivestorageEx.S3Service do end end + defp do_upload(%Mogrify.Image{} = image, key) do + saved_image = Mogrify.save(image) + + with {:ok, image_io} <- File.read(saved_image.path), + {:ok, _} <- put_object_for(key, image_io) do + remove_temp_file(saved_image.path) + + :ok + else + {:error, err} -> + remove_temp_file(saved_image.path) + + {:error, err} + end + end + defp remove_temp_file(filepath) do File.rm(filepath) end diff --git a/test/service/disk_service_test.exs b/test/service/disk_service_test.exs index 03748a8..6745d22 100644 --- a/test/service/disk_service_test.exs +++ b/test/service/disk_service_test.exs @@ -48,7 +48,7 @@ defmodule ActivestorageExTest.DiskServiceTest do end describe "DiskService.upload/2" do - test "An image is sucessfully saved to disk" do + test "A %Mogrify.Image{} is sucessfully saved to disk" do Application.put_env(:activestorage_ex, :root_path, "test/files") image = Mogrify.open("test/files/image.jpg") key = "test_key" @@ -60,6 +60,18 @@ defmodule ActivestorageExTest.DiskServiceTest do File.rm(DiskService.path_for(key)) end + test "An image is successfully saved to disk from string path" do + Application.put_env(:activestorage_ex, :root_path, "test/files") + image_path = "test/files/image.jpg" + key = "test_key" + + DiskService.upload(image_path, key) + + assert File.exists?(DiskService.path_for(key)) + + File.rm(DiskService.path_for(key)) + end + test "Image directory is created if it doesn't exist" do Application.put_env(:activestorage_ex, :root_path, "test/files") image = Mogrify.open("test/files/image.jpg") diff --git a/test/service/s3_service_test.exs b/test/service/s3_service_test.exs index 799f90b..4ce71cd 100644 --- a/test/service/s3_service_test.exs +++ b/test/service/s3_service_test.exs @@ -50,7 +50,7 @@ defmodule ActivestorageExTest.S3ServiceTest do end describe "S3Service.upload/2" do - test "An image is sucessfully saved to s3" do + test "A %Mogrify.Image{} is sucessfully saved to s3" do image = Mogrify.open("test/files/image.jpg") S3Service.upload(image, @test_key) @@ -60,6 +60,16 @@ defmodule ActivestorageExTest.S3ServiceTest do delete_test_image() end + test "An image is sucessfully saved to s3 from string path" do + image_path = "test/files/image.jpg" + + S3Service.upload(image_path, @test_key) + + assert S3Service.exist?(@test_key) + + delete_test_image() + end + test "An image with a complex path is sucessfully saved to s3" do image = Mogrify.open("test/files/image.jpg") key = "variants/new_key" From 0ff67cc140dfdd94ed851323db428ae99fcc874a Mon Sep 17 00:00:00 2001 From: Kieran Eglin Date: Sat, 2 May 2020 15:22:28 -0700 Subject: [PATCH 3/6] Added test to ensure deletion is idempotent --- test/service/s3_service_test.exs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/test/service/s3_service_test.exs b/test/service/s3_service_test.exs index 4ce71cd..4f6e03a 100644 --- a/test/service/s3_service_test.exs +++ b/test/service/s3_service_test.exs @@ -93,6 +93,14 @@ defmodule ActivestorageExTest.S3ServiceTest do refute S3Service.exist?(@test_key) end + test "No error is thrown if a file doesn't exist" do + key = "super_fake_test_key" + + refute S3Service.exist?(key) + + :ok = S3Service.delete(key) + end + test "An image with a complex path is sucessfully deleted from s3" do key = "variants/new_key" upload_test_image(key) From 6020e9cecd8a657dd4e64c1fb79bcc2fc5031978 Mon Sep 17 00:00:00 2001 From: Kieran Eglin Date: Sat, 2 May 2020 15:54:39 -0700 Subject: [PATCH 4/6] Added method to determine image checksum --- lib/blob.ex | 17 ++++++++++++++++- test/blob_test.exs | 22 ++++++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) create mode 100644 test/blob_test.exs diff --git a/lib/blob.ex b/lib/blob.ex index 2a7b5c0..9bf8e7e 100644 --- a/lib/blob.ex +++ b/lib/blob.ex @@ -2,5 +2,20 @@ defmodule ActivestorageEx.Blob do @enforce_keys [:key, :content_type, :filename] defstruct key: nil, content_type: nil, - filename: nil + filename: nil, + checksum: nil + + @doc """ + Returns a string checksum from a given image path + + ## Parameters + - `image_path`: A `String.t()` path to the image on disk + """ + @spec compute_checksum!(String.t()) :: String.t() + def compute_checksum!(image_path) do + File.stream!(image_path, [], 2048) + |> Enum.reduce(:crypto.hash_init(:md5), fn (line, acc) -> :crypto.hash_update(acc, line) end) + |> :crypto.hash_final + |> Base.encode64 + end end diff --git a/test/blob_test.exs b/test/blob_test.exs new file mode 100644 index 0000000..7a9fd8e --- /dev/null +++ b/test/blob_test.exs @@ -0,0 +1,22 @@ +defmodule ActivestorageExTest.BlobTest do + use ExUnit.Case + + alias ActivestorageEx.Blob + + @image_filepath "test/files/image.jpg" + + describe "compute_checksum!/1" do + test "Computes checksum from filepath" do + result = Blob.compute_checksum!(@image_filepath) + + assert String.length(result) > 0 + end + + test "Computes same checksum each time" do + result_1 = Blob.compute_checksum!(@image_filepath) + result_2 = Blob.compute_checksum!(@image_filepath) + + assert result_1 == result_2 + end + end +end From b5f235dcbf6efe3e1c30d7ace37b13bf1daa7c44 Mon Sep 17 00:00:00 2001 From: Kieran Eglin Date: Mon, 4 May 2020 08:20:11 -0700 Subject: [PATCH 5/6] improved tests --- test/blob_test.exs | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/test/blob_test.exs b/test/blob_test.exs index 7a9fd8e..3773de5 100644 --- a/test/blob_test.exs +++ b/test/blob_test.exs @@ -7,16 +7,9 @@ defmodule ActivestorageExTest.BlobTest do describe "compute_checksum!/1" do test "Computes checksum from filepath" do - result = Blob.compute_checksum!(@image_filepath) + known_checksum = "o0K/S+b6DobHNNfe6EGCKA==" - assert String.length(result) > 0 - end - - test "Computes same checksum each time" do - result_1 = Blob.compute_checksum!(@image_filepath) - result_2 = Blob.compute_checksum!(@image_filepath) - - assert result_1 == result_2 + assert ^known_checksum = Blob.compute_checksum!(@image_filepath) end end end From 4e6ea8fd84dd363478cd790abbbd2b6ae4366fab Mon Sep 17 00:00:00 2001 From: Kieran Eglin Date: Mon, 4 May 2020 09:43:56 -0700 Subject: [PATCH 6/6] Added blob analysis function + tests --- lib/activestorage/blob_stats.ex | 18 ++++++++++++++ lib/blob.ex | 42 ++++++++++++++++++++++++++------- test/blob_test.exs | 40 ++++++++++++++++++++++++++++--- 3 files changed, 88 insertions(+), 12 deletions(-) create mode 100644 lib/activestorage/blob_stats.ex diff --git a/lib/activestorage/blob_stats.ex b/lib/activestorage/blob_stats.ex new file mode 100644 index 0000000..d66e8a4 --- /dev/null +++ b/lib/activestorage/blob_stats.ex @@ -0,0 +1,18 @@ +defmodule ActivestorageEx.Activestorage.BlobStats do + @moduledoc ~S""" + Interop with ActiveStorage requires data be a specific shape before persistence. + + This module simply contains a struct that represents how AS expects blob stats (not generated attributes like `key`) + to look before inserting in the database. + + There is some overlap with %ActivestorageEx.Blob{} but that is our internal representation, this is ActiveStorage's. + This is not used interally and is only to ensure shape on the way out + """ + + @enforce_keys [:filename, :content_type, :metadata, :checksum, :byte_size] + defstruct filename: nil, + content_type: nil, + metadata: nil, + checksum: nil, + byte_size: nil +end diff --git a/lib/blob.ex b/lib/blob.ex index 9bf8e7e..248dd9f 100644 --- a/lib/blob.ex +++ b/lib/blob.ex @@ -1,18 +1,42 @@ defmodule ActivestorageEx.Blob do + alias ActivestorageEx.Activestorage.BlobStats + @enforce_keys [:key, :content_type, :filename] defstruct key: nil, content_type: nil, - filename: nil, - checksum: nil + filename: nil + + def analyze!(image_path) do + image = + image_path + |> Mogrify.open() + |> Mogrify.verbose() + + %BlobStats{ + content_type: "image/#{image.format}", + byte_size: size_on_disk(image.path), + filename: filename(image), + checksum: compute_checksum!(image.path), + metadata: %{ + identified: true, + analyzed: true, + height: image.height, + width: image.width + } + } + end - @doc """ - Returns a string checksum from a given image path + defp size_on_disk(image_path) do + %{size: size} = File.stat!(image_path) + + size + end + + defp filename(image) do + Path.basename(image.path, image.ext) + end - ## Parameters - - `image_path`: A `String.t()` path to the image on disk - """ - @spec compute_checksum!(String.t()) :: String.t() - def compute_checksum!(image_path) do + defp compute_checksum!(image_path) do File.stream!(image_path, [], 2048) |> Enum.reduce(:crypto.hash_init(:md5), fn (line, acc) -> :crypto.hash_update(acc, line) end) |> :crypto.hash_final diff --git a/test/blob_test.exs b/test/blob_test.exs index 3773de5..e578f99 100644 --- a/test/blob_test.exs +++ b/test/blob_test.exs @@ -2,14 +2,48 @@ defmodule ActivestorageExTest.BlobTest do use ExUnit.Case alias ActivestorageEx.Blob + alias ActivestorageEx.Activestorage.BlobStats @image_filepath "test/files/image.jpg" - describe "compute_checksum!/1" do - test "Computes checksum from filepath" do + describe "analyze!/1" do + test "Returns %BlobStats{}" do + assert %BlobStats{} = Blob.analyze!(@image_filepath) + end + + test "Returns blob size on disk" do + known_byte_size = 156_746 + + assert %{byte_size: ^known_byte_size} = Blob.analyze!(@image_filepath) + end + + test "Returns blob checksum" do known_checksum = "o0K/S+b6DobHNNfe6EGCKA==" - assert ^known_checksum = Blob.compute_checksum!(@image_filepath) + assert %{checksum: ^known_checksum} = Blob.analyze!(@image_filepath) + end + + test "Returns blob content_type" do + known_content_type = "image/jpeg" + + assert %{content_type: ^known_content_type} = Blob.analyze!(@image_filepath) + end + + test "Returns blob filename" do + known_filename = "image" + + assert %{filename: ^known_filename} = Blob.analyze!(@image_filepath) + end + + test "Returns blob metadata" do + known_metadata = %{ + identified: true, + analyzed: true, + height: 720, + width: 1080 + } + + assert %{metadata: ^known_metadata} = Blob.analyze!(@image_filepath) end end end