-
Notifications
You must be signed in to change notification settings - Fork 50
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
base: master
Are you sure you want to change the base?
Add payload size limit #1151
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -76,6 +76,41 @@ 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's also add another testcase to test that There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, :unprocessable_entity) == | ||
"Message exceeds the maximum allowed length of #{max_message_length}" | ||
end | ||
|
||
@tag authenticate: :student | ||
@tag requires_setup: true | ||
test "The content length less than the maximum allowed length but conversation belongs to another user", | ||
%{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) | ||
|
||
conn = | ||
post(conn, "/v2/chats/#{conversation_id}/message", %{ | ||
"conversation_id" => conversation_id, | ||
"message" => "#{message_exceed_length}" | ||
}) | ||
|
||
assert response(conn, :not_found) == "Conversation not found" | ||
end | ||
|
||
@tag authenticate: :student | ||
test "invalid conversation id", %{conn: conn} do | ||
conversation_id = "-1" | ||
|
There was a problem hiding this comment.
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 thewith
? See https://elixirforum.com/t/proper-way-of-handling-simple-booleans-in-with-statement/45556/4 for an exampleThere was a problem hiding this comment.
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