Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/sitemap_generator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
module SitemapGenerator
autoload(:Interpreter, 'sitemap_generator/interpreter')
autoload(:FileAdapter, 'sitemap_generator/adapters/file_adapter')
autoload(:ActiveStorageAdapter, 'sitemap_generator/adapters/active_storage_adapter') if defined?(::ActiveStorage)
autoload(:ActiveStorageAdapter, 'sitemap_generator/adapters/active_storage_adapter')
autoload(:S3Adapter, 'sitemap_generator/adapters/s3_adapter')
autoload(:AwsSdkAdapter, 'sitemap_generator/adapters/aws_sdk_adapter')
autoload(:WaveAdapter, 'sitemap_generator/adapters/wave_adapter')
Expand Down
38 changes: 28 additions & 10 deletions lib/sitemap_generator/adapters/active_storage_adapter.rb
Original file line number Diff line number Diff line change
@@ -1,26 +1,44 @@
# frozen_string_literal: true

raise LoadError, <<~MSG unless defined?(ActiveStorage)
Error: 'ActiveStorage' is not defined.

Please `require 'active_storage'` - or another library that defines this class.
MSG

module SitemapGenerator
# Class for uploading sitemaps to ActiveStorage.
class ActiveStorageAdapter
attr_reader :key, :filename

def initialize key: :sitemap, filename: 'sitemap.xml.gz'
@key, @filename = key, filename
def initialize(prefix: nil)
@prefix = Pathname.new prefix.to_s
end

def write(location, raw_data)
SitemapGenerator::FileAdapter.new.write(location, raw_data)
FileAdapter.new.write(location, raw_data)
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not exactly sure why all these adapters write to file first, but they all do, so this is preserved. (Though it seems awkward? If I'm using active-storage adapter, for instance, I wouldn't expect files to also be written to disk.)


ActiveStorage::Blob.transaction do
ActiveStorage::Blob.where(key: key).destroy_all
ActiveStorage::Blob.destroy_by(key: key(location))
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

destroy_by is a Rails 6+ shortcut for where().destroy_all. Normally I would have left the existing where.destroy_all as it's well understood and explicit.

However, because ActiveStorage::Blob is an external collaborator in this codepath, reducing the interface on which we depend improves robustness. It also eases the burden on tests (in this case, it eliminates the need to support a chained stub that returns yet another stub).


ActiveStorage::Blob.create_and_upload!(
key: key,
io: open(location.path, 'rb'),
filename: filename,
content_type: 'application/gzip',
key: key(location),
io: File.open(location.path),
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to File.open to match all the other adapters.

However, a future refactor could/should make location.path return a Pathname object. Which would enable: location.path.open

filename: location.filename,
content_type: content_type(location),
identify: false
)
end
end

private

def key(location)
(@prefix / location.path_in_public).to_s
end

def content_type(location)
# Using .gz matching to be consistent with FileAdapter
# but this logic is brittle and needs refactored
"application/#{location.path.match?(/.gz$/) ? 'gzip' : 'xml'}"
end
end
end
82 changes: 57 additions & 25 deletions spec/sitemap_generator/adapters/active_storage_adapter_spec.rb
Original file line number Diff line number Diff line change
@@ -1,39 +1,71 @@
# frozen_string_literal: true

require 'spec_helper'
require 'sitemap_generator/adapters/active_storage_adapter'

RSpec.describe 'SitemapGenerator::ActiveStorageAdapter' do
let(:location) { SitemapGenerator::SitemapLocation.new }
let(:adapter) { SitemapGenerator::ActiveStorageAdapter.new }
let(:fake_active_storage_blob) {
Class.new do
def self.transaction
yield
end
subject(:adapter) { SitemapGenerator::ActiveStorageAdapter.new }

def self.where(*args)
FakeScope.new
end
let!(:active_storage) do
class_double('ActiveStorage::Blob', destroy_by: true, create_and_upload!: nil)
.tap { |blob| allow(blob).to receive(:transaction).and_yield }
.as_stubbed_const
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

class_double and as_stubbed_const are purpose built for creating safe double of classes.

The class_double gives additional verification over using Class.new, because class_double also verifies that any stubbed method is actually present on the real ActiveStorage::Blob. (If active-storage is ever loaded in these tests.)

Additionally, the Class.new approach continually redefines the FakeScope constant, so with multiple specs, you get constant-redefined warnings.

end

describe 'write' do
let(:location) do
SitemapGenerator::SitemapLocation.new(
filename: 'custom.xml',
sitemaps_path: 'some_path'
)
end

it 'creates an ActiveStorage::Blob record' do
adapter.write(location, 'data')

expect(active_storage).to have_received(:create_and_upload!)
end

it 'gets key and filename from the sitemap_location' do
adapter.write(location, 'data')

def self.create_and_upload!(**kwargs)
'ActiveStorage::Blob'
expect(active_storage).to have_received(:create_and_upload!)
.with(include(key: 'some_path/custom.xml', filename: 'custom.xml'))
end

# Ideally, this would be driven by the location or namer collaborators,
# but it's all rather murky at the moment. filename extension is what
# drives compression in FileAdapter, so consistency wins
context 'with a gzipped file' do
let(:location) { SitemapGenerator::SitemapLocation.new(filename: 'custom.xml.gz') }

specify do
adapter.write(location, 'data')

expect(active_storage).to have_received(:create_and_upload!)
.with(include(content_type: 'application/gzip'))
end
end

context 'with a non-gzipped file' do
let(:location) { SitemapGenerator::SitemapLocation.new(filename: 'custom.xml') }

class FakeScope
def destroy_all
true
end
specify do
adapter.write(location, 'data')

expect(active_storage).to have_received(:create_and_upload!)
.with(include(content_type: 'application/xml'))
end
end
}

before do
stub_const('ActiveStorage::Blob', fake_active_storage_blob)
end
context 'with a custom prefix for segmenting from other blobs' do
subject(:adapter) { SitemapGenerator::ActiveStorageAdapter.new(prefix: 'sitemaps') }

describe 'write' do
it 'should create an ActiveStorage::Blob record' do
expect(location).to receive(:filename).and_return('sitemap.xml.gz').at_least(2).times
expect(adapter.write(location, 'data')).to eq 'ActiveStorage::Blob'
it 'prefixes only the key' do
adapter.write(location, 'data')

expect(active_storage).to have_received(:create_and_upload!)
.with(include(key: 'sitemaps/some_path/custom.xml', filename: 'custom.xml'))
end
end
end
end