Conversation
06dab9c to
17d2877
Compare
| /// locally instead of deleting a revision the caller never observed. | ||
| /// | ||
| /// Default: true | ||
| pub enable_conditional_delete: bool, |
There was a problem hiding this comment.
I think having an enable config on a client is weird. It's not something we can control. I suggest we remove or deprecate all settings that follow this pattern.
There was a problem hiding this comment.
sounds good, that makes sense, I will have a separate PR to deprecate enable_conditional_read
|
the red core / webdav found something real. nextcloud's etag comes from mtime at one-second resolution, same as owncloud. so two writes in the same second share an etag — 13 of 20 rapid overwrites came back unchanged, even though the bodies differed in content and length. which means a stale etag can delete a revision you never saw. 6 of 10 tries against nextcloud: that's the race this capability was meant to remove, and none of the representative servers survive it — nginx-dav has no etag at all, owncloud and nextcloud are both one-second. i left ci red on purpose instead of overriding nextcloud too, since that would hide the problem while we still advertise the capability. @Xuanwo so should i drop the capability and note this on #8157, or do you see a deployment-side CapabilityOverrideLayer as enough? |
Which issue does this PR close?
Part of #8157.
Rationale for this change
The webdav service already supports delete but ignored the
OpDeleteargument, soIf-Matchnever reached the DELETE request. RFC 9110 definesIf-Matchfor state-changing methods including DELETE, and RFC 4918 recommends entity-tag conditions for WebDAV modifications.What changes are included in this PR?
If-Matchinwebdav_delete.delete_with_if_match, gated by a newenable_conditional_deleteoption defaulting to true, mirroringenable_conditional_readfrom feat(services/webdav): support conditional read headers #7637.ConditionNotMatchinstead of success when a conditional delete gets 404, matching s3 and azblob.Verified against the three servers CI covers. Nextcloud passes the full behavior suite with the capability on. nginx-dav omits ETags from PROPFIND. ownCloud derives ETags from a one-second mtime, so two writes in the same second share one tag and a stale ETag still matches;
test_delete_with_if_match_mismatchfailed two of three runs there. Both keep the capability off.The delete path deliberately does not pass
with_if_matchtoparse_error. SabreDAV answers a conditional delete of a missing target with412, which that read-oriented carve-out would report asNotFoundinstead ofConditionNotMatch.Are there any user-facing changes?
Yes —
op.delete_with(path).if_match(etag)now works against WebDAV servers that expose a usable ETag.enable_conditional_deleteis a new config option, default true. No breaking changes.AI Usage Statement
AI-assisted implementation.