Replace retrofit api dependency with implementation #5950

boguszpawlowski · 2025-10-18T17:32:29Z

Summary

Replaced api Retrofit dependency from common module with implementation.
Closes #5421

Checklist

New or updated tests have been added to cover the changes following the testing guidelines.
The code follows the project's code style and best_practices.
The changes have been thoroughly tested, and edge cases have been considered.
Changes are backward compatible whenever feasible. Any breaking changes are documented in the changelog for users and/or in the code for developers depending on the relevance.

Screenshots

Link to pull request in documentation repositories

User Documentation: home-assistant/companion.home-assistant#

Developer Documentation: home-assistant/developers.home-assistant#

Any other notes

home-assistant

Hi @boguszpawlowski

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

boguszpawlowski · 2025-10-18T17:35:01Z

...ain/kotlin/io/homeassistant/companion/android/common/data/HttpExceptionCallAdapterFactory.kt

+import java.lang.reflect.ParameterizedType
+import java.lang.reflect.Type
+
+class HttpExceptionCallAdapterFactory : CallAdapter.Factory() {


Added this adapter to minimize the amount of code changes. IMO we should replace it with an Result style adapter to remove exception-based error handling. Im happy to do it in this /separate PR.

Changing every API to return a Result<> is a big task. I invite you to first create an issue with proper explanations. It needs to be in another PR (maybe multiples one), then adding a lint rule to force the usage of Result.

Currently exception handling in the application is not the best and would need huge changes in general (stopping using catch(e:Exception) that is way too broad for instance)

github-advanced-security

ktlint found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.

Copilot

Pull Request Overview

This PR replaces the api dependency configuration with implementation for Retrofit in the common module, preventing the leakage of Retrofit types into dependent modules' public APIs. This change encapsulates the HTTP client implementation details within the common module.

Key changes:

Created a custom HttpException class to replace Retrofit's HttpException throughout the codebase
Introduced UrlBuilder utility class to encapsulate OkHttp URL building logic
Updated all modules to use the new custom exception type

Reviewed Changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
common/build.gradle.kts	Changed Retrofit dependency from `api` to `implementation`
common/src/main/kotlin/.../HttpException.kt	Added custom `HttpException` data class
common/src/main/kotlin/.../HttpExceptionCallAdapterFactory.kt	Added call adapter to convert Retrofit's `HttpException` to custom type
common/src/main/kotlin/.../UrlBuilder.kt	Added utility class for URL building
common/src/main/kotlin/.../HomeAssistantApis.kt	Registered `HttpExceptionCallAdapterFactory` with Retrofit
onboarding/.../ConnectionViewModel.kt	Replaced OkHttp URL building with `UrlBuilder`
onboarding/.../NameYourDeviceViewModel.kt	Updated to use custom `HttpException`
app/.../LaunchActivity.kt	Updated to use custom `HttpException`
app/.../HaControlsProviderService.kt	Updated to use custom `HttpException` and removed manual error creation
wear/.../EntityStateDataSourceService.kt	Removed unused Retrofit `HttpException` import
*/gradle.lockfile	Updated dependency configurations reflecting the change from `api` to `implementation`

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

TimoPtr

Today our application is not using retrofit2.Call API from retrofit but the coroutine capability of Retrofit, so the factory that you've created won't do anything unfortunately.

Furthermore you are using the onFailure from the Call API. The documentation is clear

Invoked when a network exception occurred talking to the server or when an unexpected exception occurred creating the request or processing the response.

Error like 404 are still handled by onResponse according to the documentation

Invoked for a received HTTP response.
Note: An HTTP response may still indicate an application-level failure such as a 404 or 500. Call Response.isSuccessful() to determine if the response indicates success.

It means that if we merge your PR today the app won't display the right message in LaunchActivity since it is going to be the retrofit HttpException thrown not yours.

TimoPtr · 2025-10-23T08:03:41Z

...ain/kotlin/io/homeassistant/companion/android/common/data/HttpExceptionCallAdapterFactory.kt

@@ -0,0 +1,53 @@
+package io.homeassistant.companion.android.common.data
+
+import io.homeassistant.companion.android.common.exception.HttpException


Suggested change

import io.homeassistant.companion.android.common.exception.HttpException

import io.homeassistant.companion.android.common.data.exception.HttpException

And you should also move MalformedHttpUrlException

TimoPtr · 2025-10-23T08:05:40Z

...ain/kotlin/io/homeassistant/companion/android/common/data/HttpExceptionCallAdapterFactory.kt

+import retrofit2.Response
+import retrofit2.Retrofit
+
+class HttpExceptionCallAdapterFactory : CallAdapter.Factory() {


From https://developers.home-assistant.io/docs/android/best_practices#documentation Future-proof: Ask yourself, "Will I understand what I did in 6 months?"

I think we would benefits from having a documentation on the class definition explaining what is the goal of this factory.

TimoPtr · 2025-10-23T08:15:20Z

...ain/kotlin/io/homeassistant/companion/android/common/data/HttpExceptionCallAdapterFactory.kt

+import java.lang.reflect.ParameterizedType
+import java.lang.reflect.Type
+
+class HttpExceptionCallAdapterFactory : CallAdapter.Factory() {


Changing every API to return a Result<> is a big task. I invite you to first create an issue with proper explanations. It needs to be in another PR (maybe multiples one), then adding a lint rule to force the usage of Result.

Currently exception handling in the application is not the best and would need huge changes in general (stopping using catch(e:Exception) that is way too broad for instance)

TimoPtr · 2025-10-23T08:54:18Z

...ain/kotlin/io/homeassistant/companion/android/common/data/HttpExceptionCallAdapterFactory.kt

+import retrofit2.Response
+import retrofit2.Retrofit
+
+class HttpExceptionCallAdapterFactory : CallAdapter.Factory() {


Suggested change

class HttpExceptionCallAdapterFactory : CallAdapter.Factory() {

internal class HttpExceptionCallAdapterFactory : CallAdapter.Factory() {

This should not be exposed outside of the common module.

TimoPtr · 2025-10-23T09:18:12Z

...ain/kotlin/io/homeassistant/companion/android/common/data/HttpExceptionCallAdapterFactory.kt

+                val convertedException = if (t is retrofit2.HttpException) {
+                    HttpException(
+                        code = t.code(),
+                        message = t.response()?.errorBody()?.string() ?: t.message(),


I would simply use the message from the exception they already handle everything properly.

Suggested change

message = t.response()?.errorBody()?.string() ?: t.message(),

message = t.message,

TimoPtr · 2025-10-23T09:19:08Z

common/src/main/kotlin/io/homeassistant/companion/android/util/UrlBuilder.kt

+import okhttp3.HttpUrl.Companion.toHttpUrl
+
+@Singleton
+class UrlBuilder @Inject constructor() {


I don't think this is relevant to the current PR.

home-assistant · 2025-10-23T09:21:13Z

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

Remove retrofit api dependency

f313227

home-assistant bot requested changes Oct 18, 2025

View reviewed changes

home-assistant bot added cla-needed Hacktoberfest cla-recheck cla-signed and removed cla-recheck cla-needed labels Oct 18, 2025

boguszpawlowski commented Oct 18, 2025

View reviewed changes

boguszpawlowski changed the title ~~[Issue 5421] Remove retrofit api dependency~~ [Issue 5421] Replace retrofit api dependency with implementation Oct 18, 2025

github-advanced-security bot found potential problems Oct 18, 2025

View reviewed changes

boguszpawlowski added 2 commits October 18, 2025 19:58

Run ktlintFormat

2127995

Bring back mock google services json

e5861dc

boguszpawlowski marked this pull request as ready for review October 18, 2025 18:23

Copilot AI review requested due to automatic review settings October 18, 2025 18:23

Copilot AI reviewed Oct 18, 2025

View reviewed changes

boguszpawlowski added 3 commits October 18, 2025 20:44

Fix missed class

0a93c4f

Fix view model tests

e5ac7c0

Run ktlintFormat

7811882

jpelgrom changed the title ~~[Issue 5421] Replace retrofit api dependency with implementation~~ Replace retrofit api dependency with implementation Oct 19, 2025

TimoPtr requested changes Oct 23, 2025

View reviewed changes

home-assistant bot marked this pull request as draft October 23, 2025 09:21

		@@ -0,0 +1,53 @@
		package io.homeassistant.companion.android.common.data

		import io.homeassistant.companion.android.common.exception.HttpException

	class HttpExceptionCallAdapterFactory : CallAdapter.Factory() {
	internal class HttpExceptionCallAdapterFactory : CallAdapter.Factory() {

	message = t.response()?.errorBody()?.string() ?: t.message(),
	message = t.message,

Uh oh!

Replace retrofit api dependency with implementation #5950

Are you sure you want to change the base?

Replace retrofit api dependency with implementation #5950

Conversation

boguszpawlowski commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Screenshots

Link to pull request in documentation repositories

Any other notes

Uh oh!

home-assistant bot left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-advanced-security bot left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

TimoPtr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

home-assistant bot commented Oct 23, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

boguszpawlowski commented Oct 18, 2025 •

edited

Loading