DEV: removes default service actions (#26078)

Previously services would let you define a high level default `def default_actions_for_service; end` which would define various handlers like `on_success`, after months of usage we consider the cons are superior to the pros here.

Two mains cons:
- people would often not understand where the handling was coming from
- it's easy to miss a case when you write your specs

plugins/chat/app/controllers/chat/api/channel_messages_controller.rb

@@ -4,29 +4,51 @@ class Chat::Api::ChannelMessagesController < Chat::ApiController
   def index
     with_service(::Chat::ListChannelMessages) do
       on_success { render_serialized(result, ::Chat::MessagesSerializer, root: false) }
+      on_failure { render(json: failed_json, status: 422) }
       on_failed_policy(:can_view_channel) { raise Discourse::InvalidAccess }
       on_failed_policy(:target_message_exists) { raise Discourse::NotFound }
       on_model_not_found(:channel) { raise Discourse::NotFound }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 
   def destroy
-    with_service(Chat::TrashMessage) { on_model_not_found(:message) { raise Discourse::NotFound } }
+    with_service(Chat::TrashMessage) do
+      on_success { render(json: success_json) }
+      on_failure { render(json: failed_json, status: 422) }
+      on_model_not_found(:message) { raise Discourse::NotFound }
+      on_failed_policy(:invalid_access) { raise Discourse::InvalidAccess }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
+    end
   end
 
   def restore
     with_service(Chat::RestoreMessage) do
+      on_success { render(json: success_json) }
+      on_failure { render(json: failed_json, status: 422) }
+      on_failed_policy(:invalid_access) { raise Discourse::InvalidAccess }
       on_model_not_found(:message) { raise Discourse::NotFound }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 
   def update
     with_service(Chat::UpdateMessage) do
       on_success { render json: success_json.merge(message_id: result[:message].id) }
+      on_failure { render(json: failed_json, status: 422) }
       on_model_not_found(:message) { raise Discourse::NotFound }
       on_model_errors(:message) do |model|
         render_json_error(model.errors.map(&:full_message).join(", "))
       end
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 
@@ -38,6 +60,7 @@ class Chat::Api::ChannelMessagesController < Chat::ApiController
 
     with_service(Chat::CreateMessage) do
       on_success { render json: success_json.merge(message_id: result[:message_instance].id) }
+      on_failure { render(json: failed_json, status: 422) }
       on_failed_policy(:no_silenced_user) { raise Discourse::InvalidAccess }
       on_model_not_found(:channel) { raise Discourse::NotFound }
       on_failed_policy(:allowed_to_join_channel) { raise Discourse::InvalidAccess }
@@ -55,6 +78,9 @@ class Chat::Api::ChannelMessagesController < Chat::ApiController
       on_model_errors(:message_instance) do |model|
         render_json_error(model.errors.map(&:full_message).join(", "))
       end
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 end

plugins/chat/app/controllers/chat/api/channel_thread_messages_controller.rb

@@ -12,11 +12,14 @@ class Chat::Api::ChannelThreadMessagesController < Chat::ApiController
           include_thread_original_message: false,
         )
       end
-
+      on_failure { render(json: failed_json, status: 422) }
       on_failed_policy(:ensure_thread_enabled) { raise Discourse::NotFound }
       on_failed_policy(:target_message_exists) { raise Discourse::NotFound }
       on_failed_policy(:can_view_thread) { raise Discourse::InvalidAccess }
       on_model_not_found(:thread) { raise Discourse::NotFound }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 end

plugins/chat/app/controllers/chat/api/channel_threads_controller.rb

@@ -22,6 +22,10 @@ class Chat::Api::ChannelThreadsController < Chat::ApiController
       on_failed_policy(:can_view_channel) { raise Discourse::InvalidAccess }
       on_model_not_found(:channel) { raise Discourse::NotFound }
       on_model_not_found(:threads) { render json: success_json.merge(threads: []) }
+      on_failure { render(json: failed_json, status: 422) }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 
@@ -38,9 +42,13 @@ class Chat::Api::ChannelThreadsController < Chat::ApiController
           participants: result.participants,
         )
       end
-
+      on_failed_policy(:invalid_access) { raise Discourse::InvalidAccess }
       on_failed_policy(:threading_enabled_for_channel) { raise Discourse::NotFound }
       on_model_not_found(:thread) { raise Discourse::NotFound }
+      on_failure { render(json: failed_json, status: 422) }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 
@@ -53,6 +61,11 @@ class Chat::Api::ChannelThreadsController < Chat::ApiController
       on_failed_step(:update) do
         render json: failed_json.merge(errors: [result["result.step.update"].error]), status: 422
       end
+      on_success { render(json: success_json) }
+      on_failure { render(json: failed_json, status: 422) }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 
@@ -73,6 +86,10 @@ class Chat::Api::ChannelThreadsController < Chat::ApiController
         render json: failed_json.merge(errors: [result["result.step.create_thread"].error]),
                status: 422
       end
+      on_failure { render(json: failed_json, status: 422) }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 end

plugins/chat/app/controllers/chat/api/channel_threads_current_user_notifications_settings_controller.rb

@@ -13,6 +13,10 @@ class Chat::Api::ChannelThreadsCurrentUserNotificationsSettingsController < Chat
           root: "membership",
         )
       end
+      on_failure { render(json: failed_json, status: 422) }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 end

plugins/chat/app/controllers/chat/api/channels_controller.rb

@@ -34,7 +34,13 @@ class Chat::Api::ChannelsController < Chat::ApiController
 
   def destroy
     with_service Chat::TrashChannel do
+      on_failed_policy(:invalid_access) { raise Discourse::InvalidAccess }
       on_model_not_found(:channel) { raise ActiveRecord::RecordNotFound }
+      on_success { render(json: success_json) }
+      on_failure { render(json: failed_json, status: 422) }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 
@@ -75,6 +81,10 @@ class Chat::Api::ChannelsController < Chat::ApiController
       on_model_errors(:membership) do
         render_json_error(result.membership, type: :record_invalid, status: 422)
       end
+      on_failure { render(json: failed_json, status: 422) }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 

plugins/chat/app/controllers/chat/api/channels_current_user_membership_controller.rb

@@ -12,6 +12,13 @@ class Chat::Api::ChannelsCurrentUserMembershipController < Chat::Api::ChannelsCo
   end
 
   def destroy
-    with_service(Chat::LeaveChannel) { on_model_not_found(:channel) { raise Discourse::NotFound } }
+    with_service(Chat::LeaveChannel) do
+      on_success { render(json: success_json) }
+      on_failure { render(json: failed_json, status: 422) }
+      on_model_not_found(:channel) { raise Discourse::NotFound }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
+    end
   end
 end

plugins/chat/app/controllers/chat/api/channels_current_user_membership_follows_controller.rb

@@ -11,6 +11,10 @@ class Chat::Api::ChannelsCurrentUserMembershipFollowsController < Chat::Api::Cha
         )
       end
       on_model_not_found(:channel) { raise Discourse::NotFound }
+      on_failure { render(json: failed_json, status: 422) }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 end

plugins/chat/app/controllers/chat/api/channels_drafts_controller.rb

@@ -2,6 +2,13 @@
 
 class Chat::Api::ChannelsDraftsController < Chat::ApiController
   def create
-    with_service(Chat::UpsertDraft) { on_model_not_found(:channel) { raise Discourse::NotFound } }
+    with_service(Chat::UpsertDraft) do
+      on_success { render(json: success_json) }
+      on_failure { render(json: failed_json, status: 422) }
+      on_model_not_found(:channel) { raise Discourse::NotFound }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
+    end
   end
 end

plugins/chat/app/controllers/chat/api/channels_invites_controller.rb

@@ -3,8 +3,13 @@
 class Chat::Api::ChannelsInvitesController < Chat::ApiController
   def create
     with_service(Chat::InviteUsersToChannel) do
+      on_success { render(json: success_json) }
+      on_failure { render(json: failed_json, status: 422) }
       on_failed_policy(:can_view_channel) { raise Discourse::InvalidAccess }
       on_model_not_found(:channel) { raise Discourse::NotFound }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 end

plugins/chat/app/controllers/chat/api/channels_memberships_controller.rb

@@ -31,9 +31,14 @@ class Chat::Api::ChannelsMembershipsController < Chat::Api::ChannelsController
 
   def create
     with_service(Chat::AddUsersToChannel) do
+      on_success { render(json: success_json) }
+      on_failure { render(json: failed_json, status: 422) }
       on_failed_policy(:can_add_users_to_channel) do
         render_json_error(I18n.t("chat.errors.users_cant_be_added_to_channel"))
       end
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 end

plugins/chat/app/controllers/chat/api/channels_messages_flags_controller.rb

@@ -5,8 +5,13 @@ class Chat::Api::ChannelsMessagesFlagsController < Chat::ApiController
     RateLimiter.new(current_user, "flag_chat_message", 4, 1.minutes).performed!
 
     with_service(Chat::FlagMessage) do
+      on_success { render(json: success_json) }
+      on_failure { render(json: failed_json, status: 422) }
       on_model_not_found(:message) { raise Discourse::NotFound }
       on_failed_policy(:can_flag_message_in_channel) { raise Discourse::InvalidAccess }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 end

plugins/chat/app/controllers/chat/api/channels_messages_streaming_controller.rb

@@ -3,9 +3,14 @@
 class Chat::Api::ChannelsMessagesStreamingController < Chat::Api::ChannelsController
   def destroy
     with_service(Chat::StopMessageStreaming) do
+      on_success { render(json: success_json) }
+      on_failure { render(json: failed_json, status: 422) }
       on_model_not_found(:message) { raise Discourse::NotFound }
       on_failed_policy(:can_join_channel) { raise Discourse::InvalidAccess }
       on_failed_policy(:can_stop_streaming) { raise Discourse::InvalidAccess }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 end

plugins/chat/app/controllers/chat/api/channels_status_controller.rb

@@ -6,6 +6,10 @@ class Chat::Api::ChannelsStatusController < Chat::Api::ChannelsController
       on_success { render_serialized(result.channel, Chat::ChannelSerializer, root: "channel") }
       on_model_not_found(:channel) { raise ActiveRecord::RecordNotFound }
       on_failed_policy(:check_channel_permission) { raise Discourse::InvalidAccess }
+      on_failure { render(json: failed_json, status: 422) }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 end

plugins/chat/app/controllers/chat/api/channels_threads_drafts_controller.rb

@@ -3,8 +3,13 @@
 class Chat::Api::ChannelsThreadsDraftsController < Chat::ApiController
   def create
     with_service(Chat::UpsertDraft) do
+      on_success { render(json: success_json) }
+      on_failure { render(json: failed_json, status: 422) }
       on_model_not_found(:channel) { raise Discourse::NotFound }
       on_failed_step(:check_thread_exists) { raise Discourse::NotFound }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 end

plugins/chat/app/controllers/chat/api/chatables_controller.rb

@@ -6,6 +6,10 @@ class Chat::Api::ChatablesController < Chat::ApiController
   def index
     with_service(::Chat::SearchChatable) do
       on_success { render_serialized(result, ::Chat::ChatablesSerializer, root: false) }
+      on_failure { render(json: failed_json, status: 422) }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 end

plugins/chat/app/controllers/chat/api/current_user_threads_controller.rb

@@ -19,6 +19,10 @@ class Chat::Api::CurrentUserThreadsController < Chat::ApiController
         )
       end
       on_model_not_found(:threads) { render json: success_json.merge(threads: []) }
+      on_failure { render(json: failed_json, status: 422) }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 end

plugins/chat/app/controllers/chat/api/direct_messages_controller.rb

@@ -30,6 +30,10 @@ class Chat::Api::DirectMessagesController < Chat::ApiController
       on_model_errors(:channel) do |model|
         render_json_error(model, type: :record_invalid, status: 422)
       end
+      on_failure { render(json: failed_json, status: 422) }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 end

plugins/chat/app/controllers/chat/api/reads_controller.rb

@@ -5,12 +5,18 @@ class Chat::Api::ReadsController < Chat::ApiController
     params.require(%i[channel_id message_id])
 
     with_service(Chat::UpdateUserLastRead) do
+      on_success { render(json: success_json) }
+      on_failure { render(json: failed_json, status: 422) }
       on_failed_policy(:ensure_message_id_recency) do
         raise Discourse::InvalidParameters.new(:message_id)
       end
       on_model_not_found(:message) { raise Discourse::NotFound }
       on_model_not_found(:active_membership) { raise Discourse::NotFound }
       on_model_not_found(:channel) { raise Discourse::NotFound }
+      on_failed_policy(:invalid_access) { raise Discourse::InvalidAccess }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 
@@ -19,6 +25,7 @@ class Chat::Api::ReadsController < Chat::ApiController
       on_success do
         render(json: success_json.merge(updated_memberships: result.updated_memberships))
       end
+      on_failure { render(json: failed_json, status: 422) }
     end
   end
 end

plugins/chat/app/controllers/chat/api/thread_reads_controller.rb

@@ -5,7 +5,13 @@ class Chat::Api::ThreadReadsController < Chat::ApiController
     params.require(%i[channel_id thread_id])
 
     with_service(Chat::UpdateUserThreadLastRead) do
+      on_success { render(json: success_json) }
+      on_failure { render(json: failed_json, status: 422) }
       on_model_not_found(:thread) { raise Discourse::NotFound }
+      on_failed_policy(:invalid_access) { raise Discourse::InvalidAccess }
+      on_failed_contract do |contract|
+        render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
+      end
     end
   end
 end

plugins/chat/app/controllers/chat/api_controller.rb

@@ -3,18 +3,5 @@
 module Chat
   class ApiController < ::Chat::BaseController
     include Chat::WithServiceHelper
-
-    private
-
-    def default_actions_for_service
-      proc do
-        on_success { render(json: success_json) }
-        on_failure { render(json: failed_json, status: 422) }
-        on_failed_policy(:invalid_access) { raise Discourse::InvalidAccess }
-        on_failed_contract do |contract|
-          render(json: failed_json.merge(errors: contract.errors.full_messages), status: 400)
-        end
-      end
-    end
   end
 end

plugins/chat/app/controllers/chat/incoming_webhooks_controller.rb

@@ -63,6 +63,7 @@ module Chat
         incoming_chat_webhook: webhook,
       ) do
         on_success { render json: success_json }
+        on_failure { render(json: failed_json, status: 422) }
         on_failed_contract do |contract|
           raise Discourse::InvalidParameters.new(contract.errors.full_messages)
         end

plugins/chat/app/helpers/chat/with_service_helper.rb

@@ -8,14 +8,14 @@ module Chat
     # @param service [Class] A class including {Chat::Service::Base}
     # @param dependencies [kwargs] Any additional params to load into the service context,
     #   in addition to controller @params.
-    def with_service(service, default_actions: true, **dependencies, &block)
+    def with_service(service, **dependencies, &block)
       object = self
-      merged_block =
-        proc do
-          instance_exec(&object.method(:default_actions_for_service).call) if default_actions
-          instance_exec(&(block || proc {}))
-        end
-      ServiceRunner.call(service, object, **dependencies, &merged_block)
+      ServiceRunner.call(
+        service,
+        object,
+        **dependencies,
+        &proc { instance_exec(&(block || proc {})) }
+      )
     end
 
     def run_service(service, dependencies)
@@ -24,9 +24,5 @@ module Chat
       @_result =
         service.call(params.to_unsafe_h.merge(guardian: self.try(:guardian) || nil, **dependencies))
     end
-
-    def default_actions_for_service
-      proc {}
-    end
   end
 end

plugins/chat/app/jobs/regular/chat/auto_join_channel_batch.rb

@@ -5,6 +5,7 @@ module Jobs
     class AutoJoinChannelBatch < ServiceJob
       def execute(args)
         with_service(::Chat::AutoJoinChannelBatch, **args) do
+          on_failure { Rails.logger.error("Failed with unexpected error") }
           on_failed_contract do |contract|
             Rails.logger.error(contract.errors.full_messages.join(", "))
           end

plugins/chat/app/services/chat/lookup_channel_threads.rb

@@ -42,6 +42,13 @@ module Chat
       validates :channel_id, presence: true
 
       attribute :limit, :integer
+      validates :limit,
+                numericality: {
+                  less_than_or_equal_to: THREADS_LIMIT,
+                  only_integer: true,
+                },
+                allow_nil: true
+
       attribute :offset, :integer
     end
 

plugins/chat/lib/service_runner.rb

@@ -27,8 +27,6 @@
 # argument to their block. `on_model_errors` receives the actual model so it’s
 # easier to inspect it.
 #
-# Default actions for each of these are defined in [Chat::ApiController#default_actions_for_service]
-#
 # @example In a controller
 #   def create
 #     with_service MyService do

plugins/chat/spec/requests/chat/api/channel_messages_controller_spec.rb

@@ -28,6 +28,14 @@ RSpec.describe Chat::Api::ChannelMessagesController do
       end
     end
 
+    context "when params are invalid" do
+      it "returns a 400" do
+        get "/chat/api/channels/#{channel.id}/messages?page_size=9999"
+
+        expect(response.status).to eq(400)
+      end
+    end
+
     context "when readonly mode" do
       fab!(:message_1) { Fabricate(:chat_message, chat_channel: channel) }
 

plugins/chat/spec/requests/chat/api/channel_thread_messages_controller_spec.rb

@@ -73,5 +73,13 @@ RSpec.describe Chat::Api::ChannelThreadMessagesController do
         expect(response.status).to eq(404)
       end
     end
+
+    context "when params are invalid" do
+      it "returns a 400" do
+        get "/chat/api/channels/#{thread.channel.id}/threads/#{thread.id}/messages?page_size=9999"
+
+        expect(response.status).to eq(400)
+      end
+    end
   end
 end

plugins/chat/spec/requests/chat/api/channel_threads_controller_spec.rb

@@ -168,6 +168,13 @@ RSpec.describe Chat::Api::ChannelThreadsController do
         expect(response.status).to eq(404)
       end
     end
+
+    context "when params are invalid" do
+      it "returns a 400" do
+        get "/chat/api/channels/#{public_channel.id}/threads?limit=9999"
+        expect(response.status).to eq(400)
+      end
+    end
   end
 
   describe "update" do

plugins/chat/spec/services/chat/lookup_channel_threads_spec.rb

@@ -1,7 +1,13 @@
 # frozen_string_literal: true
 
 RSpec.describe ::Chat::LookupChannelThreads::Contract, type: :model do
-  it { is_expected.to validate_presence_of :channel_id }
+  subject(:contract) { described_class.new(channel_id: 1) }
+
+  it { is_expected.to validate_presence_of(:channel_id) }
+  it { is_expected.to allow_values(1, 0, nil, "a").for(:limit) }
+  it do
+    is_expected.not_to allow_values(::Chat::LookupChannelThreads::THREADS_LIMIT + 1).for(:limit)
+  end
 end
 
 RSpec.describe ::Chat::LookupChannelThreads do
@@ -30,9 +36,7 @@ RSpec.describe ::Chat::LookupChannelThreads do
     context "when limit is over max" do
       let(:limit) { described_class::THREADS_LIMIT + 1 }
 
-      it "defaults to a max value" do
-        expect(result.limit).to eq(described_class::THREADS_LIMIT)
-      end
+      it { is_expected.to fail_a_contract }
     end
 
     context "when limit is under min" do