Replace magic getter/setter for phlex options with explicit set_ methods

Author: fnordfish

README.md

@@ -30,6 +30,8 @@ gem install roda-phlex
 * `:layout_opts` (`Object`): Options that are passed to the layout
   class when it is instantiated. These options can be used to customize
   the behavior of the layout. Usually, this is a `Hash`.
+  To avoid external changes effecting subsequent requests, you should `.freeze` this
+  and all nested objects.
 * `:layout_handler` (`#call`): A custom handler for creating layout
   instances. This proc receives three arguments: the layout class, the
   layout options, and the object to be rendered. By default, it runs
@@ -142,16 +144,17 @@ end
 
 ```ruby
 # Define a default layout and layout options for the whole application
-plugin :phlex, layout: MyLayout, layout_opts: { title: +"My App" }
+plugin :phlex, layout: MyLayout, layout_opts: { title: "My App".freeze }.freeze
+
 route do |r|
   r.on "posts" do
     # redefine the layout and layout options for this route tree
-    phlex_layout MyPostLayout
-    phlex_layout_opts[:title] << " - Posts"
+    set_phlex_layout MyPostLayout
+    set_phlex_layout_opts phlex_layout_opts.merge(title: "#{phlex_layout_opts[:title]} - Posts")
 
     r.get 'new' do
       # Redefine the layout and layout options for this route
-      phlex_layout_opts[:title] = "Create new post"
+      phlex_layout_opts[:title] << " - Create new post"
       phlex MyView.new
     end
   end

lib/roda/plugins/phlex.rb

@@ -26,9 +26,6 @@ module RodaPlugins
     #      (like "ApplicationView") to avoid polluting the global namespace.
     # - `:delegate_name`: The name of the method that delegates to the Roda app. Defaults to `"app"`.
     module Phlex
-      Undefined = Object.new
-      private_constant :Undefined
-
       Error = Class.new(StandardError)
 
       # Custom TypeError class for Phlex errors.
@@ -101,90 +98,98 @@ def #{delegate}(...)
       end
 
       module InstanceMethods
-        # Retrieves or sets the layout.
-        # When no argument is provided, it returns the current layout.
-        # Use +nil+ or +false+ to disable layout.
-        #
-        # @param layout [Class, nil, false] The layout (a +Phlex::SGML+ class) to be set.
+        # Retrieves the layout class.
         # @return [Class, nil] The current layout (a +Phlex::SGML+ class) or nil if not set.
-        def phlex_layout(layout = Undefined)
-          case layout
-          when Undefined
-            opts.dig(:phlex, :layout)
-          when nil, false
-            opts[:phlex].delete(:layout)
-          else
-            if layout <= ::Phlex::SGML
-              opts[:phlex][:layout] = layout
-            else
-              raise TypeError.new(layout)
-            end
-          end
+        def phlex_layout
+          return @_phlex_layout if defined?(@_phlex_layout)
+          @_phlex_layout = opts.dig(:phlex, :layout)
         end
 
-        # Retrieves or sets the layout options.
-        # When no argument is provided, it returns the current layout options.
-        # Use +nil+ to delete layout options.
+        # Sets the layout class.
         #
-        # @note The {DEFAULT_LAYOUT_HANDLER} expects +layout_opts+ to be a +Hash+.
-        # @param layout_opts [Object, nil] The layout options to be set, usually a +Hash+.
-        # @return [Object, nil] The current layout options or nil if not set.
-        def phlex_layout_opts(layout_opts = Undefined)
-          case layout_opts
-          when Undefined
-            opts.dig(:phlex, :layout_opts)
-          when nil
-            opts[:phlex].delete(:layout_opts)
+        # @param layout [Class, nil] The layout class to be set.
+        # @return [Class, nil] The layout class that was set.
+        def set_phlex_layout(layout)
+          if !layout || layout <= ::Phlex::SGML
+            @_phlex_layout = layout
           else
-            opts[:phlex][:layout_opts] = layout_opts
+            raise TypeError.new(layout)
           end
         end
 
-        # Retrieves or sets the layout handler.
-        # When no argument is provided, it returns the current layout handler.
-        # Use +nil+ or +:default: to reset the layout handler to the {DEFAULT_LAYOUT_HANDLER}.
-        #
-        # @param handler [#call, nil, :default] The layout handler to be set.
+        # Retrieves the layout options hash.
+        # @return [Object, nil] The current layout options or nil if not set.
+        # @note Layout options set via the plugin configuration will get `dep`ed
+        #       for the current request. Be aware that this is a shallow copy and
+        #       changes to nested objects will affect the original object, that is
+        #       all subsequent requests. This is *unsafe* and should be avoided:
+        #       ```ruby
+        #       plugin :phlex, layout_opts: {key: {nested: "value"}}
+        #       # ...
+        #       # UNSAFE: Changes to phlex_layout_opts[:key] will affect the plugin config.
+        #       phlex_layout_opts[:key][:nested] = "other value"
+        #       ```
+        def phlex_layout_opts
+          return @_phlex_layout_opts if defined?(@_phlex_layout_opts)
+          @_phlex_layout_opts = opts.dig(:phlex, :layout_opts).dup
+        end
+
+        # Sets the layout options.
+        # @param layout_opts [Object, nil] The layout options to be set.
+        # @return [Object, nil] The layout options that were set.
+        # @note The {DEFAULT_LAYOUT_HANDLER} expects +layout_opts+ to be a +Hash+.
+        def set_phlex_layout_opts(layout_opts)
+          @_phlex_layout_opts = layout_opts
+        end
+
+        # Retrieves the layout handler.
         # @return [#call] The current layout handler.
-        def phlex_layout_handler(handler = Undefined)
-          case handler
-          when Undefined
-            opts.dig(:phlex, :layout_handler)
+        def phlex_layout_handler
+          return @_phlex_layout_handler if defined?(@_phlex_layout_handler)
+          @_phlex_layout_handler = opts.dig(:phlex, :layout_handler)
+        end
+
+        # Sets the layout handler.
+        # Use +nil+ or +:default: to reset the layout handler to the {DEFAULT_LAYOUT_HANDLER}.
+        def set_phlex_layout_handler(handler)
+          @_phlex_layout_handler = case handler
           when nil, :default
-            opts[:phlex][:layout_handler] = DEFAULT_LAYOUT_HANDLER
+            DEFAULT_LAYOUT_HANDLER
           else
-            opts[:phlex][:layout_handler] = handler
+            handler
           end
         end
 
-        # Retrieves or sets the Phlex context.
-        # When no argument is provided, it returns the current Phlex context.
-        #
+        # Retrieves the Phlex context.
+        # @return [Hash, nil] The current Phlex context.
+        def phlex_context
+          return @_phlex_context if defined?(@_phlex_context)
+          @phlex_context = opts.dig(:phlex, :context).dup
+        end
+
+        # Sets the Phlex context.
         # @param context [Hash] The Phlex context to be set.
-        # @return [Hash] The current Phlex context.
-        def phlex_context(context = Undefined)
-          case context
-          when Undefined
-            opts.dig(:phlex, :context)
-          else
-            opts[:phlex][:context] = context
-          end
+        # @return [Hash] The Phlex context that was set.
+        def set_phlex_context(context)
+          @_phlex_context = context
         end
 
         # Renders a Phlex object.
         # @param obj [Phlex::SGML] The Phlex object to be rendered.
-        # @param context [Hash] The Phlex context to be used for rendering.
+        # @param layout [Class, nil] The layout to be used for rendering. Defaults to the layout set by {#phlex_layout}.
+        #   - The layout class will be initialized with the object and layout options from #{phlex_layout_opts} via {#phlex_layout_handler}.
+        #   - +nil+ or +false+ will disable layout and render the +obj+ directly.
+        # @param context [Hash, nil] The Phlex context to be used for rendering. Defaults to the context set by {#phlex_context}.
         # @param content_type [String, nil] The content type of the response.
         # @param stream [Boolean] Whether to stream the response or not.
-        def phlex(obj, context: phlex_context, content_type: nil, stream: false)
+        def phlex(obj, layout: phlex_layout, context: phlex_context, content_type: nil, stream: false)
           raise TypeError.new(obj) unless obj.is_a?(::Phlex::SGML)
 
           content_type ||= "image/svg+xml" if obj.is_a?(::Phlex::SVG)
           response["Content-Type"] = content_type if content_type
 
-          phlex_opts = opts[:phlex]
-          renderer = if (layout = phlex_opts[:layout])
-            phlex_layout_handler.call(layout, phlex_opts[:layout_opts], obj)
+          renderer = if layout
+            phlex_layout_handler.call(layout, phlex_layout_opts, obj)
           else
             obj
           end

spec/roda/phlex_spec.rb

@@ -151,6 +151,36 @@
         expect(last_response.body).to include("<title>route-title</title>")
       end
     end
+
+    describe "mutating layout options" do
+      before do
+        @test_app_plugins = {phlex: {layout: AlternativeLayout, layout_opts: {title: +"Default"}}}
+      end
+
+      it "carries to the next request when changing the plugin config" do
+        get "/layout/mutate_opts/unsafe", {title: "A"}
+        expect(last_response.body).to include("<title>A - Default</title>")
+
+        get "/layout/mutate_opts/unsafe", {title: "B"}
+        expect(last_response.body).to include("<title>B - A - Default</title>")
+      end
+
+      it "does not carry to the next request when changing the shallow copy" do
+        get "/layout/mutate_opts/safer", {title: "A"}
+        expect(last_response.body).to include("<title>A - Default</title>")
+
+        get "/layout/mutate_opts/safer", {title: "B"}
+        expect(last_response.body).to include("<title>B - Default</title>")
+      end
+
+      it "does not carry to the next request when creating a new config hash" do
+        get "/layout/mutate_opts/safe", {title: "A"}
+        expect(last_response.body).to include("<title>A - Default</title>")
+
+        get "/layout/mutate_opts/safe", {title: "B"}
+        expect(last_response.body).to include("<title>B - Default</title>")
+      end
+    end
   end
 
   context "when using layout_handler" do

spec/support/capybara_helper.rb

@@ -21,5 +21,5 @@ def needs_server?
   Capybara.register_driver(:needs_server) { NeedsServerDriver.new }
   Capybara.app = TestAppHelper.build_test_app(phlex: {}, streaming: {})
   Capybara.default_driver = :needs_server
-  Capybara.server = :puma, {Silent: true}
+  Capybara.server = :puma, {Silent: true, Threads: "1:1"}
 end

spec/support/test_app_helper.rb

@@ -85,29 +85,31 @@ def build_test_app(test_app_plugins = {})
           r.is do
             r.get do
               if (title = r.params["title"])
-                phlex_layout_opts title: title
+                set_phlex_layout_opts title: title
               end
               phlex FooView.new, content_type: "text/html"
             end
           end
 
           r.get "nil" do
-            phlex_layout nil
+            set_phlex_layout nil
             phlex FooView.new
           end
 
           r.get "false" do
-            phlex_layout false
+            set_phlex_layout false
             phlex FooView.new
           end
 
           r.get "phlex_layout_override" do
-            phlex_layout AlternativeLayout
-            phlex_layout_opts title: "phlex_layout_override"
+            set_phlex_layout AlternativeLayout
+            set_phlex_layout_opts title: "phlex_layout_override"
             phlex FooView.new("content")
           end
 
           r.on "merge_opts_route_override" do
+            # Directly mutating the plugin config is not recommended.
+            # Use `set_phlex_layout` and `set_phlex_layout_opts` instead.
             opts[:phlex][:layout] = AlternativeLayout
             opts[:phlex][:layout_opts] = {title: "route-title"}
 
@@ -118,11 +120,37 @@ def build_test_app(test_app_plugins = {})
               phlex FooView.new, content_type: "text/html"
             end
           end
+
+          r.on "mutate_opts" do
+            r.get "unsafe" do
+              if (title = r.params["title"])
+                # DON'T do this. This will mutate the plugin config and carry over to the next request.
+                phlex_layout_opts[:title].prepend title, " - "
+              end
+              phlex FooView.new, content_type: "text/html"
+            end
+
+            r.get "safer" do
+              if (title = r.params["title"])
+                # Mutating the shallow copy of layout options
+                phlex_layout_opts[:title] = phlex_layout_opts[:title].dup.prepend title, " - "
+              end
+              phlex FooView.new, content_type: "text/html"
+            end
+
+            r.get "safe" do
+              if (title = r.params["title"])
+                # Setting a new layout options hash
+                set_phlex_layout_opts phlex_layout_opts.merge(title: "#{title} - #{phlex_layout_opts[:title]}")
+              end
+              phlex FooView.new, content_type: "text/html"
+            end
+          end
         end
 
         r.on "layout_handler" do
-          phlex_layout AlternativeLayout
-          phlex_layout_handler ->(layout, _opts, obj) {
+          set_phlex_layout AlternativeLayout
+          set_phlex_layout_handler ->(layout, _opts, obj) {
             layout.new(obj, title: "phlex_layout_handler override")
           }
 
@@ -131,12 +159,12 @@ def build_test_app(test_app_plugins = {})
           end
 
           r.get "reset_via_nil" do
-            phlex_layout_handler nil
+            set_phlex_layout_handler nil
             phlex FooView.new
           end
 
           r.get "reset_via_default" do
-            phlex_layout_handler :default
+            set_phlex_layout_handler :default
             phlex FooView.new
           end
         end