Merge pull request #85 from blocknotes/internal-improvements-4

Internal improvements 4

Author: blocknotes

README.md

@@ -5,8 +5,8 @@
 [![maintainability](https://api.codeclimate.com/v1/badges/92e1e703c308744a0f66/maintainability)](https://codeclimate.com/github/blocknotes/active_storage_db/maintainability)
 
 [![linters](https://github.com/blocknotes/active_storage_db/actions/workflows/linters.yml/badge.svg)](https://github.com/blocknotes/active_storage_db/actions/workflows/linters.yml)
-[![Specs Postgres Rails 8.0](https://github.com/blocknotes/active_storage_db/actions/workflows/specs_postgres_rails80.yml/badge.svg)](https://github.com/blocknotes/active_storage_db/actions/workflows/specs_postgres_rails80.yml)
-[![Specs MySQL Rails 8.0](https://github.com/blocknotes/active_storage_db/actions/workflows/specs_mysql_rails80.yml/badge.svg)](https://github.com/blocknotes/active_storage_db/actions/workflows/specs_mysql_rails80.yml)
+[![Specs Postgres Rails 8.1](https://github.com/blocknotes/active_storage_db/actions/workflows/specs_postgres_rails81.yml/badge.svg)](https://github.com/blocknotes/active_storage_db/actions/workflows/specs_postgres_rails81.yml)
+[![Specs MySQL Rails 8.1](https://github.com/blocknotes/active_storage_db/actions/workflows/specs_mysql_rails81.yml/badge.svg)](https://github.com/blocknotes/active_storage_db/actions/workflows/specs_mysql_rails81.yml)
 
 An Active Storage service upload/download plugin that stores files in a PostgreSQL or MySQL database.
 Experimental support also for MSSQL and SQLite.
@@ -21,8 +21,8 @@ Useful also with platforms like Heroku (due to their ephemeral file system).
 - Add to your Gemfile `gem 'active_storage_db'` (and execute: `bundle`)
 - Install the gem migrations: `bin/rails active_storage_db:install:migrations` (and execute: `bin/rails db:migrate`)
 - Add to your `config/routes.rb`: `mount ActiveStorageDB::Engine => '/active_storage_db'`
-- Change Active Storage service in *config/environments/development.rb* to: `config.active_storage.service = :db`
-- Add to *config/storage.yml*:
+- Change Active Storage service in _config/environments/development.rb_ to: `config.active_storage.service = :db`
+- Add to _config/storage.yml_:
 
 ```yml
 db:

active_storage_db.gemspec

@@ -1,29 +1,29 @@
 # frozen_string_literal: true
 
-$:.push File.expand_path('lib', __dir__)
+$:.push File.expand_path("lib", __dir__)
 
 # Maintain your gem's version:
-require 'active_storage_db/version'
+require "active_storage_db/version"
 
 # Describe your gem and declare its dependencies:
 Gem::Specification.new do |spec|
-  spec.name        = 'active_storage_db'
-  spec.version     = ActiveStorageDB::VERSION
-  spec.authors     = ['Mattia Roccoberton']
-  spec.email       = ['mat@blocknot.es']
-  spec.homepage    = 'https://github.com/blocknotes/active_storage_db'
-  spec.summary     = 'ActiveStorage DB Service'
-  spec.description = 'An ActiveStorage service plugin to store files in database.'
-  spec.license     = 'MIT'
+  spec.name = "active_storage_db"
+  spec.version = ActiveStorageDB::VERSION
+  spec.authors = ["Mattia Roccoberton"]
+  spec.email = ["mat@blocknot.es"]
+  spec.homepage = "https://github.com/blocknotes/active_storage_db"
+  spec.summary = "ActiveStorage DB Service"
+  spec.description = "An ActiveStorage service plugin to store files in database."
+  spec.license = "MIT"
 
-  spec.required_ruby_version = '>= 2.7.0'
+  spec.required_ruby_version = ">= 2.7.0"
 
-  spec.metadata['homepage_uri'] = spec.homepage
-  spec.metadata['source_code_uri'] = spec.homepage
-  spec.metadata['rubygems_mfa_required'] = 'true'
+  spec.metadata["homepage_uri"] = spec.homepage
+  spec.metadata["source_code_uri"] = spec.homepage
+  spec.metadata["rubygems_mfa_required"] = "true"
 
-  spec.files = Dir["{app,config,db,lib}/**/*", 'MIT-LICENSE', 'Rakefile', 'README.md']
+  spec.files = Dir["{app,config,db,lib}/**/*", "MIT-LICENSE", "Rakefile", "README.md"]
 
-  spec.add_dependency 'activestorage', '>= 6.0'
-  spec.add_dependency 'rails', '>= 6.0'
+  spec.add_dependency "activestorage", ">= 6.0"
+  spec.add_dependency "rails", ">= 6.0"
 end

app/models/active_storage_db/file.rb

@@ -4,7 +4,7 @@ module ActiveStorageDB
   class File < ApplicationRecord
     validates :ref,
               presence: true,
-              allow_blank: false,
               uniqueness: { case_sensitive: false }
+    validates :data, presence: true
   end
 end

lib/tasks/active_storage_db_tasks.rake

@@ -9,9 +9,13 @@ module ActiveStorage
     end
 
     def print_blob(blob, digits: 0)
-      size = (blob.byte_size / 1024).to_s.rjust(7)
+      size = if blob.byte_size < 1024
+               "#{blob.byte_size}B".rjust(8)
+             else
+               "#{blob.byte_size / 1024}K".rjust(8)
+             end
       date = blob.created_at.strftime("%Y-%m-%d %H:%M")
-      puts "#{size}K  #{date}  #{blob.id.to_s.rjust(digits)}  #{blob.filename}"
+      puts "#{size}  #{date}  #{blob.id.to_s.rjust(digits)}  #{blob.filename}"
     end
   end
 end

spec/dummy/app/models/post.rb

@@ -5,5 +5,5 @@ class Post < ApplicationRecord
     attachable.variant :thumb, resize_to_limit: [100, 100]
   end
 
-  validates :title, presence: true, allow_blank: false
+  validates :title, presence: true
 end

spec/models/active_storage_db/file_spec.rb

@@ -41,6 +41,15 @@
         )
       end
     end
+
+    context "when data is nil" do
+      it "raises record invalid exception" do
+        expect { create(:active_storage_db_file, data: nil) }.to raise_exception(
+          ActiveRecord::RecordInvalid,
+          /Data can't be blank/
+        )
+      end
+    end
   end
 
   describe "CRUD lifecycle" do

spec/requests/file_controller_spec.rb

@@ -98,7 +98,7 @@ def unprocessable
       put blob.service_url_for_direct_upload, params: data, headers: { "Content-Type" => "text/plain" }
 
       expect(response).to have_http_status(:no_content)
-      expect(data).to eq blob.download
+      expect(blob.download).to eq data
     end
 
     it "uses blob direct upload with mismatched content type" do

spec/service/active_storage/service/db_service_spec.rb

@@ -110,7 +110,7 @@
 
     before { upload }
 
-    it { is_expected.to be_truthy }
+    it { is_expected.to be true }
 
     it "deletes the file" do
       expect { delete }.to change(ActiveStorageDB::File, :count).from(1).to(0)
@@ -119,17 +119,23 @@
     context "when the attachment is not found" do
       subject(:delete) { service.delete("#{key}!") }
 
-      it { is_expected.to be_falsey }
+      it { is_expected.to be false }
     end
   end
 
   describe ".delete_prefixed" do
     subject(:delete_prefixed) { service.delete_prefixed(key[0..10]) }
 
-    before { upload }
+    let(:other_file) { create(:active_storage_db_file, ref: "unrelated_prefix_file") }
+
+    before do
+      other_file
+      upload
+    end
 
-    it "deletes the files" do
-      expect { delete_prefixed }.to change(ActiveStorageDB::File, :count).from(1).to(0)
+    it "deletes only matching files", :aggregate_failures do
+      expect { delete_prefixed }.to change(ActiveStorageDB::File, :count).by(-1)
+      expect(ActiveStorageDB::File.find_by(ref: "unrelated_prefix_file")).to be_present
     end
 
     context "when no files match the prefix" do
@@ -151,8 +157,6 @@
     context "with an existing file" do
       before { upload }
 
-      after { service.delete(key) }
-
       it "downloads the data" do
         expect(download).to eq fixture_data
       end
@@ -167,9 +171,9 @@
         it "yields multiple chunks when data exceeds chunk size", :aggregate_failures do
           chunks = []
           service.download(key) { |chunk| chunks << chunk }
-          # fixture_data is a PNG (~100 bytes), chunk size is 100 bytes,
-          # so we expect at least 1 chunk yielded via the streaming path
-          expect(chunks).not_to be_empty
+          # fixture_data is 126 bytes, chunk size is 100 bytes,
+          # so we expect exactly 2 chunks via the streaming path
+          expect(chunks.size).to eq 2
           expect(chunks.join).to eq fixture_data
         end
       end
@@ -192,8 +196,6 @@
 
     before { upload }
 
-    after { service.delete(key) }
-
     it { is_expected.to eq fixture_data[range] }
 
     context "when the attachment is not found" do
@@ -206,14 +208,12 @@
   describe ".exist?" do
     subject { service.exist?(key) }
 
-    it { is_expected.to be_falsey }
+    it { is_expected.to be false }
 
     context "when a file is uploaded" do
       before { upload }
 
-      after { service.delete(key) }
-
-      it { is_expected.to be_truthy }
+      it { is_expected.to be true }
     end
   end
 
@@ -226,17 +226,13 @@
   describe ".upload" do
     it "uploads the data" do
       expect(upload).to be_a ActiveStorageDB::File
-    ensure
-      service.delete(key)
     end
 
     context "with the checksum" do
       let(:upload) { service.upload(key, StringIO.new(fixture_data), checksum: checksum) }
 
       it "uploads the data" do
         expect(upload).to be_a ActiveStorageDB::File
-      ensure
-        service.delete(key)
       end
     end
 
@@ -256,8 +252,6 @@
     context "with a duplicate key" do
       before { upload }
 
-      after { service.delete(key) }
-
       it "raises a uniqueness error" do
         expect {
           service.upload(key, StringIO.new(fixture_data))
@@ -286,8 +280,6 @@
       end
     end
 
-    after { service.delete(key) }
-
     it { is_expected.to start_with "#{url_options[:protocol]}#{url_options[:host]}" }
 
     it "generates a token that contains the expected fields" do

spec/tasks/active_storage_db_tasks_spec.rb

@@ -6,9 +6,9 @@
   describe "asdb:list" do
     subject(:task) { execute_task("asdb:list") }
 
-    let!(:first_file) { create(:active_storage_blob, filename: "some file 1", created_at: 1.hour.ago) }
-    let!(:second_file) { create(:active_storage_blob, filename: "some file 2", created_at: 5.hours.ago) }
-    let!(:third_file) { create(:active_storage_blob, filename: "some file 3", created_at: 3.hours.ago) }
+    let!(:first_file) { create(:active_storage_blob, filename: "some file 1") }
+    let!(:second_file) { create(:active_storage_blob, filename: "some file 2") }
+    let!(:third_file) { create(:active_storage_blob, filename: "some file 3") }
 
     it "prints the columns header + the list of 3 files", :aggregate_failures do
       pattern = /#{third_file.id}  #{third_file.filename}.+#{second_file.id}  #{second_file.filename}.+#{first_file.id}  #{first_file.filename}/m
@@ -29,10 +29,6 @@
     context "with a missing source" do
       subject(:task) { execute_task("asdb:download", blob_id: "some_file") }
 
-      before do
-        allow(File).to receive(:writable?).and_return(true)
-      end
-
       it "exits showing a not found error" do
         with_captured_stderr do
           expect { task }.to raise_exception(SystemExit, /Source file not found/)
@@ -46,7 +42,7 @@
       let(:blob) { build_stubbed(:active_storage_blob) }
 
       before do
-        allow(File).to receive(:writable?).and_return(false)
+        allow(File).to receive(:writable?).with(".").and_return(false)
         allow(ActiveStorage::Blob).to receive(:find_by).and_return(blob)
       end
 
@@ -63,13 +59,16 @@
       let(:blob) { build_stubbed(:active_storage_blob) }
 
       before do
-        allow(File).to receive_messages(binwrite: 1000, writable?: true)
+        allow(File).to receive_messages(binwrite: 1000)
+        allow(File).to receive(:writable?).with(".").and_return(true)
         allow(ActiveStorage::Blob).to receive(:find_by).and_return(blob)
         allow(blob).to receive(:download).and_return("some data")
       end
 
-      it "prints the number of bytes written" do
+      it "downloads the blob and writes it to the destination", :aggregate_failures do
         expect(task).to eq "1000 bytes written - some_path\n"
+        expect(blob).to have_received(:download)
+        expect(File).to have_received(:binwrite).with("some_path", "some data")
       end
     end
   end
@@ -93,13 +92,13 @@
       end
     end
 
-    context "with there are some results" do
+    context "when there are some results" do
       subject(:task) { execute_task("asdb:search", filename: "just ") }
 
       before do
-        create(:active_storage_blob, filename: "just a file", created_at: 1.hour.ago)
-        create(:active_storage_blob, filename: "just another file", created_at: 5.hours.ago)
-        create(:active_storage_blob, filename: "the last file", created_at: 3.hours.ago)
+        create(:active_storage_blob, filename: "just a file")
+        create(:active_storage_blob, filename: "just another file")
+        create(:active_storage_blob, filename: "the last file")
       end
 
       it "prints the files that matches", :aggregate_failures do