Skip to content

Upgrade okhttp to 5.1.0#74

Merged
rfc2822 merged 12 commits intomainfrom
73-update-okhttp-to-5x
Jul 17, 2025
Merged

Upgrade okhttp to 5.1.0#74
rfc2822 merged 12 commits intomainfrom
73-update-okhttp-to-5x

Conversation

@ArnyminerZ
Copy link
Member

@ArnyminerZ ArnyminerZ commented Jul 14, 2025

There were some things changed on MockWebServer. Mainly that MockResponse now is immutable, so MockResponse.Builder exists; some function names have been changed.

Also Response.body now is never null, so in order to check whether a body has been set or not, I've checked whether the response length is 0, and whether it has a content type. If none of this conditions apply, body is not set. This is done like this instead of simply checking for content length because when content type is not set we automatically assume it's XML.

Signed-off-by: Arnau Mora <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ self-assigned this Jul 14, 2025
@ArnyminerZ ArnyminerZ added the refactoring Internal improvement of existing functions label Jul 14, 2025
@ArnyminerZ ArnyminerZ linked an issue Jul 14, 2025 that may be closed by this pull request
2 tasks
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ requested a review from Copilot July 14, 2025 07:10

This comment was marked as outdated.

@rfc2822
Copy link
Member

rfc2822 commented Jul 14, 2025

Very cool!

Is it possible to keep API the same (assertMultiStatus not inline / without body, it's purpose is just to throw an Exception if needed)?

When it's ready, please request review from @sunkup.

Signed-off-by: Arnau Mora <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ requested a review from Copilot July 16, 2025 06:02

This comment was marked as outdated.

Signed-off-by: Arnau Mora <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ requested a review from Copilot July 16, 2025 06:09

This comment was marked as outdated.

Signed-off-by: Arnau Mora <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ requested a review from Copilot July 16, 2025 06:14
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Updates the project to use OkHttp 5.1.0 and migrates MockWebServer tests to the new immutable MockResponse.Builder API, replaces shutdown() with close(), and adds logic in DavResource to detect empty response bodies.

  • Migrate tests to mockwebserver3.MockResponse.Builder and update assertions to use new APIs.
  • Add isEmptyResponseBody helper in DavResource to handle non-null Response.body.
  • Validate and adjust multi-status responses in DavResource.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

File Description
src/test/kotlin/at/bitfire/dav4jvm/... Tests updated to use MockResponse.Builder and close().
src/main/kotlin/at/bitfire/dav4jvm/DavResource.kt Added isEmptyResponseBody and adjusted assertMultiStatus.
gradle/libs.versions.toml Upgraded okhttp and mockwebserver artifact versions.
Comments suppressed due to low confidence (1)

src/main/kotlin/at/bitfire/dav4jvm/DavResource.kt:734

  • [nitpick] Method name isEmptyResponseBody suggests it returns true when the body is empty, but its doc and logic return true when a body is present. Consider renaming this method to hasResponseBody or inverting its return values to match its name and documentation.
    private fun isEmptyResponseBody(response: Response): Boolean {

Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ requested a review from sunkup July 16, 2025 06:25
sunkup
sunkup previously approved these changes Jul 16, 2025
Copy link
Member

@sunkup sunkup left a comment

Choose a reason for hiding this comment

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

Yup. Looks good

Copy link
Member

@rfc2822 rfc2822 left a comment

Choose a reason for hiding this comment

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

Very nice! Some comments:

  1. In assertMultiStatus, we have
val firstBytes = ByteArray(XML_SIGNATURE.size)
body.source().peek().readFully(firstBytes)

There's now response.peekBody(size) and we can directly use its return value to check the XML signature:

response.peekBody(XML_SIGNATURE.size).use {
  //
}
  1. We have some httpResponse.body?. in the code. Body is now non-nullable, so we can remove the ?.

Signed-off-by: Arnau Mora <arnyminerz@proton.me>
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
rfc2822
rfc2822 previously approved these changes Jul 17, 2025
Signed-off-by: Arnau Mora <arnyminerz@proton.me>
@ArnyminerZ ArnyminerZ requested a review from rfc2822 July 17, 2025 08:20
@rfc2822 rfc2822 merged commit fda8229 into main Jul 17, 2025
5 checks passed
@rfc2822 rfc2822 deleted the 73-update-okhttp-to-5x branch July 17, 2025 08:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

refactoring Internal improvement of existing functions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Update okhttp to 5.x

3 participants