Skip to content
Merged
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
56 changes: 56 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 @@ -230,6 +230,37 @@ 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)
@renderers ||= {}
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
@renderers[name] = RendererEntry.new(block)
end

class LogContext
# @private
def initialize
Expand Down Expand Up @@ -999,7 +1030,32 @@ def __finalize
end

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

__define_custom_renderers if @renderers
end

# @private
class RendererEntry
attr_reader :block

def initialize(block)
@block = block
@applied = false
end

def applied? = @applied
def applied! = (@applied = true)
end
private_constant :RendererEntry

def __define_custom_renderers
@renderers.each do |name, entry|
next if entry.applied?
RageController::API.__register_renderer(name, entry.block)
entry.applied!
end
end
private :__define_custom_renderers
end

# @!parse [ruby]
Expand Down
32 changes: 30 additions & 2 deletions lib/rage/controller/api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,30 @@ def __rage_dynamic_#{name}(condition)
RUBY
end

# @private
def __register_renderer(name, block)
method_name = :"render_#{name}"

if method_defined?(method_name)
loc = instance_method(method_name).source_location
loc_str = loc ? "#{loc[0]}:#{loc[1]}" : "unknown location"

raise ArgumentError,
"cannot register renderer :#{name} — `#{method_name}` is already defined at #{loc_str}"
end

dynamic_method_name = Rage::Internal.define_dynamic_method(self, block)

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)
return if @__rendered
render plain: result.to_s, status: (status || 200)
end
RUBY
end

############
#
# PUBLIC API
Expand Down Expand Up @@ -445,11 +469,14 @@ def prepare_action_params(action_name = nil, **opts, &block)
end
end # class << self

DEFAULT_CONTENT_TYPE = "application/json; charset=utf-8"
private_constant :DEFAULT_CONTENT_TYPE

# @private
def initialize(env, params)
@__env = env
@__params = params
@__status, @__headers, @__body = 204, { "content-type" => "application/json; charset=utf-8" }, []
@__status, @__headers, @__body = 204, { "content-type" => DEFAULT_CONTENT_TYPE }, []
@__rendered = false
end

Expand Down Expand Up @@ -508,7 +535,8 @@ def render(json: nil, plain: nil, sse: nil, status: nil)
@__body << if json
json.is_a?(String) ? json : json.to_json
else
@__headers["content-type"] = "text/plain; charset=utf-8"
ct = @__headers["content-type"]
@__headers["content-type"] = "text/plain; charset=utf-8" if ct.nil? || ct == DEFAULT_CONTENT_TYPE
plain.to_s
end

Expand Down
170 changes: 170 additions & 0 deletions spec/configuration/custom_renderer_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
# frozen_string_literal: true

require "securerandom"

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

def unique_renderer_name(base)
:"#{base}_#{SecureRandom.hex(4)}"
end

it "registers a renderer and defines render_<name> on RageController::API after finalize" do
name = unique_renderer_name(:csv)

config.renderer(name) do |object, delimiter: ","|
headers["content-type"] = "text/csv"
object.join(delimiter)
end

config.__finalize

controller = build_controller do
define_method(:index) do
public_send(:"render_#{name}", %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
name = unique_renderer_name(:csv)

config.renderer(name) do |object|
headers["content-type"] = "text/csv"
object.join(",")
end

config.__finalize

controller = build_controller do
define_method(:index) do
public_send(:"render_#{name}", %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
name = unique_renderer_name(:csv)
config.renderer(name) { "x" }

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

it "raises when generated method conflicts with existing API method" do
name = unique_renderer_name(:conflict)
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
name = unique_renderer_name(:ctx)
config.renderer(name) do |_|
headers["content-type"] = "text/plain; charset=utf-8"
"id=#{params[:id]}"
end

config.__finalize

controller = build_controller do
define_method(:index) do
public_send(:"render_#{name}", 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
name = unique_renderer_name(:empty)
config.renderer(name) do |_|
headers["content-type"] = "text/plain; charset=utf-8"
nil
end

config.__finalize

controller = build_controller do
define_method(:index) do
public_send(:"render_#{name}", 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
name = unique_renderer_name(:sse_like)

config.renderer(name) do |_|
render plain: "from-inner-render", status: :accepted
end

config.__finalize

controller = build_controller do
define_method(:index) do
public_send(:"render_#{name}", nil)
end
end

status, _headers, body = run_action(controller, :index)
expect(status).to eq(202)
expect(body).to eq(["from-inner-render"])
end

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

controller = build_controller do
define_method(:index) do
render plain: "first"
public_send(:"render_#{name}", %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