Use uppercase field names for VTODO in caldav

[FrnchFrgg]

As per RFC5545 ical property names are always uppercase, and the caldav library we use automatically uppercases them while producing the ICAL stream.

But this means that different casings (including the default values from the library) are treated as different dict keys and will be separately iterated on. This can create duplicate entries which are commonly forbidden by RFC5545.

Use only uppercase in property names to alleviate this risk.

Breaking change

Proposed change

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• New feature (which adds functionality to an existing integration)
• Deprecation (breaking change to happen in the future)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Additional information

• This PR fixes or closes issue: fixes Adding a CALDAV todo item with the radicale server does not work #105383
• This PR is related to issue: Adding a CALDAV todo item with the radicale server does not work #105383
• Link to documentation pull request:

Checklist

• The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the development checklist
• I have followed the perfect PR recommendations
• The code has been formatted using Ruff (ruff format homeassistant tests)
• Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.
• Untested files have been added to .coveragerc.

To help with the load of incoming pull requests:

• I have reviewed two other open pull requests in this repository.

[FrnchFrgg]

Note that the issue should be solved upstream by python-caldav/caldav#354 which maps all property names to lowercase to remove duplicates.

It seems that the caldav maintainers consider that the authoritative case in their API arguments is lowercase even though is is output as uppercase in the ICAL data itself.

This fix, while not harmful even with later caldav releases, is thus a bit contrary to the library API ethos.

On the other hand, no caldav release contains the fix yet and we may want to merge this workaround at least temporarily (the caldav maintainer told in python-caldav/caldav#355 that a minor release should come very soon).

[tobixen]

It seems that the caldav maintainers consider that the authoritative case in their API arguments is lowercase even though is is output as uppercase in the ICAL data itself.

When the arguments are coming in as parameters to a python function or method, then lowercase seems a lot more appropriate than uppercase to me - in other context, uppercase would be more appropriate.

The primary task of the caldav library is to do the caldav communication, hence it should be mostrly agnostic to the icalendar content. In this case I've added a convenience-method for adding events or tasks to the calendar without the need of the caller to generate the icalendar content. There was a specific bug here adding a default status line when creating tasks, without considering that the input parameter would come in as lower case.

When accessing the icalendar data, the recommended practice is to work on it through the icalendar library. In the icalendar library, the icalendar object is stored as a case-insensitive dict, meaning that the keys can be accessed either as upper case or lower case (or even CamelCase if one would prefer that).

I hope to make a new release of the caldav library later this weekend or early next week, including this bugfix (and quite some other minor bugfixes).

[allenporter]

Yes, it makes sense to me that python function arguments should be lowercase. So it sounds like we consider this a bug in python-caldav and a fix is incoming?

Would it be better to assert on the rfc5545 content in the test catch this type of issue? or keep that as an implementation detail of python-caldav

(Also this PR has test failures)

[FrnchFrgg]

Note that I also agree that arguments are prettier in lowercase. I was just pointing out that my workaround was going against the (IMHO correct) design choices of caldav API and thus should be only temporary if ever.

[allenporter]

Thanks for clarifying. I think pulling in the upstream library bump sounds right given there is already a fix that was committed and it just needs to be released.

[allenporter]

I think there are some additional problems with how we're using save_todo call here also from
#105246

The due field when using update expects an ICS string, but when using save the due field expects a datetime or date.

[allenporter]

Is there a better way to use the Todo API so that we can call it the same way on create and update? I had a little trouble navigating the best way to handle this.

[DerFlob]

Is there a better way to use the Todo API so that we can call it the same way on create and update? I had a little trouble navigating the best way to handle this.

I had a little go at that issue yesterday as well. The best I did come up with was always returning a date/datetime from the _to_ics_fields helper method. That will fix the save_todo method.
And for the update case, instead of directly using the returned dict in the .update of the icalender_component, I called .set_due with the date/datetime value, popping it from the returned dict and then using the rest of it in the .update.

Imho, that has the advantage, that the caldav library will be handling correct date/time formatting. It unfortunately broke the tests as well because of the different date handling.
I didn't get around to fix the tests and wasn't 100% happy or sure if it is the right way to use it.

See #105435

[tobixen]

Is there a better way to use the Todo API so that we can call it the same way on create and update? I had a little trouble navigating the best way to handle this.

I'm not sure how this is done today, but the recommended way to update fields in an event or task is like this:

event_or_task.icalendar_component[field] = new_value
event_or_task.save()

The icalendar_component will return an object from the icalendar library. Case of the field parameter does not matter. Timestamps are a bit tricky, it has to be done like this:

task['DUE'].dt = datetime.now() + timedelta(days=2)
task.save()

The convenience-method task.set_due (there seems to be no equivalent set_dtend, that's a bug) will handle cases where DURATION is set. (According to the RFC, DURATION cannot be combined with DTEND or DUE. It's a bit annoying. I'd like DTSTART to be the earliest possible time I expect to start with a task, DUE to be the hard due date, and DURATION to be the estimated time for doing the task. I've personally ended up with using DTSTART as the latest possible time it's possible to start with the task, assuming I will work on this one rather than any of the overlapping events and tasks, but it feels a bit silly. If I have some paperwork that should be done this year, I think that DUE is at midnight the new years eve, but obviously it would be better to get the task done some days in advance, as I may have other plans for the new years eve).

The plan is to move set_due et al to the icalendar library, as I think they belong there - but I do care a lot about backward-compatibility, so they will stay in the caldav library for many years to come.

If someone sees room for improvement in the API, then feel free to shout out. Having save_todo to accept different kind of datetimes, i.e. ICS strings or elements from the icalendar library seems to me to be a no-brainer - raise an issue or a pull request at the caldav library and I'll try to have it out in the next minor-release.

[allenporter]

@tobixen is there an API to use that can share code when setting fields on initial create and update? That is I care less about if it's setting strings or objects just want to have a simple way to do it the same way in both scenarios. While there are lots of examples in the code, they seem to show very different ways for these two scenarios so I assume I'm missing something basic. Thanks!

[allenporter]

#105435 is a date fix showing the difference needed for create vs update.

[tobixen]

@tobixen is there an API to use that can share code when setting fields on initial create and update?

As for now, no - but we could make something. The obvious thing would be to create event_or_task.update(**kwargs), where the same arguments can be used when creating and updating an event or task.

I think it would make sense to try to put as much as possible of the actual logic into the icalendar library rather than the caldav library.

[tobixen]

v1.3.8 is out.

[allenporter]

Awesome thank you @tobixen !
@FrnchFrgg want to try to see if the bump addresses the issue?

[edenhaus]

Drafting it as the following needs to be done first:

• the question of Allen needs to be answered first
• merge conflicts needs to be resolved
• tests are failing

Please mark it as ready for review afterwards

[FrnchFrgg]

Awesome thank you @tobixen ! @FrnchFrgg want to try to see if the bump addresses the issue?

Testing locally within a python interpreter no longer raises the error and correctly adds the task which is then visible by HomeAssistant and all other caldav clients.

I'd say this is fixed by the new caldav release. Do you want me to force-update the caldav library within my home assistant instance ? I should be able to do so using the privileged SSH addon.

[FrnchFrgg]

#105508 submitted instead of this PR which is unneeded I think.