Fix update_task wiping unrelated fields on partial updates#10
Open
partymola wants to merge 1 commit into
Open
Conversation
Problem
-------
When a caller updates a single field (e.g. priority), every other field
on the task is silently reset to its default or cleared. Three distinct
bugs combine to cause this:
1. model_dump() includes defaults for unset fields. TaskObject declares
priority with a default of 0, so calling update_task with only a new
title still sends priority=0 to the API, resetting any existing
priority. The same applies to every Optional field that defaults to
None -- dates, reminders, content, etc. are all sent as null.
2. The merge result is never used. Lines 312-313 fetch the existing task
and call task_obj.update(task_object), intending to merge, but line
315 sends task_object.model_dump() (the raw partial input) instead of
the merged task_obj. The merge has no effect.
3. dict.update() with a Pydantic model does not work as intended.
task_obj is a dict (from get_by_id), but task_object is a Pydantic
TaskObject. dict.update() iterates the model's attribute names as
keys, which does not produce a correct field-level merge.
Failure scenarios:
- User sets priority=5 on a task. Later calls update_task to change
only the title. The task loses its priority (reset to 0), its dates,
and its reminders.
- User calls update_task to reschedule a task to a new date. The task's
priority is reset to 0 and its reminders are cleared.
- Any partial update effectively becomes a full replace with defaults
for all unspecified fields.
Additionally, sending the full API-response dict back (which includes
read-only fields like creator, deleted, kind, isFloating) causes HTTP
500 errors from the TickTick API. And reminders returned as objects
[{id, trigger}] must be normalized to strings before sending back.
Fix
---
- Use model_dump(exclude_unset=True) so only explicitly provided fields
are included in the update payload.
- Merge those fields into the existing task fetched via get_by_id().
- Filter the merged dict to only API-accepted fields (UPDATABLE_FIELDS).
- Normalize reminders from object format to string format.
- Add unit tests covering all scenarios above.
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.
Problem
update_tasksilently resets fields that were not included in the update call. Three distinct bugs combine to cause this:1.
model_dump()includes defaults for unset fieldsTaskObjectdeclaresprioritywith a default of0, so callingupdate_taskwith only a new title still sendspriority=0to the API, resetting any existing priority. The same applies to everyOptionalfield that defaults toNone-- dates, reminders, content, etc. are all sent as null.2. The merge result is never used
Lines 312-313 fetch the existing task and call
task_obj.update(task_object), intending to merge, but line 315 sendstask_object.model_dump()(the raw partial input) instead of the mergedtask_obj. The merge has no effect.3.
dict.update()with a Pydantic model doesn't work correctlytask_objis a dict (fromget_by_id), buttask_objectis a PydanticTaskObject.dict.update()iterates the model's attribute names as keys, which does not produce a correct field-level merge.Failure scenarios
priority=5on a task with dates and reminders. Later callsupdate_taskto change only the title. The task loses its priority (reset to 0), its dates, and its reminders.update_taskto reschedule a task to a new date. The task's priority is reset to 0 and its reminders are cleared.Additionally, sending the full API-response dict back (which includes read-only fields like
creator,deleted,kind,isFloating) causes HTTP 500 errors from the TickTick API. And reminders returned as objects[{id, trigger}]must be normalized to strings before sending back.Fix
model_dump(exclude_unset=True)so only explicitly provided fields are included in the update payloadget_by_id()UPDATABLE_FIELDSwhitelist)[{id, trigger}]to string format["TRIGGER:..."]Tests
10 unit tests added covering:
creator,deleted,kind,isFloating,etag, etc.) are filtered outpriority=0is NOT sent when unset (preserves existing priority)priority=0IS sent (overwrites existing)