From: aescling <aescling+gitlab@cat.family>
Date: Tue, 6 Jun 2023 16:46:09 +0000 (-0400)
Subject: Improve error handling in PostStatusService
X-Git-Url: https://git.xn--scling-oua.cat.family/?a=commitdiff_plain;h=33c45e90f6750f7a34ccf1c994fdf8b46e881e4e;p=mastodon.git

Improve error handling in PostStatusService

* Explicitly catch invalid post visibility, and 422 instead of 500
        - ~~Reject "limited" visibility~~
        - Accept empty visibilities, ignoring them
* Explicitly alert the user when a post is rejected for having both
  media and polls

The former currently allows the visibility property to be blank or null.
I do not know if this is more desirable than, for example, rejecting
empty strings.

Additionally to the above, improve documentation for the call method.

* Improve consistency in documentation
* Improve rubocop assessment in new code we are responsible for
        - Improve documentation

* Correct scheduled post parse error semantics
        - Return 422, not 400
        - Respond with a useful error message

* Allow creating limited visibility posts from the API again

* Add appropriate yard tags to documentation

* Correct erronous documentation, copy-editing it somewhat
	- Use a labelled list for Status#visibility
	- Improve specificity of "make a note of that"

* Remove PostStatusService#visibility_invalid?
	- !visibility_invalid? -> !visibility_valid?
---

diff --git a/app/models/status.rb b/app/models/status.rb
index 7f74cb9da..dea138db7 100644
--- a/app/models/status.rb
+++ b/app/models/status.rb
@@ -50,37 +50,47 @@ class Status < ApplicationRecord
 
   update_index('statuses', :proper)
 
-  # @!attribute [rw] status
-  #   Status visibility affects the audience of a post, its federation
-  #   behavior, and what interactions are allowed on the post.
+  # @!attribute [rw] visibility
+  #   A {https://api.rubyonrails.org/v6.1.7.4/classes/ActiveRecord/Enum.html
+  #   Rails enum}. Status visibility affects the audience of a post, federation
+  #   behavior for the post, and what user interactions are allowed on the
+  #   post. The documented visibilities are as follows:
   #
-  #   A public post is sent to the public timeline; remote instances with their
-  #   own public timelines will also send the post to them. In addition, public
-  #   posts are sent to following users' home timelines. Any user may see a
-  #   public post, and depending on instance configuration, the post may be
-  #   visible via a permalink without any form of user authentication.
+  #   public::
+  #     Sent to the public timeline; remote instances with their
+  #     own public timelines will also send the post to them. In addition,
+  #     public posts are sent to following users' home timelines. Any user may
+  #     see a public post, and depending on instance configuration, the post
+  #     may be visible via a permalink without any form of user authentication.
   #
-  #   An unlisted post is not sent to public timelines, but otherwise has the
-  #   same behavior as a public post.
+  #   unlisted::
+  #     Not sent to public timelines, but otherwise has the same behavior as
+  #     a public post.
   #
-  #   A private post is only visible to following users. Consequently, it
-  #   cannot be boosted, and its permalink is always guarded by user
-  #   authentication.
+  #   private::
+  #     Only visible to following users. Consequently, it cannot be boosted,
+  #     and its permalink is always guarded by user authentication.
   #
-  #   A direct post is a direct message (DM); it is only visible to mentioned
-  #   accounts, and will additionally be sent to the users' direct timelines.
-  #   Its behavior is otherwise like that of a private post.
+  #   direct::
+  #     A direct message (DM). Only visible to mentioned accounts, and will
+  #     additionally be sent to the users' direct timelines. Its behavior is
+  #     otherwise like that of a private post.
   #
-  #   The "limited" visibility is for the visibility of Google+-like circles,
-  #   which ActivityPub strictly supports, and which some Mastodon-compatible
-  #   software does curretly use. As such posts would otherwise have to be
-  #   interpreted as DMs, the limited visibility was created alongside the
-  #   ability to create "silent" mentions, so that a post may explicitly
-  #   specify its entire audience in its mentions without necessarily notifying
-  #   those mentioned users, and without sending the post to any direct
-  #   timelines. The behavior is otherwise like that of a private post;
-  #   limited visibility posts are even serialized to clients as if they were
-  #   private!
+  #   There is an additional, undocumented, "limited" visibility, explained
+  #   below:
+  #
+  #   limited::
+  #     Undocumented, but strictly allowed in the API. Used internally to
+  #     represent statuses whose audience is explicily spelled out in the manner
+  #     of Google+ circles, which ActivityPub strictly supports, and which some
+  #     Mastodon-compatible software does curretly allow. As such posts would
+  #     otherwise have to be interpreted as DMs in order to respect the indeded
+  #     audience, the limited visibility was created alongside the concept of
+  #     "silent" mentions, so that a post may explicitly specify its audience
+  #     in its mentions without necessarily notifying those mentioned users, and
+  #     without sending the post to any direct timelines. The behavior is
+  #     otherwise like that of a private post; limited visibility posts are even
+  #     serialized to clients as if they were private!
   #
   #   @see https://github.com/mastodon/mastodon/pull/8950 Mastodon PR #8950:
   #     Improve support for aspects/circles
@@ -94,7 +104,8 @@ class Status < ApplicationRecord
   #     timeline.
   #
   #   @return [:public, :unlisted, :private, :direct, :limited] The status
-  #     visibility
+  #     visibility, as a {https://api.rubyonrails.org/v6.1.7.4/classes/ActiveRecord/Enum.html
+  #     Rails enum}.
   enum visibility: [:public, :unlisted, :private, :direct, :limited], _suffix: :visibility
 
   belongs_to :application, class_name: 'Doorkeeper::Application', optional: true
diff --git a/app/services/post_status_service.rb b/app/services/post_status_service.rb
index 36592a531..a4cacf0c4 100644
--- a/app/services/post_status_service.rb
+++ b/app/services/post_status_service.rb
@@ -6,21 +6,56 @@ class PostStatusService < BaseService
 
   MIN_SCHEDULE_OFFSET = 5.minutes.freeze
 
-  # Post a text status update, fetch and notify remote users mentioned
+  # Validate and, if appropriate, post or schedule a text status update,
+  # fetching and notifying any mentioned remote users. If the new status is
+  # in reply to an account the user is not already following, make a note of
+  # that for account recommendation purposes.
+  #
+  # @note It is invalid to create a post with media attachments and a poll at
+  #   the same time.
+  #
+  # @note Attempting to schedule a status in the past will simply post the
+  #   status immediately.
+  #
+  # @note This is undocumented in the upstream API documentation, but
+  #   attempting to create a status with limited visibility is allowed.
+  #   That is to say, sending a POST request to the status creation endpoint
+  #   with the visibility attribute set to "limited" will create a status
+  #   with the (undocumented) "limited" visibility! The implication of this
+  #   are nonobvious, and, frankly, not at all useful; see {Status#visibility}
+  #   for the details.
+  #
   # @param [Account] account Account from which to post
-  # @param [Hash] options
-  # @option [String] :text Message
-  # @option [Status] :thread Optional status to reply to
-  # @option [Boolean] :sensitive
-  # @option [String] :visibility
-  # @option [String] :spoiler_text
-  # @option [String] :language
-  # @option [String] :scheduled_at
-  # @option [Hash] :poll Optional poll to attach
-  # @option [Enumerable] :media_ids Optional array of media IDs to attach
-  # @option [Doorkeeper::Application] :application
-  # @option [String] :idempotency Optional idempotency key
-  # @option [Boolean] :with_rate_limit
+  # @param [Hash] options Options hash
+  # @option options [String] :text Message to post. Defaults to the empty
+  #   string, unless there is nonblank spoiler text, in which case a single
+  #   period is used for text posts, or an appropriate emoji for media
+  # @option options [Status] :thread Optional status to reply to
+  # @option options [Boolean] :sensitive Optional; forced to true if
+  #   spoiler_text is present. Defaults to the user's default post sensitivity
+  #   setting
+  # @option options [String] :visibility Optional; if present, SHOULD be one of
+  #   "public", "unlisted", "private", or "direct", but see {Status#visibility}.
+  #   Defaults to the user's default post privacy.
+  # @option options [String] :spoiler_text Optional content warning
+  # @option options [String] :language Optional two-letter language code. If
+  #   blank, invalid, or unknown to the backend, defaults to the user's default
+  #   post language if it exists, and then to the default locale
+  # @option options [String] :scheduled_at Optional; if present, should be a
+  #   valid timestamp, and should refer to a time at least {MIN_SCHEDULE_OFFSET}
+  #   into the future. (If determined to be scheduled in the past, the status
+  #   will be created immediately!)
+  # @option options [Hash] :poll Optional poll to attach. Invalid to be present
+  #   when media attachments are also present
+  # @option options [Enumerable] :media_ids Optional array of media IDs to
+  #   attach. Invalid to be present when a poll is also present
+  # @option options [Doorkeeper::Application] :application Optional application
+  #   the status was posted from
+  # @option options [String] :idempotency Optional idempotency key, preventing
+  #   this status from being created if a status with this key already exists
+  # @option options [Boolean] :with_rate_limit Strictly optional, but should be
+  #   forced to true inside a controller. Can be safely ignored if, for
+  #   example, creating a status as part of a test
   # @return [Status]
   def call(account, options = {})
     @account     = account
@@ -60,12 +95,17 @@ class PostStatusService < BaseService
      end
     end
     @sensitive    = (@options[:sensitive].nil? ? @account.user&.setting_default_sensitive : @options[:sensitive]) || @options[:spoiler_text].present?
+
+    if !visibility_valid?
+      raise Mastodon::ValidationError, I18n.t('statuses.validations.visibility', visibility: @options[:visibility])
+    end
+
     @visibility   = @options[:visibility] || @account.user&.setting_default_privacy
     @visibility   = :unlisted if @visibility&.to_sym == :public && @account.silenced?
     @scheduled_at = @options[:scheduled_at]&.to_datetime
     @scheduled_at = nil if scheduled_in_the_past?
   rescue ArgumentError
-    raise ActiveRecord::RecordInvalid
+    raise Mastodon::ValidationError, I18n.t('statuses.validations.scheduled_at', datetime: @options[:scheduled_at])
   end
 
   def process_status!
@@ -114,7 +154,8 @@ class PostStatusService < BaseService
       return
     end
 
-    raise Mastodon::ValidationError, I18n.t('media_attachments.validations.too_many') if @options[:media_ids].size > 4 || @options[:poll].present?
+    raise Mastodon::ValidationError, I18n.t('media_attachments.validations.poll') if @options[:poll].present?
+    raise Mastodon::ValidationError, I18n.t('media_attachments.validations.too_many') if @options[:media_ids].size > 4
 
     @media = @account.media_attachments.where(status_id: nil).where(id: @options[:media_ids].take(4).map(&:to_i))
 
@@ -158,6 +199,10 @@ class PostStatusService < BaseService
     @scheduled_at.present? && @scheduled_at <= Time.now.utc + MIN_SCHEDULE_OFFSET
   end
 
+  # If the status was made in reply to an account the user is not already
+  # following, increase the number of recorded interactions that user has had
+  # with the author of the replied status, for account recommendation purposes.
+  # @return void
   def bump_potential_friendship!
     return if !@status.reply? || @account.id == @status.in_reply_to_account_id
     ActivityTracker.increment('activity:interactions')
@@ -205,4 +250,17 @@ class PostStatusService < BaseService
       options_hash[:with_rate_limit] = false
     end
   end
+
+  # If a visibility is present in the options hash, ensure it represents a
+  # proper status visibility
+  # @example Behaviors that are implicitly specified by Mastodon
+  #   @options[:visibility] = nil; visibility_vaild?              # => true
+  #   @options[:visibility] = ""; visibility_vaild?               # => true
+  #   @options[:visibility] = "         "; visibility_vaild?      # => true
+  #   @options[:visibility] = "antohusathue"; visibility_invalid? # => true
+  #   @options[:visibility] = "limited"; visibility_vaild?        # => true
+  # @return [Boolean]
+  def visibility_valid?
+    @options[:visibility].nil? || @options[:visibility].blank? || Status.visibilities.key?(@options[:visibility])
+  end
 end
diff --git a/config/locales-kibicat/en.yml b/config/locales-kibicat/en.yml
index 1491cafbf..d200633ab 100644
--- a/config/locales-kibicat/en.yml
+++ b/config/locales-kibicat/en.yml
@@ -1,2 +1,9 @@
 ---
 en:
+  media_attachments:
+    validations:
+      poll: Cannot attach media and polls to the same status
+  statuses:
+    validations:
+      scheduled_at: 'Invalid scheduled datetime "%{datetime}"'
+      visibility: 'Invalid status visibility "%{visibility}"'