Fix add object recurrence import#676
Merged
Merged
Conversation
I tried to import some slightly broken icalendar, including a recurrence of a task, containing a RECURRENCE-ID, but without any matching master object containing the RRULE. The library should have just sent the object to the server, but instead it was raising a NotFound-exception. Now the only_this_recurrence-parameter to the save method has become a tristate: True (default) – merge recurrence into master or raise NotFoundError None – merge if master exists, PUT fragment as-is if not False – bypass merge entirely, PUT as-is And add_object() now passes None Quite a lot of fuzz for a weird edge-case. This was done by Claude, but not without a lot of discussions forth and back. Claude is better than any rubber-duck: Prompt: Consider ce42399, I'm not sure I agree on that one. The problem here was that an orphaned recurrence was added to the calendar, things broke, and the claude-session responsible for the import decided to fix add_object() to save things with `only_this_recurrence=False`. Consider if using add_object on an ics fragment containing just a recurrence for an object already existing in the calendar. I think that two things could happen in this case. If the master event was inserted using the caldav library, then the same URL would be used for tossing in the ics fragment, and (dependent on the calendar server behaviour) the calendar server would (most likely) overwrite the original event. If the master event came from some other source, then probably a different URL would be chosen by the caldav library, and most servers would object because we're trying to insert two objects with same UID to the calendar. The correct thing in such a case would be to merge. I think this weird corner case is better fixed by adding some guard/excpetion handling in save() to abort merging and tossing in the ical fragment as-is if the master object cannot be found. Does this make sense? (Claude agreed with the analysis, but responded that the NotFoundError may be desirable when using `save()`) Followup-prompt: Perhaps the better fix is to catch NotFoundError in add_object and retry the save with only_this_recurrence=False on the exception? (Claude responded that it would cause duplicated code due to the async path) Followup-prompt: Oh noes. Perhaps a tristate for the only_this_recurrence is better. (Claude considered it to be a good idea) Followup-prompt: go for it Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Docker supports uid=1001,gid=1001 on tmpfs mounts to set ownership; Podman does not. Dropping those options works because CalendarServer runs as root inside the container and doesn't need the ownership hint. I'm happy to leave most of the CI infrastructure to Claude. Prompt: (after changing from Docker to rootless Podman): Please try to fix ccs and rebuild ox. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
tobixen
force-pushed
the
fix-add-object-recurrence-import
branch
from
e99b2e6 to
5857687
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This needs some rebasing and QA before it can be merged