Refactor status check to uniformly handle Multi-Status responses#171
Open
rfc2822 wants to merge 2 commits into
Open
Refactor status check to uniformly handle Multi-Status responses#171rfc2822 wants to merge 2 commits into
rfc2822 wants to merge 2 commits into
Conversation
- Replace manual `HttpStatusCode.MultiStatus`/`HTTP_MULTISTATUS` checks with unified `multiStatusIsError` parameter - Update `checkStatus` signature in both Ktor and OkHttp implementations - Remove redundant comments and simplify error handling logic
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors checkStatus in both OkHttp and Ktor DavResource implementations to centralize handling of HTTP 207 (Multi-Status), allowing callers (notably mkCol) to uniformly treat 207 as an error when required (per #167 / RFC guidance).
Changes:
- Added a
multiStatusIsErrorparameter tocheckStatusto optionally treat HTTP 207 as an error. - Updated MOVE/COPY/MKCOL/DELETE call sites to pass
multiStatusIsError = truewhere 207 must fail the operation. - Simplified method bodies by removing duplicated 207 checks and related inline comments.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/main/kotlin/at/bitfire/dav4jvm/okhttp/DavResource.kt |
Adds multiStatusIsError to checkStatus and routes MOVE/COPY/MKCOL/DELETE through it to throw on 207. |
src/main/kotlin/at/bitfire/dav4jvm/ktor/DavResource.kt |
Mirrors the same checkStatus refactor and updates relevant request methods to treat 207 as error. |
Comments suppressed due to low confidence (2)
src/main/kotlin/at/bitfire/dav4jvm/okhttp/DavResource.kt:655
- Changing
checkStatusfromcheckStatus(response: Response)tocheckStatus(response: Response, multiStatusIsError: Boolean = false)is a binary-incompatible change for any external subclasses compiled against the previous protected method signature (and also breaks Java callers/subclasses, because no 1-arg overload exists). Consider keeping a 1-argument overload that delegates to the new method (or using@JvmOverloads) to preserve compatibility while still supporting the new flag.
protected fun checkStatus(response: Response, multiStatusIsError: Boolean = false) {
// handle 2xx response codes
if (response.code / 100 == 2) {
if (response.code == HTTP_MULTISTATUS && multiStatusIsError)
throw HttpException(response)
src/main/kotlin/at/bitfire/dav4jvm/ktor/DavResource.kt:566
- Changing
checkStatusto add themultiStatusIsErrorparameter is a binary-incompatible change for external subclasses compiled against the previous protected signature, and it’s also awkward to call from Java (no 1-arg overload is generated for default params). Consider keeping a 1-argument overload delegating to the new method (or adding@JvmOverloads) to preserve compatibility.
protected suspend fun checkStatus(response: HttpResponse, multiStatusIsError: Boolean = false) {
// handle 2xx response codes
if (response.status.isSuccess()) {
if (response.status == HttpStatusCode.MultiStatus && multiStatusIsError)
throw HttpException.fromResponse(response)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
This pull request refactors the
checkStatusfunction to handle Multi-Status responses uniformly across implementations.Key changes:
multiStatusIsErrorparameter to replace manualHttpStatusCode.MultiStatus/HTTP_MULTISTATUSchecksmultiStatusIsError = trueformkcol()(fixes MKCOL/MKCALENDAR: 207 Multi-Status should be treated as error #167)