Repository navigation
feat: wildcard preconditions on no_overwrite / no_create saves - #802
Merged
Merged
Conversation
save(no_overwrite=True) used to search for the UID and then PUT unconditionally, leaving a window in which another client could create the object first. By default the PUT now carries If-None-Match: *, so the server itself refuses an overwrite at that URL (RFC 2616 section 14.26, required of CalDAV servers by RFC 4791 section 2; its section 5.3.2 asks clients to send it). The UID search stays, as it also finds the UID under another URL, unless the new save.no-uid-conflict hint says the server refuses that too; the server profiles get its measured values. Where the new save.if-none-match hint is unsupported, the header is not sent and the old search-then-PUT is used. A 200 reply to a PUT now counts as success. Prompt: Please get a subagent to do the needful on the caldav side for supporting the `If-None-Match: *` [I had a feeling of deja-vu here, so I also asked it to verify that there were no issues on this or forgotten work in some forgotten branch]. Followup-Prompt: (on the review finding that the hint defaults 'full' make unprobed servers get the wildcard headers) Check the RFC. If the `If-Match: *`-behaviour is mandated by the RFC, then keep the default as full ... if not, then set the default unknown ... Followup-Prompt: (on this commit's message) Please rewrite the commit message. Drop information about what servers supports and doesn't support things, just write the default behaviour and how the behaviour can be changed through the server compatbility (sic) hints. Followup-Prompt: (on the diff) This is a BREAKING CHANGE if all of the following conditions are met: * `save(no_overwrite=True)` or `save(no_create=True)` is in use * server implementation deviates from the RFC * server is set up without specifying `features` (or using a hand-specified feature list that wasn't updated) This should be reflected in the changelog, eventually. Followup-Prompt: (on the review document, round 2) Back to the drawing board. `If-None-Match` is required by RFC 9110 released in 2022. I think it's a stretch saying that a CalDAV server that doesn't support RFC 9110 is "broken". Hence, the default should be moved to "unknown", and this will not be a breaking change anymore. [superseded by the Followup-Prompt asking to verify the RFCs: they were checked, and the default stays full] Followup-Prompt: 1) there was some confusion on the RFCs. In the commit message, an RFC from 2022 was referenced. It's fully acceptable for a CalDAV-compliant server to not honor that RFC. Then more research was done, and the latest session decided that a CalDAV server not honoring If-Match and If-None-Match is broken. The latest finding should be verified. If it stays, then "full" should be the default support level, but the commit message has to reference the correct RFC(s). 2) the commit messages was (sic) not good, they were too confusing I think, and a bit too long. To shorten it a bit, I asked for the lists of servers supporting and not supporting the RFC to be dropped. Please look through again and come up with good commit messages explaining the old behaviour, the default new behaviour, and the new behaviour when it's known that the server is not honoring the RFC> (sic) 3) The changelog should not repeat the commit message, it should not give all the details, it suffices to say that those parameters have "improved handling on RFC-compliant servers". And that the change is breaking on non-compliant servers unless the `features` is correctly configured. And then a list of the known non-compliant servers. Followup-Prompt: (on the review finding that the breaking change is not marked feat! or with a BREAKING CHANGE footer) Remark that this is a *potentially* breaking change. We will not bump the version number to 4.0 for this, and the changelog should be clear on this: it's the server and not the client that is broken, and the correct workaround is to tell the client through the configureation (sic) that the server is broken. Followup-Prompt: Was caldav-server-tester updated with those things yet? We need to run the caldav-server-tester and grade all the servers. I will fire up the podman containers. Followup-Prompt: Run them towards the test config under [directory containing test server passwords] Followup-Prompt: ok [agreeing to record the measured save.no-uid-conflict values in the server profiles, with a refusal lacking a recognised precondition graded 'ungraceful'] Assisted-By: Claude Opus 5.5 (1M context) Reviewed-by: Tobias Brox <tobias@redpill-linpro.com>
save(no_create=True) used to search for the UID before the PUT, with the same race window as no_overwrite had. By default the PUT now carries If-Match: * instead, so the server itself refuses to create a missing object (RFC 2616 section 14.24). This checks the URL rather than the UID: an object stored under another name is now refused instead of found. A cached strong ETag or a Schedule-Tag is sent instead of the wildcard, and keeps the search, since a 412 to it cannot tell a deleted object from a changed one; so does a recurrence instance, which is merged into its master. Where the save-load.mutable.if-match-wildcard hint is not supported, the old search-then-PUT is used. Prompt: If I remember correct, there was some similar header to force through the other variant (no_create)? Followup-Prompts: [See the previous commit] Assisted-By: Claude Opus 5.5 (1M context) Reviewed-by: Tobias Brox <tobias@redpill-linpro.com>
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.
Disclaimer: the code and the text below the line are AI-generated, by Claude Opus 5.5, from the prompts (the full prompt record is in the two commit messages):
save(no_overwrite=True)now sendsIf-None-Match: *andsave(no_create=True)sendsIf-Match: *. The server then enforces the flag in the same request as the write, which closes the race window between the old UID search and the PUT. RFC 4791 section 2 requires CalDAV servers to support RFC 2616 ETags, and RFC 2616 sections 14.24/14.26 make both checks a MUST, so the default isfull. On servers known to get them wrong, per thesave.if-none-matchandsave-load.mutable.if-match-wildcardhints, the old search-then-PUT is kept.The new
save.no-uid-conflicthint letsno_overwriteskip the UID search where the server itself refuses a duplicate UID under another URL. The server profiles carry values measured with caldav-server-tester on 21 servers.This is potentially breaking, but only on servers that are themselves broken and not configured as such; the changelog fragment says so and lists the known ones. Unit tests pass, and so do the overwrite/create integration tests against all 21 servers. The matching caldav-server-tester probes are on its
feat/probe-create-preconditionsbranch.