From afdb1ac0a017e7301ec69ca16e58dc8b1d6aec27 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lo=C3=AFc=20Guitaut?= <loic@discourse.org>
Date: Thu, 19 Sep 2024 12:37:47 +0200
Subject: [PATCH] DEV: Disallow default params in service steps
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

With the current implementation, a service step can be written as:
```ruby
def my_step(a_default_value: 2)
  …
end
```
That’s a pattern we want to avoid as default values (if needed) should
be probably defined in a contract.

This patch makes a service raise an exception if a default value is
encountered.
---
 lib/service/base.rb                           |  3 +
 .../chat/app/services/chat/search_chatable.rb |  2 +-
 spec/lib/service_spec.rb                      | 61 +++++++++++++++++++
 3 files changed, 65 insertions(+), 1 deletion(-)
 create mode 100644 spec/lib/service_spec.rb

diff --git a/lib/service/base.rb b/lib/service/base.rb
index fee2cff02e2..a73b11ba527 100644
--- a/lib/service/base.rb
+++ b/lib/service/base.rb
@@ -112,6 +112,9 @@ module Service
       def call(instance, context)
         object = class_name&.new(context)
         method = object&.method(:call) || instance.method(method_name)
+        if method.parameters.any? { _1[0] != :keyreq }
+          raise "In #{type} '#{name}': default values in step implementations are not allowed. Maybe they could be defined in a contract?"
+        end
         args = context.to_h.slice(*method.parameters.select { _1[0] == :keyreq }.map(&:last))
         context[result_key] = Context.build(object: object)
         instance.instance_exec(**args, &method)
diff --git a/plugins/chat/app/services/chat/search_chatable.rb b/plugins/chat/app/services/chat/search_chatable.rb
index ec9a575cddb..37566821905 100644
--- a/plugins/chat/app/services/chat/search_chatable.rb
+++ b/plugins/chat/app/services/chat/search_chatable.rb
@@ -62,7 +62,7 @@ module Chat
       search_category_channels(context, guardian)
     end
 
-    def fetch_direct_message_channels(guardian:, contract:, users:, **args)
+    def fetch_direct_message_channels(guardian:, contract:, users:)
       return unless contract.include_direct_message_channels
       return unless guardian.can_create_direct_message?
       search_direct_message_channels(context, guardian, contract, users)
diff --git a/spec/lib/service_spec.rb b/spec/lib/service_spec.rb
new file mode 100644
index 00000000000..94746999223
--- /dev/null
+++ b/spec/lib/service_spec.rb
@@ -0,0 +1,61 @@
+# frozen_string_literal: true
+
+RSpec.describe Service do
+  let(:service_class) { Class.new { include Service::Base } }
+
+  describe "Steps" do
+    describe "Model step" do
+      context "when providing default values to step implementation" do
+        before do
+          service_class.class_eval do
+            model :my_model
+
+            def fetch_my_model(default_arg: 2)
+              true
+            end
+          end
+        end
+
+        it "raises an error" do
+          expect { service_class.call }.to raise_error(/In model 'my_model': default values/)
+        end
+      end
+    end
+
+    describe "Policy step" do
+      context "when providing default values to step implementation" do
+        before do
+          service_class.class_eval do
+            policy :my_policy
+
+            def my_policy(default_arg: 2)
+              true
+            end
+          end
+        end
+
+        it "raises an error" do
+          expect { service_class.call }.to raise_error(/In policy 'my_policy': default values/)
+        end
+      end
+    end
+
+    describe "Generic step" do
+      context "when providing default values to step implementation" do
+        before do
+          service_class.class_eval do
+            step :generic_step
+
+            def generic_step(default_arg: 2)
+              true
+            end
+          end
+        end
+
+        it "raises an error" do
+          expect { service_class.call }.to raise_error(/In step 'generic_step': default values/)
+        end
+      end
+    end
+  end
+end