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

fix: google may return empty ical #366

Merged
merged 2 commits into from
Dec 26, 2023

Conversation

zhwei
Copy link
Contributor

@zhwei zhwei commented Dec 26, 2023

something like:

BEGIN:VCALENDAR
PRODID:-//Google Inc//Google Calendar 70.9054//EN
VERSION:2.0
CALSCALE:GREGORIAN
X-WR-CALNAME:TEST
X-WR-TIMEZONE:Asia/Shanghai
BEGIN:VTIMEZONE
TZID:Asia/Shanghai
X-LIC-LOCATION:Asia/Shanghai
BEGIN:STANDARD
TZOFFSETFROM:+0800
TZOFFSETTO:+0800
TZNAME:CST
DTSTART:19700101T000000
END:STANDARD
END:VTIMEZONE
END:VCALENDAR

something like:

```
BEGIN:VCALENDAR
PRODID:-//Google Inc//Google Calendar 70.9054//EN
VERSION:2.0
CALSCALE:GREGORIAN
X-WR-CALNAME:TEST
X-WR-TIMEZONE:Asia/Shanghai
BEGIN:VTIMEZONE
TZID:Asia/Shanghai
X-LIC-LOCATION:Asia/Shanghai
BEGIN:STANDARD
TZOFFSETFROM:+0800
TZOFFSETTO:+0800
TZNAME:CST
DTSTART:19700101T000000
END:STANDARD
END:VTIMEZONE
END:VCALENDAR
```
@tobixen
Copy link
Member

tobixen commented Dec 26, 2023

Please use o.is_loaded() to check if the object has a payload or not, it's more efficient. errata: I see now that o.is_loaded() will return True, so it's no good. errata: I also see that this construct is used elsewhere.

I remember that this issue (google returning empty objects) have been looked into a couple of times, but I don't remember the conclusion. Will o.load() help?

@tobixen
Copy link
Member

tobixen commented Dec 26, 2023

I'm a bit baffled why the tests break. I'm looking into it.

@tobixen
Copy link
Member

tobixen commented Dec 26, 2023

I do not like the usage of o.icalendar_component here, becuase it may involve converting the object into an object from the icalendar library. Anyway, I can see that it's already used in the same function, after if kwargs.get("expand", False):, so then my objection is moot. :-) I should probably go over and refactor at a later stage. It's still needed to figure out why the test breaks though.

@zhwei
Copy link
Contributor Author

zhwei commented Dec 26, 2023

@tobixen Thanks for your review.

Will o.load() help?

It has no effect, the original data have been loaded, but have no event/todo/etc.

FYI:
this event looks like a event's recurrence item, id format is idexample001_R20231222@google.com, i cannot reproduce this event, but my user have 😞 .
The weird thing is i call search with expand=False, this recurrence item should not return standalone. If do not pass sort_keys parameter, the return result is like

Event(https://...../idexample001@google.com.ics)
Event(https://...../idexample001_R20231222@google.com.ics)

@tobixen
Copy link
Member

tobixen commented Dec 26, 2023

I promised to look into why the test breaks - but then I got interrupted and busy. I will try and have a look now.

@tobixen
Copy link
Member

tobixen commented Dec 26, 2023

Sorry for overwriting your changes. I think it will be better this way. I think the special logic for dealing with objects that aren't loaded or doesn't contain components is not specific to when expand=True, and hence it should be outside the if.

I consider this to be a bugfix, as it will cause consistent and expected behaviour even when the caldav server doesn't conform to the RFCs.

@tobixen tobixen force-pushed the dev-fix-google-return-empty-event branch from 647d842 to b7a5553 Compare December 26, 2023 19:12
Some logic was only run when the expand-flag was set to True:

1) The logic automatically loading objects that aren't loaded

2) The logic removing "empty" responses from Google,

This logic ensures a consistent return from the search-method also for servers
not conforming to the RFC (by returning unloaded data or empty responses)

The second was fixed in the previous commit, but it feels wrong converting the
data to an icalendar object unless it's needed.
@tobixen tobixen force-pushed the dev-fix-google-return-empty-event branch from b7a5553 to 4c143c0 Compare December 26, 2023 19:20
@tobixen tobixen added this pull request to the merge queue Dec 26, 2023
Merged via the queue into python-caldav:master with commit 4daf65f Dec 26, 2023
7 checks passed
@tobixen
Copy link
Member

tobixen commented Dec 26, 2023

Could you confirm that this solves your problems?

@zhwei
Copy link
Contributor Author

zhwei commented Dec 27, 2023

@tobixen Yes, it's works for me.

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.

2 participants