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

Added option to skip objects the remote fails to provide #866

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hvanderheide
Copy link

To deal with a broken CalDAV implementation (and all-around terrible product) SmarterMail, I needed to be able to skip remote objects that are listed but can't be queried. It even fails to render them in its own calendar webinterface. It seems to fail on certain special characters that can be introduced by external calendar invites. There is little chance it will be fixed upstream, so this seemed the best solution.

Does this feature make sense, as does its implementation?

@@ -367,7 +367,7 @@ def init_and_remaining_args(cls, **kwargs):

def __init__(self, url, username='', password='', verify=True, auth=None,
useragent=USERAGENT, verify_fingerprint=None,
auth_cert=None):
auth_cert=None, ignore_missing_href=False):
Copy link
Member

Choose a reason for hiding this comment

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

Don't you need to somehow pass this data to this function?

I guess it should be a setting for the storage, is that what you had a mind?

If so, that would be an acceptable change.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @WhyNotHugo, yes correct. I have this in my config:
[storage example]
...
ignore_missing_href = true

I'm not sure about the name of the option. It probably won't be obvious what it does for someone that hasn't read the code

@dilyanpalauzov
Copy link
Contributor

Some CalDAV servers announce URLs with @ , but want %40 in the request (and vice-versa). You might want to percent-encode/decode the characters to find out what the server wants.

@NickHu
Copy link

NickHu commented Oct 31, 2022

I've encountered this problem with Google Calendar's CalDAV implementation too. I believe that the steps to reproduce it are as follows:

  1. import some 'bad' .ics file not from Google Calendar;
  2. vdirsyncer will choke on trying to download the .ics file corresponding to the new Google Calendar event - indeed, the API endpoint itself returns 404.

Debug log:

error: Unknown error occurred for calendar_gcal/<redacted>: /caldav/v2/<redacted>/events/%[email protected]
error: Use `-vdebug` to see the full traceback.
debug:   File "/nix/store/16aybf8fy7n176hhlb38j9ynqcivkwy8-python3.10-vdirsyncer-0.19.0/lib/python3.10/site-packages/vdirsyncer/cli/tasks.py", line 72, in sync_collection
debug:     await sync.sync(
debug:   File "/nix/store/16aybf8fy7n176hhlb38j9ynqcivkwy8-python3.10-vdirsyncer-0.19.0/lib/python3.10/site-packages/vdirsyncer/sync/__init__.py", line 144, in sync
debug:     a_nonempty = await a_info.prepare_new_status()
debug:   File "/nix/store/16aybf8fy7n176hhlb38j9ynqcivkwy8-python3.10-vdirsyncer-0.19.0/lib/python3.10/site-packages/vdirsyncer/sync/__init__.py", line 62, in prepare_new_status
debug:     async for href, item, etag in self.storage.get_multi(prefetch):
debug:   File "/nix/store/16aybf8fy7n176hhlb38j9ynqcivkwy8-python3.10-vdirsyncer-0.19.0/lib/python3.10/site-packages/vdirsyncer/storage/dav.py", line 555, in get_multi
debug:     raise exceptions.NotFoundError(href)

I don't know what ICS files are considered 'bad', but perhaps it has something to do with the %2B in there - perhaps it needs to be not URL encoded to resolve properly. Another hypothesis is that the UID field of the ICS containing a / causes it to be bad.

The contents of this PR are a workaround.

@WhyNotHugo
Copy link
Member

A "bad" ICS file would be very useful to reproduce this.

@vwegert
Copy link

vwegert commented Dec 10, 2023

@hvanderheide In your opinion, would this patch fix #1095 as well or is that a different issue?

@WhyNotHugo
Copy link
Member

I don't really know how to test this patch, but it does make sense IMHO.

Have you tested it reliably?

@WhyNotHugo
Copy link
Member

Knowing how to reproduce the underlying issue would help test this.

@vwegert
Copy link

vwegert commented Feb 27, 2024

Have you tested it reliably?

I have not tested this exact patch, but the modification outlined in #1095 which is similar in function, but not configurable. I've had this patch in place since Dec 10th and have had no issues related to this since.

Knowing how to reproduce the underlying issue would help test this.

I completely agree - but at least in my case, I have no idea how the Google backend got into the state I'm seeing.

@hvanderheide
Copy link
Author

This PR is already quite old and for me personally obsolete as I moved to a different job. I'm happy to close it if it has little to no value.

@vwegert I agree that it looks like a similar use-case.

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

Successfully merging this pull request may close these issues.

5 participants