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 max error length #473

Merged
merged 3 commits into from Oct 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion CHANGES.rst
Expand Up @@ -6,7 +6,10 @@ Changelog

Minor changes:

- ...
- Calendar.from_ical no longer throws long errors
Ref: #473
Fixes: #472
[jacadzaca]

Breaking changes:

Expand Down
18 changes: 13 additions & 5 deletions src/icalendar/cal.py
Expand Up @@ -390,14 +390,22 @@ def from_ical(cls, st, multiple=False):
if multiple:
return comps
if len(comps) > 1:
raise ValueError(f'Found multiple components where '
f'only one is allowed: {st!r}')
raise ValueError(cls._format_error(
'Found multiple components where only one is allowed', st))
if len(comps) < 1:
raise ValueError(f'Found no components where '
f'exactly one is required: '
f'{st!r}')
raise ValueError(cls._format_error(
'Found no components where exactly one is required', st))
return comps[0]

def _format_error(error_description, bad_input, elipsis='[...]'):
# there's three character more in the error, ie. ' ' x2 and a ':'
Copy link
Member

Choose a reason for hiding this comment

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

I think, a docstring would be nice, also linking to tje issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think a docstring would be redundant and the issue's linked in the commit message.

max_error_length = 100 - 3
jacadzaca marked this conversation as resolved.
Show resolved Hide resolved
if len(error_description) + len(bad_input) + len(elipsis) > max_error_length:
truncate_to = max_error_length - len(error_description) - len(elipsis)
return f'{error_description}: {bad_input[:truncate_to]} {elipsis}'
else:
return f'{error_description}: {bad_input}'

def content_line(self, name, value, sorted=True):
"""Returns property as content line.
"""
Expand Down
41 changes: 41 additions & 0 deletions src/icalendar/tests/calendars/big_bad_calendar.ics
@@ -0,0 +1,41 @@
BEGIN:VCALENDAR
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
BEGIN:VEVENT
END:VEVENT
3 changes: 3 additions & 0 deletions src/icalendar/tests/calendars/small_bad_calendar.ics
@@ -0,0 +1,3 @@
BEGIN:VCALENDAR
BEGIN:VEVENT
END:VEVENT
11 changes: 11 additions & 0 deletions src/icalendar/tests/test_components_break_on_bad_ics.py
Expand Up @@ -26,3 +26,14 @@ def test_rdate_dosent_become_none_on_invalid_input_issue_464(events):
assert events.issue_464_invalid_rdate.is_broken
assert ('RDATE', 'Expected period format, got: 199709T180000Z/PT5H30M') in events.issue_464_invalid_rdate.errors
assert not b'RDATE:None' in events.issue_464_invalid_rdate.to_ical()

@pytest.mark.parametrize('calendar_name', [
'big_bad_calendar',
'small_bad_calendar',
'multiple_calendar_components',
])
def test_error_message_doesnt_get_too_big(calendars, calendar_name):
with pytest.raises(ValueError) as exception:
calendars[calendar_name]
assert len(str(exception).split(': ')[1]) <= 100
Copy link
Collaborator

Choose a reason for hiding this comment

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

That should probably be .split(': ', 1) so a second ': ' does not hide later parts of the message from the assertion.