-
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
Conversation
%{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 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
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.
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?
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 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?
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 the with
? See https://elixirforum.com/t/proper-way-of-handling-simple-booleans-in-with-statement/45556/4 for an example
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.
Thank you for pointing me to this. Fixed
Closes #1150