fix(calendar): stop rebuilding a VTODO from the partial update dict on error - #1377
Open
thejdubb02 wants to merge 1 commit into
Open
fix(calendar): stop rebuilding a VTODO from the partial update dict on error#1377thejdubb02 wants to merge 1 commit into
thejdubb02 wants to merge 1 commit into
Conversation
…n error `_merge_ical_todo_properties` ended in a catch-all that returned `_create_ical_todo(todo_data, todo_uid)`. On the update path `todo_data` carries only the fields the caller passed, so any exception at all — including a transient parse failure on an otherwise intact resource — silently replaced the stored todo with one built from that partial dict, dropping summary, due date, dtstart, the completed timestamp, categories, RRULE, alarms and every custom property the caller did not happen to re-send, while reporting success. This is the defect removed from the event merge in cbcoutinho#544. `_merge_ical_properties` raises instead, and its docstring records why; the todo path never got the same treatment. It had already been half-fixed — `except ValueError: raise` was added with a comment pointing at that precedent — but everything else still fell through to the rebuild. `todo_uid` goes with it. Like `event_uid` before it, the parameter existed only to feed the fallback; the UID carries through from the stored iCal as a preserved property. The bulk of the diff is the dedent from dropping the `try:`. `?w=1` shows the change is 35 lines. Fixes cbcoutinho#1252
|
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
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.



Fixes #1252.
_merge_ical_todo_propertiesended in a catch-all that returned_create_ical_todo(todo_data, todo_uid). On the update pathtodo_dataholds only thefields the caller passed, so any exception replaced the stored todo with one rebuilt from
that partial dict — dropping summary, due date, dtstart, the completed timestamp,
categories, RRULE, alarms and every custom property the caller did not re-send — and
reported success.
This is the same defect removed from the event merge in #544.
_merge_ical_propertiesraises instead and its docstring records why; the todo path never got the same treatment.
It was already half-fixed —
except ValueError: raisewas added with a comment pointingat that precedent — but every other exception still fell through to the rebuild.
todo_uidgoes with it, for the reasonevent_uiddid: the parameter existed only tofeed the fallback, and the UID carries through from the stored iCal as a preserved
property.
Reviewing
Most of the diff is the dedent from dropping the
try:.?w=1shows the real change: 35 lines.Test
test_a_non_value_error_does_not_silently_rebuild_the_todopasses a malformed reminder,which raises
AttributeError— precisely what the catch-all swallowed. It fails onmasterand passes with this change. No mocking; the input is real.On propagating instead of catching
One reviewer raised whether removing a catch-all risks unhandled exceptions reaching the
client. Worth stating what I checked:
nc_calendar_update_todocatches onlyDavPreconditionFailedand lets everything else propagate, and the event path hasbehaved this way since #544. This change introduces no exception path that is not already
live for events. The alternative traded that for guaranteed silent data loss.
Provenance
Written with AI assistance, then verified rather than trusted:
pre-existing on untouched
master(missing optional test deps in my environment), notintroduced here.
ruff checkreports one fewer error thanmaster;ruff format --checkis clean.the one dissent is answered above.
Happy to split the whitespace-only dedent into its own commit if that reads better.