Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
75 changes: 75 additions & 0 deletions lib/rage/configuration.rb
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This looks great!

Thinking about responsibilities, there's one more thing I'd love to improve. Currently, the configuration class defines the custom renderer methods on RageController::API. Ideally though, we want the configuration class to simply hold the data and know as little about the outside world as possible.

  • Let's update the renderer method to not call Rage::Internal.define_dynamic_method - all it should do is just store the block in a RendererEntry
  • In __define_custom_renderers, let's move everything inside the RageController::API.method_defined? condition into a dedicated method in RageController::API, e.g. __register_rederer

This way, Configuration won't know the internal details of RageController::API, and the loop inside __define_custom_renderers will simply iterate over the renderers, call __register_rederer and flip the applied flag.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sounds good — I’ll refactor so Configuration#renderer only stores the block in RendererEntry, and move the method-definition/conflict-check logic into RageController::API.__register_renderer. Then __define_custom_renderers will just iterate, call __register_renderer, and mark the entry as applied.

Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,12 @@ class Rage::Configuration
# @private
include Hooks

def initialize
@renderers = {}
end

attr_reader :renderers

# @private
# used in DSL
def config = self
Expand Down Expand Up @@ -230,6 +236,38 @@ def run_after_initialize!
__finalize
end

# Register a custom renderer that generates a <tt>render_<name></tt> method on all controllers.
# The block receives the object passed to <tt>render_<name></tt> and any additional keyword arguments.
# The return value of the block is used as the response body.
#
# @param name [Symbol, String] the name of the renderer
# @param block [Proc] the rendering logic
#
# @example Register a CSV renderer
# Rage.configure do
# config.renderer(:csv) do |object, delimiter: ","|
# headers["content-type"] = "text/csv"
# object.join(delimiter)
# end
# end
#
# @example Use in a controller
# class ReportsController < RageController::API
# def index
# render_csv %w[a b c], delimiter: ";", status: :ok
# end
# end

def renderer(name, &block)
raise ArgumentError, "renderer requires a block" unless block_given?
name = name.to_sym
if @renderers.key?(name)
raise ArgumentError, "a renderer named :#{name} is already registered"
end
dynamic_method_name = RageController::API.define_dynamic_method(block)
@renderers[name] = dynamic_method_name
end

class LogContext
# @private
def initialize
Expand Down Expand Up @@ -999,6 +1037,43 @@ def __finalize
end

Rage::Telemetry.__setup(@telemetry.handlers_map) if @telemetry

@renderers.each do |name, dynamic_method_name|
method_name = :"render_#{name}"

# if this method was already defined by a previous finalize run,
# skip it to avoid false conflicts on app reload
existing = RageController::API.__custom_renderers[method_name]
if existing
next
end

# if the method exists but we didn't define it, someone else owns it — raise
if RageController::API.method_defined?(method_name)
raise ArgumentError,
"cannot register renderer :#{name} — `#{method_name}` is already defined"
end

# generate the render_<name> method on RageController::API so every
# controller inherits it automatically
RageController::API.class_eval <<~RUBY
def render_#{name}(*args,status: nil, **kwargs)
raise "Render was called multiple times in this action" if @__rendered
result = #{dynamic_method_name}(*args, **kwargs)
unless @__rendered
@__rendered = true
@__body << result.to_s
@__status = if status.is_a?(Symbol)
::Rack::Utils::SYMBOL_TO_STATUS_CODE[status] ||
raise(ArgumentError, "unknown status code: \#{status.inspect}")
else
status || 200
end
end
end
RUBY
RageController::API.__custom_renderers[method_name] = true
end
end
end

Expand Down
7 changes: 7 additions & 0 deletions lib/rage/controller/api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@

class RageController::API
class << self

# Tracks render_<name> methods defined by custom renderers so re-finalize
# can skip them instead of raising a false conflict error.
def __custom_renderers
@__custom_renderers ||= {}
end

# @private
# used by the router to register a new action;
# registering means defining a new method which calls the action, makes additional calls (e.g. before actions) and
Expand Down
155 changes: 155 additions & 0 deletions spec/configuration/custom_renderer_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,155 @@
# frozen_string_literal: true

module ConfigurationCustomRendererSpec
class BaseController < RageController::API
end
end

RSpec.describe Rage::Configuration do
describe "#renderer / custom renderers" do
let(:config) { described_class.new }

def build_controller(&block)
klass = Class.new(ConfigurationCustomRendererSpec::BaseController)
klass.class_eval(&block) if block
klass
end

it "registers a renderer and defines render_<name> on RageController::API after finalize" do
config.renderer(:csv) do |object, delimiter: ","|
headers["content-type"] = "text/csv"
object.join(delimiter)
end

config.__finalize

controller = build_controller do
def index
render_csv %w[a b c], delimiter: ";"
end
end

expect(run_action(controller, :index)).to eq(
[200, { "content-type" => "text/csv" }, ["a;b;c"]]
)
end

it "supports status: on generated render_<name> method" do
config.renderer(:csv) do |object|
headers["content-type"] = "text/csv"
object.join(",")
end

config.__finalize

controller = build_controller do
def index
render_csv %w[a b], status: :created
end
end

expect(run_action(controller, :index)).to eq(
[201, { "content-type" => "text/csv" }, ["a,b"]]
)
end

it "raises when renderer is registered without a block" do
expect { config.renderer(:csv) }.to raise_error(ArgumentError)
end

it "raises on duplicate renderer names" do
config.renderer(:csv) { "x" }

expect {
config.renderer(:csv) { "y" }
}.to raise_error(ArgumentError)
end

it "raises when generated method conflicts with existing API method" do
name = :conflict_renderer
method_name = :"render_#{name}"

# create a real method so the conflict is real
RageController::API.define_method(method_name) {}

config.renderer(name) { "x" }

expect {
config.__finalize
}.to raise_error(ArgumentError, /#{Regexp.escape(method_name.to_s)}/)
ensure
RageController::API.send(:remove_method, method_name) if RageController::API.method_defined?(method_name)
end

it "executes renderer in controller context (can access headers/request/params)" do
config.renderer(:ctx) do |_|
headers["content-type"] = "text/plain; charset=utf-8"
"id=#{params[:id]}"
end

config.__finalize

controller = build_controller do
def index
render_ctx nil
end
end

expect(run_action(controller, :index, params: { id: 42 })).to eq(
[200, { "content-type" => "text/plain; charset=utf-8" }, ["id=42"]]
)
end

it "converts nil return value to empty string body" do
config.renderer(:empty) do |_|
headers["content-type"] = "text/plain; charset=utf-8"
nil
end

config.__finalize

controller = build_controller do
def index
render_empty nil
end
end

expect(run_action(controller, :index)).to eq(
[200, { "content-type" => "text/plain; charset=utf-8" }, [""]]
)
end

it "does not double-render when renderer block calls render internally" do
config.renderer(:sse_like) do |_|
render plain: "from-inner-render", status: :accepted
end

config.__finalize

controller = build_controller do
def index
render_sse_like nil
end
end

expect(run_action(controller, :index)).to eq(
[202, { "content-type" => "text/plain; charset=utf-8" }, ["from-inner-render"]]
)
end

it "raises if custom renderer is called after already rendering in action" do
config.renderer(:csv) { |_obj| "x" }
config.__finalize

controller = build_controller do
def index
render plain: "first"
render_csv %w[a b]
end
end

expect { run_action(controller, :index) }
.to raise_error("Render was called multiple times in this action")
end
end
end
Loading