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.

Sign up for GitHub

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

Jump to bottom

irc: Fix maximal message length #1063

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

irc: Fix maximal message length #1063

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 8 additions & 1 deletion errbot/backends/irc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,13 @@
end_inline_code='')

IRC_NICK_REGEX = r'[a-zA-Z\[\]\\`_\^\{\|\}][a-zA-Z0-9\[\]\\`_\^\{\|\}-]+'
IRC_MESSAGE_SIZE_LIMIT = 510
# According to the RFC the IRC message size can be at most 512 bytes
# Out of these 2 bytes are necessary for '\r\n'.
# At least 10 bytes for 'PRIVMSG <to> :'
# (note that <to> is being taken care of in split_and_send_message
# using MESSAGE_SIZE_LIMIT_INCLUDES_ADDRESEE)
# And one more byte for b'\x03' -- EXT - End Of Text
IRC_MESSAGE_SIZE_LIMIT = 499

try:
import irc.connection
Expand Down Expand Up @@ -669,6 +675,7 @@ def __init__(self, config):
)
self.md = irc_md()
config.MESSAGE_SIZE_LIMIT = IRC_MESSAGE_SIZE_LIMIT
config.MESSAGE_SIZE_LIMIT_INCLUDES_ADDRESEE = True

def send_message(self, msg):
super().send_message(msg)
Expand Down
5 changes: 4 additions & 1 deletion errbot/bootstrap.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,10 @@ def bot_config_defaults(config):
if not hasattr(config, 'DIVERT_TO_PRIVATE'):
config.DIVERT_TO_PRIVATE = ()
if not hasattr(config, 'MESSAGE_SIZE_LIMIT'):
config.MESSAGE_SIZE_LIMIT = 10000 # Corresponds with what HipChat accepts
# Corresponds with what HipChat accepts
config.MESSAGE_SIZE_LIMIT = 10000
if not hasattr(config, 'MESSAGE_SIZE_LIMIT_INCLUDES_ADDRESEE'):
config.MESSAGE_SIZE_LIMIT_INCLUDES_ADDRESEE = False
if not hasattr(config, 'GROUPCHAT_NICK_PREFIXED'):
config.GROUPCHAT_NICK_PREFIXED = False
if not hasattr(config, 'AUTOINSTALL_DEPS'):
Expand Down
9 changes: 8 additions & 1 deletion errbot/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,14 @@ def send_templated(self, identifier, template_name, template_parameters, in_repl
return self.send(identifier, text, in_reply_to, groupchat_nick_reply)

def split_and_send_message(self, msg):
for part in split_string_after(msg.body, self.bot_config.MESSAGE_SIZE_LIMIT):
size_limit = self.bot_config.MESSAGE_SIZE_LIMIT

# If MESSAGE_SIZE_LIMIT_INCLUDES_ADDRESEE is set to True, lower the
# size_limit by the size of the string representation of the addressee
if self.bot_config.MESSAGE_SIZE_LIMIT_INCLUDES_ADDRESEE:
size_limit -= len(str(msg.to))
Copy link
Member

Choose a reason for hiding this comment

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

How about taking a slightly different approach of modifying the signature of split_and_send_message to:

def split_and_send_message(self, msg, max_length=None):
    if max_length is None:
        max_length = self.bot_config.MESSAGE_SIZE_LIMIT

    for part in split_string_after(msg.body, size_limit):
        partial_message = msg.clone()
        partial_message.body = part
        self.send_message(partial_message)

You can then do the max_length -= len(str(msg.to)) calculation in the IRC backend directly and pass max_length to split_and_send_message explicitly. That way it's completely self-contained within the IRC backend.

Copy link
Member Author

Choose a reason for hiding this comment

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

@zoni Sorry for my late reply here.

Your solution would indeed be much better but sadly, split_and_send_message is not called from the IRC backend. The IRC backend could still set some sort of object variable/flag on IRCBackend (which inherits from ErrBot which in turn contains split_and_send_message) that would cause the split_and_send_message function to execute code similar to what there currently is in the PR, without the global config option.

I may also not fully understand what you meant, so please feel free to point out if I missed anything.

Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

@mrshu Hey! Following up for @zoni

I think the goal here is trying to keep this specific IRC code in the backend. While split_and_send_message is not called from IRCBackend, I agree that it still could be self-contained in the backend.

I recommend looking at the SlackBackend's implementation for their size limits:

errbot/errbot/backends/slack.py

Lines 651 to 668 in fe64c25

limit = min(self.bot_config.MESSAGE_SIZE_LIMIT, SLACK_MESSAGE_LIMIT)
parts = self.prepare_message_body(body, limit)
timestamps = []
for part in parts:
data = {
'channel': to_channel_id,
'text': part,
'unfurl_media': 'true',
'link_names': '1',
'as_user': 'true',
}
# Keep the thread_ts to answer to the same thread.
if 'thread_ts' in msg.extras:
data['thread_ts'] = msg.extras['thread_ts']
result = self.api_call('chat.postMessage', data=data)

it's completely self contained. Is there a reason to have a toggleable option for additional size limitation? Wouldn't all IRC messages contain ADDRESEE? or is that only for PRIVMSG? It's bene a while since I've interacted with the IRC backend.


for part in split_string_after(msg.body, size_limit):
partial_message = msg.clone()
partial_message.body = part
self.send_message(partial_message)
Expand Down