Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add payload size limit #1151

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 46 additions & 33 deletions lib/cadet_web/controllers/chat_controller.ex
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor nit: Can we combine the if into the with? See https://elixirforum.com/t/proper-way-of-handling-simple-booleans-in-with-statement/45556/4 for an example

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for pointing me to this. Fixed

Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ defmodule CadetWeb.ChatController do
use PhoenixSwagger

alias Cadet.Chatbot.{Conversation, LlmConversations}
@max_content_size 1000

def init_chat(conn, %{"section" => section, "initialContext" => initialContext}) do
user = conn.assigns.current_user
Expand All @@ -21,7 +22,8 @@ defmodule CadetWeb.ChatController do
"conversation_init.json",
%{
conversation_id: conversation.id,
last_message: conversation.messages |> List.last()
last_message: conversation.messages |> List.last(),
max_content_size: @max_content_size
}
)

Expand Down Expand Up @@ -51,45 +53,54 @@ defmodule CadetWeb.ChatController do
response(200, "OK")
response(400, "Missing or invalid parameter(s)")
response(401, "Unauthorized")
response(421, "Message exceeds the maximum allowed length")
yiwen101 marked this conversation as resolved.
Show resolved Hide resolved
response(500, "When OpenAI API returns an error")
end

def chat(conn, %{"conversationId" => conversation_id, "message" => user_message}) do
user = conn.assigns.current_user
if String.length(user_message) > @max_content_size do
send_resp(
conn,
:unprocessable_entity,
"Message exceeds the maximum allowed length of #{@max_content_size}"
)
else
user = conn.assigns.current_user

with {:ok, conversation} <-
LlmConversations.get_conversation_for_user(user.id, conversation_id),
{:ok, updated_conversation} <-
LlmConversations.add_message(conversation, "user", user_message),
payload <- generate_payload(updated_conversation) do
case OpenAI.chat_completion(model: "gpt-4", messages: payload) do
{:ok, result_map} ->
choices = Map.get(result_map, :choices, [])
bot_message = Enum.at(choices, 0)["message"]["content"]

case LlmConversations.add_message(updated_conversation, "assistant", bot_message) do
{:ok, _} ->
render(conn, "conversation.json", %{
conversation_id: conversation_id,
response: bot_message
})

{:error, error_message} ->
send_resp(conn, 500, error_message)
end

{:error, reason} ->
error_message = reason["error"]["message"]
IO.puts("Error message from openAI response: #{error_message}")
LlmConversations.add_error_message(updated_conversation)
send_resp(conn, 500, error_message)
end
else
{:error, {:not_found, error_message}} ->
send_resp(conn, :not_found, error_message)

with {:ok, conversation} <-
LlmConversations.get_conversation_for_user(user.id, conversation_id),
{:ok, updated_conversation} <-
LlmConversations.add_message(conversation, "user", user_message),
payload <- generate_payload(updated_conversation) do
case OpenAI.chat_completion(model: "gpt-4", messages: payload) do
{:ok, result_map} ->
choices = Map.get(result_map, :choices, [])
bot_message = Enum.at(choices, 0)["message"]["content"]

case LlmConversations.add_message(updated_conversation, "assistant", bot_message) do
{:ok, _} ->
render(conn, "conversation.json", %{
conversation_id: conversation_id,
response: bot_message
})

{:error, error_message} ->
send_resp(conn, 500, error_message)
end

{:error, reason} ->
error_message = reason["error"]["message"]
IO.puts("Error message from openAI response: #{error_message}")
LlmConversations.add_error_message(updated_conversation)
{:error, error_message} ->
send_resp(conn, 500, error_message)
end
else
{:error, {:not_found, error_message}} ->
send_resp(conn, :not_found, error_message)

{:error, error_message} ->
send_resp(conn, 500, error_message)
end
end

Expand All @@ -107,4 +118,6 @@ defmodule CadetWeb.ChatController do

conversation.prepend_context ++ messages_payload
end

def max_content_length, do: @max_content_size
end
8 changes: 6 additions & 2 deletions lib/cadet_web/views/chat_view.ex
Original file line number Diff line number Diff line change
@@ -1,8 +1,12 @@
defmodule CadetWeb.ChatView do
use CadetWeb, :view

def render("conversation_init.json", %{conversation_id: id, last_message: last}) do
%{conversationId: id, response: last}
def render("conversation_init.json", %{
conversation_id: id,
last_message: last,
max_content_size: size
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking if there is a need to configure it this way (as opposed to hardcoding), but I am fine with this as well. What do you think?

}) do
%{conversationId: id, response: last, maxContentSize: size}
end

def render("conversation.json", %{conversation_id: id, response: response}) do
Expand Down
18 changes: 18 additions & 0 deletions test/cadet_web/controllers/chat_controller_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,24 @@ defmodule CadetWeb.ChatControllerTest do
end
end

@tag authenticate: :student
@tag requires_setup: true
test "The content length is too long",
%{conn: conn, conversation_id: conversation_id} do
assert conversation_id != nil
max_message_length = ChatController.max_content_length()
message_exceed_length = String.duplicate("a", max_message_length + 1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's also add another testcase to test that String.duplicate("a", max_message_length) passes normally

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added in later commit. However, in current setup the expected result is similar to the "conversation id does not match user id" case. Do you know how to fix this?


conn =
post(conn, "/v2/chats/#{conversation_id}/message", %{
"conversation_id" => conversation_id,
"message" => "#{message_exceed_length}"
})

assert response(conn, 422) ==
"Message exceeds the maximum allowed length of #{max_message_length}"
end

@tag authenticate: :student
test "invalid conversation id", %{conn: conn} do
conversation_id = "-1"
Expand Down
Loading