One home per rule: error bodies, mime types, asset paths, ordering
Four small duplications, each of which had already drifted or was one edit away from it. **The JSON error body.** The host's `ApiController` was a line-by-line copy of the engine's four `rescue_from` blocks plus `resolve_mime`. It is a `WarpEngine::ApiErrorRendering` concern now, included by both, so "what a failure looks like on the wire" is decided once. **Mime resolution.** The same three lines lived in the engine's API controller and the host's. `WarpEngine::Storage.mime_for` owns it — next to the adapter that hands out the files. **Finding the asset behind a path.** `DownloadService` and `FileService` each escaped `%` and `_` by hand and ran their own `LIKE` — the second one without the exact-match attempt the first one had. `ReleaseAsset.for_relative_path` is the one lookup: the absolute path first, then a suffix match anchored at `/` rather than the old `%path%`, which could match a different file whose name merely contained this one. Two now-unreachable helpers (`DownloadService.base_path`, `FileService.base_path`) go with it. **Ordering.** `PlatformLink` and `SoftwareImage` ordered inside their `default_scope`, which leaks into every association and aggregate — the proof was already in the tree: `admin/images.rb` had to write `SoftwareImage.unscope(:order).distinct.pluck(:image_id)`, because MySQL will not order a DISTINCT by a column it does not select. Ordering is a scope you ask for now (`ordered`), the two places that need it ask (`for_platform`, the `software_images` association, so the API's image order is unchanged), and the `unscope` is gone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -0,0 +1,28 @@
|
|||||||
|
module WarpEngine
|
||||||
|
module ApiErrorRendering
|
||||||
|
extend ActiveSupport::Concern
|
||||||
|
|
||||||
|
included do
|
||||||
|
rescue_from StandardError do |e|
|
||||||
|
Rails.logger.error("[#{self.class.name}] #{e.class}: #{e.message}")
|
||||||
|
render json: { error: "Internal server error" }, status: :internal_server_error
|
||||||
|
end
|
||||||
|
|
||||||
|
rescue_from ActiveRecord::RecordNotFound do
|
||||||
|
render json: { error: "Not found" }, status: :not_found
|
||||||
|
end
|
||||||
|
|
||||||
|
rescue_from Errno::ENOENT do
|
||||||
|
render json: { error: "Not found" }, status: :not_found
|
||||||
|
end
|
||||||
|
|
||||||
|
rescue_from ArgumentError do |e|
|
||||||
|
render json: { error: e.message }, status: :bad_request
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
private
|
||||||
|
|
||||||
|
def resolve_mime(path) = WarpEngine::Storage.mime_for(path)
|
||||||
|
end
|
||||||
|
end
|
||||||
@@ -7,25 +7,9 @@ module WarpEngine
|
|||||||
|
|
||||||
before_action :set_version_header
|
before_action :set_version_header
|
||||||
|
|
||||||
|
include WarpEngine::ApiErrorRendering
|
||||||
include WarpEngine::SubjectAuthentication
|
include WarpEngine::SubjectAuthentication
|
||||||
|
|
||||||
rescue_from StandardError do |e|
|
|
||||||
Rails.logger.error("[#{self.class.name}] #{e.class}: #{e.message}")
|
|
||||||
render json: { error: "Internal server error" }, status: :internal_server_error
|
|
||||||
end
|
|
||||||
|
|
||||||
rescue_from ActiveRecord::RecordNotFound do |e|
|
|
||||||
render json: { error: "Not found" }, status: :not_found
|
|
||||||
end
|
|
||||||
|
|
||||||
rescue_from Errno::ENOENT do |e|
|
|
||||||
render json: { error: "Not found" }, status: :not_found
|
|
||||||
end
|
|
||||||
|
|
||||||
rescue_from ArgumentError do |e|
|
|
||||||
render json: { error: e.message }, status: :bad_request
|
|
||||||
end
|
|
||||||
|
|
||||||
rescue_from WarpEngine::DownloadService::Denied do
|
rescue_from WarpEngine::DownloadService::Denied do
|
||||||
render json: { error: "Forbidden" }, status: :forbidden
|
render json: { error: "Forbidden" }, status: :forbidden
|
||||||
end
|
end
|
||||||
@@ -33,13 +17,7 @@ module WarpEngine
|
|||||||
private
|
private
|
||||||
|
|
||||||
def set_version_header
|
def set_version_header
|
||||||
|
|
||||||
response.headers["WarpEngine-Version"] = WarpEngine::VERSION
|
response.headers["WarpEngine-Version"] = WarpEngine::VERSION
|
||||||
end
|
end
|
||||||
|
|
||||||
def resolve_mime(path)
|
|
||||||
ext = File.extname(path.to_s).delete_prefix(".")
|
|
||||||
Mime::Type.lookup_by_extension(ext) || "application/octet-stream"
|
|
||||||
end
|
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|||||||
@@ -13,6 +13,11 @@ module WarpEngine
|
|||||||
|
|
||||||
default_scope { where(deleted_at: nil) }
|
default_scope { where(deleted_at: nil) }
|
||||||
|
|
||||||
|
def self.for_relative_path(relative)
|
||||||
|
absolute = File.join(WarpEngine.config.file_container_path, relative.to_s)
|
||||||
|
find_by(path: absolute) || where("path LIKE ?", "%/#{sanitize_sql_like(relative.to_s)}").first
|
||||||
|
end
|
||||||
|
|
||||||
def self.ransackable_attributes(auth_object = nil)
|
def self.ransackable_attributes(auth_object = nil)
|
||||||
%w[created_at deleted_at id kind path release_id updated_at]
|
%w[created_at deleted_at id kind path release_id updated_at]
|
||||||
end
|
end
|
||||||
|
|||||||
@@ -13,7 +13,7 @@ module WarpEngine
|
|||||||
|
|
||||||
before_save :unset_other_defaults, if: -> { is_default? && is_default_changed? }
|
before_save :unset_other_defaults, if: -> { is_default? && is_default_changed? }
|
||||||
|
|
||||||
default_scope { order(:position) }
|
scope :ordered, -> { order(:position) }
|
||||||
|
|
||||||
def self.ransackable_attributes(auth_object = nil)
|
def self.ransackable_attributes(auth_object = nil)
|
||||||
%w[created_at id image_id is_default position software_id updated_at]
|
%w[created_at id image_id is_default position software_id updated_at]
|
||||||
|
|||||||
@@ -3,14 +3,6 @@ module WarpEngine
|
|||||||
|
|
||||||
class Denied < StandardError; end
|
class Denied < StandardError; end
|
||||||
|
|
||||||
def self.container_base
|
|
||||||
WarpEngine.config.file_container_path
|
|
||||||
end
|
|
||||||
|
|
||||||
def self.base_path
|
|
||||||
Pathname.new(container_base).realpath
|
|
||||||
end
|
|
||||||
|
|
||||||
def locate(path:, ip:, user_agent:, referer:, subject: nil, request: nil)
|
def locate(path:, ip:, user_agent:, referer:, subject: nil, request: nil)
|
||||||
relative = path.to_s
|
relative = path.to_s
|
||||||
return nil unless storage.file?(relative)
|
return nil unless storage.file?(relative)
|
||||||
@@ -52,9 +44,7 @@ module WarpEngine
|
|||||||
end
|
end
|
||||||
|
|
||||||
def find_asset(relative)
|
def find_asset(relative)
|
||||||
escaped = relative.gsub("%", "\\%").gsub("_", "\\_")
|
WarpEngine::ReleaseAsset.for_relative_path(relative)
|
||||||
WarpEngine::ReleaseAsset.find_by(path: File.join(self.class.container_base, relative)) ||
|
|
||||||
WarpEngine::ReleaseAsset.where("path LIKE ?", "%#{escaped}%").first
|
|
||||||
end
|
end
|
||||||
|
|
||||||
def log_download(relative, asset:, ip:, user_agent:, referer:, subject:)
|
def log_download(relative, asset:, ip:, user_agent:, referer:, subject:)
|
||||||
|
|||||||
@@ -1,10 +1,6 @@
|
|||||||
module WarpEngine
|
module WarpEngine
|
||||||
class FileService
|
class FileService
|
||||||
|
|
||||||
def self.base_path
|
|
||||||
Pathname.new(WarpEngine.config.file_container_path).realpath
|
|
||||||
end
|
|
||||||
|
|
||||||
def show(input, subject: nil)
|
def show(input, subject: nil)
|
||||||
relative = input.path.to_s
|
relative = input.path.to_s
|
||||||
|
|
||||||
@@ -28,7 +24,7 @@ module WarpEngine
|
|||||||
|
|
||||||
return WarpEngine::Access::Grant::OPEN if WarpEngine::AccessPolicy.open?
|
return WarpEngine::Access::Grant::OPEN if WarpEngine::AccessPolicy.open?
|
||||||
|
|
||||||
asset = WarpEngine::ReleaseAsset.where("path LIKE ?", "%#{relative.gsub('%', '\\%').gsub('_', '\\_')}%").first
|
asset = WarpEngine::ReleaseAsset.for_relative_path(relative)
|
||||||
grant = WarpEngine.access_policy.authorize_download(asset: asset, subject: subject, request: nil)
|
grant = WarpEngine.access_policy.authorize_download(asset: asset, subject: subject, request: nil)
|
||||||
raise WarpEngine::DownloadService::Denied if grant.nil?
|
raise WarpEngine::DownloadService::Denied if grant.nil?
|
||||||
|
|
||||||
|
|||||||
@@ -43,6 +43,11 @@ module WarpEngine
|
|||||||
end
|
end
|
||||||
|
|
||||||
class << self
|
class << self
|
||||||
|
def mime_for(path)
|
||||||
|
ext = File.extname(path.to_s).delete_prefix(".")
|
||||||
|
Mime::Type.lookup_by_extension(ext) || "application/octet-stream"
|
||||||
|
end
|
||||||
|
|
||||||
def adapter
|
def adapter
|
||||||
configured = WarpEngine.config.storage_adapter
|
configured = WarpEngine.config.storage_adapter
|
||||||
|
|
||||||
|
|||||||
@@ -28,11 +28,21 @@ RSpec.describe WarpEngine::PlatformLink, type: :model do
|
|||||||
expect(WarpEngine::PlatformLink.all).to eq([active])
|
expect(WarpEngine::PlatformLink.all).to eq([active])
|
||||||
end
|
end
|
||||||
|
|
||||||
it "orders by position" do
|
it "does not order: ordering is asked for, not inherited" do
|
||||||
second = create(:platform_link, position: 2)
|
second = create(:platform_link, position: 2)
|
||||||
first = create(:platform_link, position: 1)
|
first = create(:platform_link, position: 1)
|
||||||
|
|
||||||
expect(WarpEngine::PlatformLink.all).to eq([first, second])
|
expect(WarpEngine::PlatformLink.ordered).to eq([ first, second ])
|
||||||
|
expect(WarpEngine::PlatformLink.all.to_sql).not_to include("ORDER BY")
|
||||||
|
end
|
||||||
|
end
|
||||||
|
|
||||||
|
describe ".ordered" do
|
||||||
|
it "is what .for_platform uses, so the API keeps its link order" do
|
||||||
|
second = create(:platform_link, platform: "tic80", position: 2)
|
||||||
|
first = create(:platform_link, platform: "tic80", position: 1)
|
||||||
|
|
||||||
|
expect(WarpEngine::PlatformLink.for_platform("tic80")).to eq([ first, second ])
|
||||||
end
|
end
|
||||||
end
|
end
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user