-
Notifications
You must be signed in to change notification settings - Fork 38
[Rendering] Custom renderer support #244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 3 commits
8136472
512bced
e31bd76
278447b
405450b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -230,6 +230,39 @@ 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 | ||||||||
|
|
||||||||
rsamoilov marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||
| 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 | ||||||||
| dynamic_method_name = Rage::Internal.define_dynamic_method(RageController::API, block) | ||||||||
| @renderers[name] = RendererEntry.new(dynamic_method_name) | ||||||||
| end | ||||||||
|
|
||||||||
| class LogContext | ||||||||
| # @private | ||||||||
| def initialize | ||||||||
|
|
@@ -999,7 +1032,51 @@ def __finalize | |||||||
| end | ||||||||
|
|
||||||||
| Rage::Telemetry.__setup(@telemetry.handlers_map) if @telemetry | ||||||||
|
|
||||||||
| __define_custom_renderers if @renderers | ||||||||
| end | ||||||||
|
|
||||||||
| # @private | ||||||||
| class RendererEntry | ||||||||
| attr_reader :dynamic_method_name | ||||||||
|
|
||||||||
| def initialize(dynamic_method_name) | ||||||||
| @dynamic_method_name = dynamic_method_name | ||||||||
| @applied = false | ||||||||
| end | ||||||||
|
|
||||||||
| def applied? = @applied | ||||||||
| def applied! = (@applied = true) | ||||||||
| end | ||||||||
| private_constant :RendererEntry | ||||||||
|
|
||||||||
| def __define_custom_renderers | ||||||||
| (@renderers || {}).each do |name, entry| | ||||||||
rsamoilov marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||
| next if entry.applied? | ||||||||
|
|
||||||||
| method_name = :"render_#{name}" | ||||||||
|
|
||||||||
| if RageController::API.method_defined?(method_name) | ||||||||
| loc = RageController::API.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 | ||||||||
|
|
||||||||
| RageController::API.class_eval <<~RUBY | ||||||||
| def render_#{name}(*args, status: nil, **kwargs) | ||||||||
|
||||||||
| def render_#{name}(*args, status: nil, **kwargs) | |
| def render_#{name}(*args, status: nil, **kwargs) | |
| raise "Render was called multiple times in this action." if @__rendered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem — I’ll add the raise Render was called multiple times in this action.if @__rendered back at the start of render_#{name} (and keep return if @__rendered after the block).
rsamoilov marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,171 @@ | ||
| # 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 |
There was a problem hiding this comment.
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.renderermethod to not callRage::Internal.define_dynamic_method- all it should do is just store the block in aRendererEntry__define_custom_renderers, let's move everything inside theRageController::API.method_defined?condition into a dedicated method inRageController::API, e.g.__register_redererThis way,
Configurationwon't know the internal details ofRageController::API, and the loop inside__define_custom_rendererswill simply iterate over the renderers, call__register_redererand flip theappliedflag.There was a problem hiding this comment.
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#rendereronly stores the block inRendererEntry, and move the method-definition/conflict-check logic intoRageController::API.__register_renderer. Then__define_custom_rendererswill just iterate, call__register_renderer, and mark the entry as applied.