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

Exception Handling and Debugging #82

Open
tfinke opened this issue Jan 19, 2021 · 35 comments
Open

Exception Handling and Debugging #82

tfinke opened this issue Jan 19, 2021 · 35 comments

Comments

@tfinke
Copy link

tfinke commented Jan 19, 2021

First of all let me thank you for this project. A CLI interface to Caldav (especially direct without sync) is very useful and urgently needed.

Exceptions thrown by the class packages used should be catched. For example the "vobject" class throws this ParseError on the ICS-Entry attached below:

File "/usr/local/bin/calendar-cli.py", line 976, in
main()
File "/usr/local/bin/calendar-cli.py", line 967, in main
return args.func(caldav_conn, args)
File "/usr/local/bin/calendar-cli.py", line 487, in calendar_agenda
if hasattr(event_cal.instance, 'vtimezone'):
File "/usr/lib/python3.6/site-packages/caldav/objects.py", line 1058, in _get_vobject_instance
self._set_vobject_instance(vobject.readOne(to_unicode(self._get_data())))
File "/usr/lib/python3.6/site-packages/vobject/base.py", line 1153, in readOne
allowQP))
File "/usr/lib/python3.6/site-packages/vobject/base.py", line 1125, in readComponents
component.transformChildrenToNative()
File "/usr/lib/python3.6/site-packages/vobject/base.py", line 674, in transformChildrenToNative
child.transformChildrenToNative()
File "/usr/lib/python3.6/site-packages/vobject/base.py", line 673, in transformChildrenToNative
child = child.transformToNative()
File "/usr/lib/python3.6/site-packages/vobject/base.py", line 185, in transformToNative
return self.behavior.transformToNative(self)
File "/usr/lib/python3.6/site-packages/vobject/icalendar.py", line 1386, in transformToNative
raise ParseError("DURATION must have a single duration string.")
vobject.base.ParseError: At line 16: DURATION must have a single duration string.

Unfortunately neither vobject nor calendar-cli tells the UID of the event. So identifying it is rather complicated.
In general no entry should cause a crash, but should only display a warning for that entry in order to handle all other entries properly.

So may I suggest to catch any exception and display a message containing the entry UID.

Also it would be helpful for debugging purposes to implement reading ICS-entries from stdin (as mentioned according to the option "--icalendar", which is not implemented yet.

Again thanks for your project and best regards

Finke

foo.txt

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021

As for the UID, it can be displayed by using a template ... see the EXAMPLE file. The uid is needed for editing or deleting stuff using the calendar_cli. Perhaps it would be an idea to have a --show-uid parameter, but that should be raised as a separate issue.

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021

Also it would be helpful for debugging purposes to implement reading ICS-entries from stdin (as mentioned according to the option "--icalendar", which is not implemented yet.

Hmm ... for taking some ical file and push it into a calendar there is the "calendar addics"-command.

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021

Ideally all expected errors should be caught and handled, but for unexpected exceptions I think it's better to show the full traceback, it makes debugging easier - so I think I would rather prefer separate issues for every ugly traceback that can be provoked.

In this case I understand it so that the caldav server has generated some icalendar that the vobject library does not like. The culprit is either that the caldav server breaks the standard or that the vobject doesn't support the standard well enough (I haven't studied the icalendar data yet). What caldav server do you use?

Ideally the problem should be solved either at the server side or in vobject, but there are some few lines in the caldav library hacking away known server standard breaches, we can probably do a workaround there.

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021

Oh ... "duration" is not even mentioned in the icalendar code attached. I'll see if I can reproduce it.

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021

I did this:

>>> import vobject
>>> with open('foo.txt', 'r') as f:
...     data=f.read()
>>> vobject.readOne(data)
<VCALENDAR(...)>

so the attached icalendar code did not break anything for me. I'm having vobject 0.9.6 installed.

@tfinke
Copy link
Author

tfinke commented Jan 19, 2021

As for the UID, it can be displayed by using a template ... see the EXAMPLE file. The uid is needed for editing or deleting stuff using the calendar_cli. Perhaps it would be an idea to have a --show-uid parameter, but that should be raised as a separate issue.

that would be useful. But the UID will not be shown on crash anyway :-(

@tfinke
Copy link
Author

tfinke commented Jan 19, 2021

I did this:

>>> import vobject
>>> with open('foo.txt', 'r') as f:
...     data=f.read()
>>> vobject.readOne(data)
<VCALENDAR(...)>

so the attached icalendar code did not break anything for me. I'm having vobject 0.9.6 installed.

you are right. I have tested it also and could not reproduce the crash by reading the file.

Maybe it was another entry, although I tried carefully to identify it. How could I see which entry caused the crash?

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021

I think there is nothing that can be done in the calendar-cli layer for identifying the data here, though it can be done in the caldav layer. I'll have a look into it.

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021

The best would probably be to hack up vobject to print out the icalendar data

@tfinke
Copy link
Author

tfinke commented Jan 19, 2021

Ideally all expected errors should be caught and handled, but for unexpected exceptions I think it's better to show the full traceback, it makes debugging easier - so I think I would rather prefer separate issues for every ugly traceback that can be provoked.

from the development point of view this is true. But for the user the traceback is not really helpful. Indeed every class developer should explain the error as precise as possible when throwing exceptions. In this case the error message from vobject is not useful at all.

In this case I understand it so that the caldav server has generated some icalendar that the vobject library does not like. The culprit is either that the caldav server breaks the standard or that the vobject doesn't support the standard well enough (I haven't studied the icalendar data yet). What caldav server do you use?

we run davical. Other clients (Evolution, Lightning, several mobile devices) have no problem on this issue.

Ideally the problem should be solved either at the server side or in vobject, but there are some few lines in the caldav library hacking away known server standard breaches, we can probably do a workaround there.

I had a look to the code but found it not very self-explanatory. I failed to even identify the reason of the crash.

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021

from the development point of view this is true. But for the user the traceback is not really helpful.

For most users, probably yes, but I think a generic error message like "something went wrong" is even less helpful.

The biggest problem is probably that tracebacks can be intimidating for the average Joe Doe, but then again I believe most users of calendar-cli is technically minded. If they don't get any smarter by reading the traceback, at least I expect that a significant amount of the users would be able to find their way here to the issue tracker and post it here.

I think I've seen that DURATION-error before actually, also with some icalendar data without DURATION set, though I think there was a due field that was wrongly set.

tobixen added a commit to python-caldav/caldav that referenced this issue Jan 19, 2021
…es not give enough information to do any proper debugging
@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021

Can you try installing the git master version of the caldav library?

@tfinke
Copy link
Author

tfinke commented Jan 19, 2021

from the development point of view this is true. But for the user the traceback is not really helpful.

For most users, probably yes, but I think a generic error message like "something went wrong" is even less helpful.

I agree.

The biggest problem is probably that tracebacks can be intimidating for the average Joe Doe, but then again I believe most users of calendar-cli is technically minded. If they don't get any smarter by reading the traceback, at least I expect that a significant amount of the users would be able to find their way here to the issue tracker and post it here.

:-) that's why I came here

I think I've seen that DURATION-error before actually, also with some icalendar data without DURATION set, though I think there was a due field that was wrongly set.

I have checked it again. It really seems to be the entry I posted above. There is no DURATION nor DUE. Hmm...

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021

I did a small change in the caldav library printing the URL of the object. You may also replace url with data to make sure it's the same data there.

@tfinke
Copy link
Author

tfinke commented Jan 19, 2021

Can you try installing the git master version of the caldav library?

of course, but I have to find out how to do this parallel to my already installed packages (I have caldav 0.7.1 installed via pip)

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021

if you don't mind overwriting the existing caldav (it should be quite save), then you can do "sudo ./setup.py install" in the working directory of the checked out caldav code.

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021

Or, eventually, you can just edit the installed file.

>>> import caldav
>>> caldav.__file__
'/usr/lib/python3.9/site-packages/caldav-0.8.0rc0-py3.9.egg/caldav/__init__.py'

@tfinke
Copy link
Author

tfinke commented Jan 19, 2021

Or, eventually, you can just edit the installed file.

>>> import caldav
>>> caldav.__file__
'/usr/lib/python3.9/site-packages/caldav-0.8.0rc0-py3.9.egg/caldav/__init__.py'

I am sorry - I am not very familiar with the Python ecosystem. Now I have:

  • /usr/lib/python3.6/site-packages/caldav (installed via PIP)
  • /usr/lib/python3.6/site-packages/caldav-0.8.0rc0-py3.6.egg/caldav (via setup.py)
    How do I force calendar-cli to use the new one?

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021

uff :-( This is a bit outside my area of competence, I've never understood that egg thing. I think the sanest way to solve this is to do a pip uninstall. the setup.py doesn't have an uninstall, but I think it's just to delete /usr/lib/python3.9/site-packages/caldav-0.8.0rc0-py3.9.egg/

@tfinke
Copy link
Author

tfinke commented Jan 19, 2021

uff :-( This is a bit outside my area of competence, I've never understood that egg thing.

this calms me :-)

I think the sanest way to solve this is to do a pip uninstall. the setup.py doesn't have an uninstall, but I think it's just to delete >/usr/lib/python3.9/site-packages/caldav-0.8.0rc0-py3.9.egg/

will try that

I did not understand:

... you can just edit the installed file
...

caldav.file
'/usr/lib/python3.9/site-packages/caldav-0.8.0rc0-py3.9.egg/caldav/init.py'

which file to edit?

@tfinke
Copy link
Author

tfinke commented Jan 19, 2021

After installing the new caldav master I recieve:

Traceback (most recent call last):
File "/usr/lib/python3.6/site-packages/caldav-0.8.0rc0-py3.6.egg/caldav/objects.py", line 1233, in _get_vobject_instance
self._set_vobject_instance(vobject.readOne(to_unicode(self._get_data())))
File "/usr/lib/python3.6/site-packages/vobject/base.py", line 1153, in readOne
allowQP))
File "/usr/lib/python3.6/site-packages/vobject/base.py", line 1125, in readComponents
component.transformChildrenToNative()
File "/usr/lib/python3.6/site-packages/vobject/base.py", line 674, in transformChildrenToNative
child.transformChildrenToNative()
File "/usr/lib/python3.6/site-packages/vobject/base.py", line 673, in transformChildrenToNative
child = child.transformToNative()
File "/usr/lib/python3.6/site-packages/vobject/base.py", line 185, in transformToNative
return self.behavior.transformToNative(self)
File "/usr/lib/python3.6/site-packages/vobject/icalendar.py", line 1386, in transformToNative
raise ParseError("DURATION must have a single duration string.")
vobject.base.ParseError: At line 16: DURATION must have a single duration string.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
File "/usr/local/bin/calendar-cli.py", line 976, in
main()
File "/usr/local/bin/calendar-cli.py", line 967, in main
return args.func(caldav_conn, args)
File "/usr/local/bin/calendar-cli.py", line 487, in calendar_agenda
if hasattr(event_cal.instance, 'vtimezone'):
File "/usr/lib/python3.6/site-packages/caldav-0.8.0rc0-py3.6.egg/caldav/objects.py", line 1235, in _get_vobject_instance
logging.critical("Something went wrong while loading icalendar data into the vobject class. ical url: " + self.url)
TypeError: must be str, not URL

seems to be a very stubborn issue

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021

oh, that's the problem with hacking things without testing. Should be str(self.url) apparently. (I had expected python would figure that out by itself, but unfortunately not). I've committed a fix.

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021

For hot-patching, it's just to read the traceback to figure out what files and what lines to edit. In the case above, line 1235 in /usr/lib/python3.6/site-packages/caldav-0.8.0rc0-py3.6.egg/caldav/objects.py and/or line 1386 in /usr/lib/python3.6/site-packages/vobject/icalendar.py. I suppose it should be possible to reinstall the official version of the library for a clean rollback.

@tfinke
Copy link
Author

tfinke commented Jan 19, 2021

That did not change much:

Traceback (most recent call last):
File "/usr/lib/python3.6/site-packages/caldav-0.8.0rc0-py3.6.egg/caldav/objects.py", line 1233, in _get_vobject_instance
self._set_vobject_instance(vobject.readOne(to_unicode(self._get_data())))
File "/usr/lib/python3.6/site-packages/vobject/base.py", line 1153, in readOne
allowQP))
File "/usr/lib/python3.6/site-packages/vobject/base.py", line 1125, in readComponents
component.transformChildrenToNative()
File "/usr/lib/python3.6/site-packages/vobject/base.py", line 674, in transformChildrenToNative
child.transformChildrenToNative()
File "/usr/lib/python3.6/site-packages/vobject/base.py", line 673, in transformChildrenToNative
child = child.transformToNative()
File "/usr/lib/python3.6/site-packages/vobject/base.py", line 185, in transformToNative
return self.behavior.transformToNative(self)
File "/usr/lib/python3.6/site-packages/vobject/icalendar.py", line 1386, in transformToNative
raise ParseError("DURATION must have a single duration string.")
vobject.base.ParseError: At line 16: DURATION must have a single duration string.

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
File "/usr/local/bin/calendar-cli.py", line 976, in
main()
File "/usr/local/bin/calendar-cli.py", line 967, in main
return args.func(caldav_conn, args)
File "/usr/local/bin/calendar-cli.py", line 487, in calendar_agenda
if hasattr(event_cal.instance, 'vtimezone'):
File "/usr/lib/python3.6/site-packages/caldav-0.8.0rc0-py3.6.egg/caldav/objects.py", line 1235, in _get_vobject_instance
logging.critical("Something went wrong while loading icalendar data into the vobject class. ical url: " + self.url)
TypeError: must be str, not URL

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021

Did you do "git pull" and "sudo python ./setup.py install"?

Eventually, you can just edit line 1235 in your /usr/lib/python3.6/site-packages/caldav-0.8.0rc0-py3.6.egg/caldav/objects.py and change self.url to str(self.url). And if that doesn't help, change self.url to self.data.

@tfinke
Copy link
Author

tfinke commented Jan 19, 2021

sorry, my mistake (I had updated calendar-cli itself - stupid). Now I have installed the recent version of caldav. It displayed:

CRITICAL:root:Something went wrong while loading icalendar data into the vobject class. ical url: ...

That is exactly the entry I suspected.

A difference to the 3-liner above might be:

vobject.readOne(to_unicode(self._get_data())

May there be an issue with "to_unicode"?

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021 via email

@tfinke
Copy link
Author

tfinke commented Jan 19, 2021

you are right - it has no effect.

@tfinke
Copy link
Author

tfinke commented Jan 19, 2021

I had a closer look to vobject::Duration::transformToNative
I disabled raising of the exception 'raise ParseError("DURATION must ...")' and returned a default duration.
Now the entry shows up correctly.
It seems, that "Duration" is expected to return a valid value.

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021

Oh, I said "due", but that's for vtodos. for vcalendar, duration is supposed to be the equivalent of the difference between dtstart and dtend - so either a dtend is expected, or duration. Probably.

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021

There is one event in that ical that looks a bit suspicious:

UID:20170920T111700Z-171533-100-1-8@foo
DTSTART:20170922T120000
DTEND;TZID=/freeassociation.sourceforge.net/Europe/Berlin:
 20170922T140000

So DTSTART is a "naive" timestamp without time zone, while DTEND is a timestamp with time zone. That may possibly be an issue. Still, very strange that it breaks when called up from the caldav library, but not when tested manually.

@tfinke
Copy link
Author

tfinke commented Jan 19, 2021

this may lead to the reason. I found that a duration of only "P" when calling transformToNative(). Normally some time should be displayed after "P". Other entries do so. Maybe DTSTART is considered UTC and DTEND CEST. The difference is two hours which is exactly the length of the event. So the single "P" may denote a duration of zero. The question is, whether "vobject" should accept this. I will try to contact it's maintainers.

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021 via email

@tfinke
Copy link
Author

tfinke commented Jan 19, 2021

it would be useful to have the exception reproducable. It seems, that the 3-liner does not even call the complete function stack like
calendar.cli does. Strange, since both actually call readOne().

@tobixen
Copy link
Owner

tobixen commented Jan 19, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants