Skip to content

feat(services/webdav): support conditional delete with if-match - #8296

Open
YuangGao wants to merge 3 commits into
apache:mainfrom
YuangGao:webdav-conditional-delete
Open

YuangGao wants to merge 3 commits into
apache:mainfrom
YuangGao:webdav-conditional-delete

Conversation

@YuangGao

@YuangGao YuangGao commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of #8157.

Rationale for this change

The webdav service already supports delete but ignored the OpDelete argument, so If-Match never reached the DELETE request. RFC 9110 defines If-Match for state-changing methods including DELETE, and RFC 4918 recommends entity-tag conditions for WebDAV modifications.

What changes are included in this PR?

  • Inject If-Match in webdav_delete.
  • Declare delete_with_if_match, gated by a new enable_conditional_delete option defaulting to true, mirroring enable_conditional_read from feat(services/webdav): support conditional read headers #7637.
  • Return ConditionNotMatch instead of success when a conditional delete gets 404, matching s3 and azblob.
  • Disable the capability in CI for the four nginx cases and for owncloud.

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_mismatch failed two of three runs there. Both keep the capability off.

The delete path deliberately does not pass with_if_match to parse_error. SabreDAV answers a conditional delete of a missing target with 412, which that read-oriented carve-out would report as NotFound instead of ConditionNotMatch.

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_delete is a new config option, default true. No breaking changes.

AI Usage Statement

AI-assisted implementation.

@YuangGao
YuangGao marked this pull request as draft September 14, 2026 06:20
@YuangGao
YuangGao marked this pull request as ready for review September 14, 2026 06:23
@YuangGao
YuangGao force-pushed the webdav-conditional-delete branch from 06dab9c to 17d2877 Compare September 14, 2026 06:28
Comment thread core/services/webdav/src/config.rs Outdated
/// locally instead of deleting a revision the caller never observed.
///
/// Default: true
pub enable_conditional_delete: bool,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sounds good, that makes sense, I will have a separate PR to deprecate enable_conditional_read

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@github-actions github-actions Bot added releases-note/feat The PR implements a new feature or has a title that begins with "feat" services/webdav size:M This PR changes 30-99 lines, ignoring generated files. labels Sep 16, 2026
@YuangGao

Copy link
Copy Markdown
Contributor Author

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:

PUT "version one"   -> etag E
PUT "version two"   -> same second, still E
DELETE If-Match: E  -> 204, gone

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

releases-note/feat The PR implements a new feature or has a title that begins with "feat" services/webdav size:M This PR changes 30-99 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants