From 9853c90abba213bdc87dccf5620cb0d9eb19c049 Mon Sep 17 00:00:00 2001 From: Maksim Pechnikov Date: Thu, 10 Sep 2020 12:24:44 +0300 Subject: [PATCH 1/2] added paginate links to headers for /chats/:id/messages --- lib/pleroma/chat.ex | 24 +++++-- .../web/api_spec/operations/chat_operation.ex | 3 +- lib/pleroma/web/controller_helper.ex | 20 +++--- .../controllers/chat_controller.ex | 62 ++++++++----------- .../controllers/chat_controller_test.exs | 50 ++++++++++----- 5 files changed, 90 insertions(+), 69 deletions(-) diff --git a/lib/pleroma/chat.ex b/lib/pleroma/chat.ex index 24a86371e..202fffb8a 100644 --- a/lib/pleroma/chat.ex +++ b/lib/pleroma/chat.ex @@ -6,6 +6,7 @@ defmodule Pleroma.Chat do use Ecto.Schema import Ecto.Changeset + import Ecto.Query alias Pleroma.Repo alias Pleroma.User @@ -16,6 +17,7 @@ defmodule Pleroma.Chat do It is a helper only, to make it easy to display a list of chats with other people, ordered by last bump. The actual messages are retrieved by querying the recipients of the ChatMessages. """ + @type t :: %__MODULE__{} @primary_key {:id, FlakeId.Ecto.CompatType, autogenerate: true} schema "chats" do @@ -39,16 +41,28 @@ def changeset(struct, params) do |> unique_constraint(:user_id, name: :chats_user_id_recipient_index) end + @spec get_by_user_and_id(User.t(), FlakeId.Ecto.CompatType.t()) :: + {:ok, t()} | {:error, :not_found} + def get_by_user_and_id(%User{id: user_id}, id) do + from(c in __MODULE__, + where: c.id == ^id, + where: c.user_id == ^user_id + ) + |> Repo.find_resource() + end + + @spec get_by_id(FlakeId.Ecto.CompatType.t()) :: t() | nil def get_by_id(id) do - __MODULE__ - |> Repo.get(id) + Repo.get(__MODULE__, id) end + @spec get(FlakeId.Ecto.CompatType.t(), String.t()) :: t() | nil def get(user_id, recipient) do - __MODULE__ - |> Repo.get_by(user_id: user_id, recipient: recipient) + Repo.get_by(__MODULE__, user_id: user_id, recipient: recipient) end + @spec get_or_create(FlakeId.Ecto.CompatType.t(), String.t()) :: + {:ok, t()} | {:error, Ecto.Changeset.t()} def get_or_create(user_id, recipient) do %__MODULE__{} |> changeset(%{user_id: user_id, recipient: recipient}) @@ -60,6 +74,8 @@ def get_or_create(user_id, recipient) do ) end + @spec bump_or_create(FlakeId.Ecto.CompatType.t(), String.t()) :: + {:ok, t()} | {:error, Ecto.Changeset.t()} def bump_or_create(user_id, recipient) do %__MODULE__{} |> changeset(%{user_id: user_id, recipient: recipient}) diff --git a/lib/pleroma/web/api_spec/operations/chat_operation.ex b/lib/pleroma/web/api_spec/operations/chat_operation.ex index b1a0d26ab..8cbea9ec4 100644 --- a/lib/pleroma/web/api_spec/operations/chat_operation.ex +++ b/lib/pleroma/web/api_spec/operations/chat_operation.ex @@ -158,7 +158,8 @@ def messages_operation do "The messages in the chat", "application/json", chat_messages_response() - ) + ), + 404 => Operation.response("Not Found", "application/json", ApiError) }, security: [ %{ diff --git a/lib/pleroma/web/controller_helper.ex b/lib/pleroma/web/controller_helper.ex index 6445966e0..69188a882 100644 --- a/lib/pleroma/web/controller_helper.ex +++ b/lib/pleroma/web/controller_helper.ex @@ -48,13 +48,13 @@ defp param_to_integer(val, default) when is_binary(val) do defp param_to_integer(_, default), do: default - def add_link_headers(conn, activities, extra_params \\ %{}) + def add_link_headers(conn, entries, extra_params \\ %{}) - def add_link_headers(%{assigns: %{skip_link_headers: true}} = conn, _activities, _extra_params), + def add_link_headers(%{assigns: %{skip_link_headers: true}} = conn, _entries, _extra_params), do: conn - def add_link_headers(conn, activities, extra_params) do - case get_pagination_fields(conn, activities, extra_params) do + def add_link_headers(conn, entries, extra_params) do + case get_pagination_fields(conn, entries, extra_params) do %{"next" => next_url, "prev" => prev_url} -> put_resp_header(conn, "link", "<#{next_url}>; rel=\"next\", <#{prev_url}>; rel=\"prev\"") @@ -78,19 +78,15 @@ defp build_pagination_fields(conn, min_id, max_id, extra_params) do } end - def get_pagination_fields(conn, activities, extra_params \\ %{}) do - case List.last(activities) do + def get_pagination_fields(conn, entries, extra_params \\ %{}) do + case List.last(entries) do %{pagination_id: max_id} when not is_nil(max_id) -> - %{pagination_id: min_id} = - activities - |> List.first() + %{pagination_id: min_id} = List.first(entries) build_pagination_fields(conn, min_id, max_id, extra_params) %{id: max_id} -> - %{id: min_id} = - activities - |> List.first() + %{id: min_id} = List.first(entries) build_pagination_fields(conn, min_id, max_id, extra_params) diff --git a/lib/pleroma/web/pleroma_api/controllers/chat_controller.ex b/lib/pleroma/web/pleroma_api/controllers/chat_controller.ex index e8a1746d4..7b5f3daf9 100644 --- a/lib/pleroma/web/pleroma_api/controllers/chat_controller.ex +++ b/lib/pleroma/web/pleroma_api/controllers/chat_controller.ex @@ -4,6 +4,8 @@ defmodule Pleroma.Web.PleromaAPI.ChatController do use Pleroma.Web, :controller + import Pleroma.Web.ControllerHelper, only: [add_link_headers: 2] + alias Pleroma.Activity alias Pleroma.Chat alias Pleroma.Chat.MessageReference @@ -47,7 +49,7 @@ def delete_message(%{assigns: %{user: %{id: user_id} = user}} = conn, %{ }) do with %MessageReference{} = cm_ref <- MessageReference.get_by_id(message_id), - ^chat_id <- cm_ref.chat_id |> to_string(), + ^chat_id <- to_string(cm_ref.chat_id), %Chat{user_id: ^user_id} <- Chat.get_by_id(chat_id), {:ok, _} <- remove_or_delete(cm_ref, user) do conn @@ -68,18 +70,13 @@ defp remove_or_delete( end end - defp remove_or_delete(cm_ref, _) do - cm_ref - |> MessageReference.delete() - end + defp remove_or_delete(cm_ref, _), do: MessageReference.delete(cm_ref) def post_chat_message( - %{body_params: params, assigns: %{user: %{id: user_id} = user}} = conn, - %{ - id: id - } + %{body_params: params, assigns: %{user: user}} = conn, + %{id: id} ) do - with %Chat{} = chat <- Repo.get_by(Chat, id: id, user_id: user_id), + with {:ok, chat} <- Chat.get_by_user_and_id(user, id), %User{} = recipient <- User.get_cached_by_ap_id(chat.recipient), {:ok, activity} <- CommonAPI.post_chat_message(user, recipient, params[:content], @@ -93,13 +90,12 @@ def post_chat_message( end end - def mark_message_as_read(%{assigns: %{user: %{id: user_id}}} = conn, %{ - id: chat_id, - message_id: message_id - }) do - with %MessageReference{} = cm_ref <- - MessageReference.get_by_id(message_id), - ^chat_id <- cm_ref.chat_id |> to_string(), + def mark_message_as_read( + %{assigns: %{user: %{id: user_id}}} = conn, + %{id: chat_id, message_id: message_id} + ) do + with %MessageReference{} = cm_ref <- MessageReference.get_by_id(message_id), + ^chat_id <- to_string(cm_ref.chat_id), %Chat{user_id: ^user_id} <- Chat.get_by_id(chat_id), {:ok, cm_ref} <- MessageReference.mark_as_read(cm_ref) do conn @@ -109,36 +105,28 @@ def mark_message_as_read(%{assigns: %{user: %{id: user_id}}} = conn, %{ end def mark_as_read( - %{ - body_params: %{last_read_id: last_read_id}, - assigns: %{user: %{id: user_id}} - } = conn, + %{body_params: %{last_read_id: last_read_id}, assigns: %{user: user}} = conn, %{id: id} ) do - with %Chat{} = chat <- Repo.get_by(Chat, id: id, user_id: user_id), - {_n, _} <- - MessageReference.set_all_seen_for_chat(chat, last_read_id) do + with {:ok, chat} <- Chat.get_by_user_and_id(user, id), + {_n, _} <- MessageReference.set_all_seen_for_chat(chat, last_read_id) do conn |> put_view(ChatView) |> render("show.json", chat: chat) end end - def messages(%{assigns: %{user: %{id: user_id}}} = conn, %{id: id} = params) do - with %Chat{} = chat <- Repo.get_by(Chat, id: id, user_id: user_id) do - cm_refs = + def messages(%{assigns: %{user: user}} = conn, %{id: id} = params) do + with {:ok, chat} <- Chat.get_by_user_and_id(user, id) do + chat_message_refs = chat |> MessageReference.for_chat_query() |> Pagination.fetch_paginated(params) conn + |> add_link_headers(chat_message_refs) |> put_view(MessageReferenceView) - |> render("index.json", chat_message_references: cm_refs) - else - _ -> - conn - |> put_status(:not_found) - |> json(%{error: "not found"}) + |> render("index.json", chat_message_references: chat_message_refs) end end @@ -158,8 +146,8 @@ def index(%{assigns: %{user: %{id: user_id} = user}} = conn, _params) do |> render("index.json", chats: chats) end - def create(%{assigns: %{user: user}} = conn, params) do - with %User{ap_id: recipient} <- User.get_by_id(params[:id]), + def create(%{assigns: %{user: user}} = conn, %{id: id}) do + with %User{ap_id: recipient} <- User.get_cached_by_id(id), {:ok, %Chat{} = chat} <- Chat.get_or_create(user.id, recipient) do conn |> put_view(ChatView) @@ -167,8 +155,8 @@ def create(%{assigns: %{user: user}} = conn, params) do end end - def show(%{assigns: %{user: user}} = conn, params) do - with %Chat{} = chat <- Repo.get_by(Chat, user_id: user.id, id: params[:id]) do + def show(%{assigns: %{user: user}} = conn, %{id: id}) do + with {:ok, chat} <- Chat.get_by_user_and_id(user, id) do conn |> put_view(ChatView) |> render("show.json", chat: chat) diff --git a/test/web/pleroma_api/controllers/chat_controller_test.exs b/test/web/pleroma_api/controllers/chat_controller_test.exs index 7be5fe09c..40f7c72ca 100644 --- a/test/web/pleroma_api/controllers/chat_controller_test.exs +++ b/test/web/pleroma_api/controllers/chat_controller_test.exs @@ -2,7 +2,7 @@ # Copyright © 2017-2020 Pleroma Authors # SPDX-License-Identifier: AGPL-3.0-only defmodule Pleroma.Web.PleromaAPI.ChatControllerTest do - use Pleroma.Web.ConnCase, async: true + use Pleroma.Web.ConnCase alias Pleroma.Chat alias Pleroma.Chat.MessageReference @@ -184,17 +184,39 @@ test "it paginates", %{conn: conn, user: user} do chat = Chat.get(user.id, recipient.ap_id) - result = - conn - |> get("/api/v1/pleroma/chats/#{chat.id}/messages") - |> json_response_and_validate_schema(200) + response = get(conn, "/api/v1/pleroma/chats/#{chat.id}/messages") + result = json_response_and_validate_schema(response, 200) + + [next, prev] = get_resp_header(response, "link") |> hd() |> String.split(", ") + api_endpoint = "/api/v1/pleroma/chats/" + + assert String.match?( + next, + ~r(#{api_endpoint}.*/messages\?id=.*&limit=\d+&max_id=.*; rel=\"next\"$) + ) + + assert String.match?( + prev, + ~r(#{api_endpoint}.*/messages\?id=.*&limit=\d+&min_id=.*; rel=\"prev\"$) + ) assert length(result) == 20 - result = - conn - |> get("/api/v1/pleroma/chats/#{chat.id}/messages?max_id=#{List.last(result)["id"]}") - |> json_response_and_validate_schema(200) + response = + get(conn, "/api/v1/pleroma/chats/#{chat.id}/messages?max_id=#{List.last(result)["id"]}") + + result = json_response_and_validate_schema(response, 200) + [next, prev] = get_resp_header(response, "link") |> hd() |> String.split(", ") + + assert String.match?( + next, + ~r(#{api_endpoint}.*/messages\?id=.*&limit=\d+&max_id=.*; rel=\"next\"$) + ) + + assert String.match?( + prev, + ~r(#{api_endpoint}.*/messages\?id=.*&limit=\d+&max_id=.*&min_id=.*; rel=\"prev\"$) + ) assert length(result) == 10 end @@ -223,12 +245,10 @@ test "it returns the messages for a given chat", %{conn: conn, user: user} do assert length(result) == 3 # Trying to get the chat of a different user - result = - conn - |> assign(:user, other_user) - |> get("/api/v1/pleroma/chats/#{chat.id}/messages") - - assert result |> json_response(404) + conn + |> assign(:user, other_user) + |> get("/api/v1/pleroma/chats/#{chat.id}/messages") + |> json_response_and_validate_schema(404) end end From df1eb5cf7ee6d718a28fca3d3715f11ab0e263cb Mon Sep 17 00:00:00 2001 From: lain Date: Tue, 22 Sep 2020 17:17:19 +0200 Subject: [PATCH 2/2] Changelog: Add info about chat pagination fix. --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e61b1d144..e0982b83c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - Removed `:managed_config` option. In practice, it was accidentally removed with 2.0.0 release when frontends were switched to a new configuration mechanism, however it was not officially removed until now. +### Fixed +- Add documented-but-missing chat pagination. ## [2.1.2] - 2020-09-17