Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

remove okhttp singleton #3047

Merged
merged 5 commits into from
Apr 27, 2022
Merged

remove okhttp singleton #3047

merged 5 commits into from
Apr 27, 2022

Conversation

theScrabi
Copy link
Contributor

@theScrabi theScrabi commented Jan 4, 2021 •
edited
Loading

This was referenced Jan 4, 2021
Merged
Closed
@theScrabi theScrabi linked an issue Jan 4, 2021 that may be closed by this pull request
Remove okHttpClient singleton from library #3048
Closed
6 tasks
@theScrabi theScrabi mentioned this pull request Jan 4, 2021
Closed
8 tasks
@theScrabi theScrabi marked this pull request as ready for review January 18, 2021 10:14
@theScrabi theScrabi force-pushed the fix/okhttp_singleton branch 2 times, most recently from 6e6f172 to 524ba00 Compare February 23, 2021 18:09
@theScrabi theScrabi self-assigned this Feb 25, 2021
@theScrabi theScrabi closed this Feb 25, 2021
@theScrabi theScrabi reopened this Feb 25, 2021
@jesmrec jesmrec mentioned this pull request Mar 3, 2021
Merged
@jesmrec
Copy link
Collaborator

jesmrec commented Mar 5, 2021

Waiting for #3096 before moving forward

@jesmrec jesmrec added the Sprint label Mar 5, 2021
@jesmrec jesmrec requested a review from abelgardep March 9, 2021 15:56
abelgardep
abelgardep suggested changes Apr 26, 2021
View reviewed changes
Copy link
Contributor

@abelgardep abelgardep left a comment

Choose a reason for hiding this comment

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

Just a tiny question @theScrabi

owncloudData/build.gradle Outdated
Comment on lines 40 to 45

testOptions {
unitTests {
includeAndroidResources = true
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that we don't need this, am I missing something?

abelgardep
abelgardep approved these changes Apr 30, 2021
View reviewed changes
Copy link
Contributor

@abelgardep abelgardep left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Approved! Fix the conflicts before moving this forward 👍

abelgardep
abelgardep suggested changes Apr 30, 2021
View reviewed changes
Copy link
Contributor

@abelgardep abelgardep left a comment

Choose a reason for hiding this comment

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

Some tiny suggestions more @theScrabi

build.gradle Outdated
@@ -29,6 +29,7 @@ buildscript {
fragmentTestVersion = "1.1.0"
uiAutomatorTestVersion = "2.2.0"
annotationTestVersion = "1.2.0"
robolectricVersion = "4.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would try not to add this version here. At the moment we use robolectric just in the library, so there is no need to add this version in the app Gradle from my point of view.

.../com/owncloud/android/data/server/datasources/implementation/OCRemoteServerInfoDataSource.kt Outdated
Comment on lines 44 to 51
// Step 1: check whether the root folder exists.
val checkPathExistenceResult =
serverInfoService.checkPathExistence(path, isUserLogged = false, client = owncloudClient)
var checkPathExistenceResult =
serverInfoService.checkPathExistence(path, isUserLogged = false, owncloudClient)
var redirectionLocation = checkPathExistenceResult.redirectedLocation
while (!redirectionLocation.isNullOrEmpty()) {
checkPathExistenceResult =
serverInfoService.checkPathExistence(redirectionLocation, isUserLogged = false, owncloudClient)
redirectionLocation = checkPathExistenceResult.redirectedLocation
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change required? We removed this loop some PR ago. We should use the same client that requested the remote status, so it should follow redirections by itself. That's why we removed it.

Is it needed now after removing the singleton?

.../owncloud/android/data/server/datasources/implementation/OCRemoteServerInfoDataSourceTest.kt Outdated
Comment on lines 72 to 70
@Test
fun getAuthenticationMethodFollowRedirections() {
val checkPathExistenceResultFollowRedirectionMocked: RemoteOperationResult<Boolean> =
createRemoteOperationResultMock(
data = true,
isSuccess = true,
redirectedLocation = OC_SERVER_INFO.baseUrl
)
val checkPathExistenceResultMocked: RemoteOperationResult<Boolean> =
createRemoteOperationResultMock(
data = true,
isSuccess = true,
resultCode = OK_SSL,
authenticationHeader = authHeadersBasic,
httpCode = HTTP_UNAUTHORIZED
)

every {
ocServerInfoService.checkPathExistence(redirectedLocation, false, ocClientMocked)
} returns checkPathExistenceResultFollowRedirectionMocked

every {
ocServerInfoService.checkPathExistence(OC_SERVER_INFO.baseUrl, false, ocClientMocked)
} returns checkPathExistenceResultMocked

val authenticationMethod = ocRemoteServerInfoDatasource.getAuthenticationMethod(redirectedLocation)

assertNotNull(authenticationMethod)
assertEquals(AuthenticationMethod.BASIC_HTTP_AUTH, authenticationMethod)

verify { ocServerInfoService.checkPathExistence(OC_SERVER_INFO.baseUrl, false, ocClientMocked) }
}

Copy link
Contributor

@abelgardep abelgardep Apr 30, 2021 •
edited by theScrabi
Loading

Choose a reason for hiding this comment

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

Related to the previous comment. If previous one is required, then keep this test, if not, we can remove it

@jesmrec jesmrec removed the Sprint label May 12, 2021
@theScrabi theScrabi force-pushed the fix/okhttp_singleton branch 2 times, most recently from 3437682 to 4f9b97b Compare August 2, 2021 08:31
@theScrabi theScrabi force-pushed the fix/okhttp_singleton branch 2 times, most recently from c893f48 to 63f5772 Compare August 5, 2021 07:54
abelgardep
abelgardep approved these changes Aug 19, 2021
View reviewed changes
Copy link
Contributor

@abelgardep abelgardep left a comment

Choose a reason for hiding this comment

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

LGTM

@sonarcloud
Copy link

sonarcloud bot commented Aug 19, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@sonarcloud
Copy link

sonarcloud bot commented Nov 12, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@theScrabi theScrabi force-pushed the fix/okhttp_singleton branch 2 times, most recently from 658e26b to a0db300 Compare March 24, 2022 16:46
@theScrabi theScrabi force-pushed the fix/okhttp_singleton branch 3 times, most recently from 0ebd3a4 to 364ba2e Compare April 13, 2022 13:23
abelgardep
abelgardep suggested changes Apr 18, 2022
View reviewed changes
Copy link
Contributor

@abelgardep abelgardep left a comment

Choose a reason for hiding this comment

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

@theScrabi Tiny suggestions here

.../com/owncloud/android/data/server/datasources/implementation/OCRemoteServerInfoDataSource.kt Outdated Show resolved Hide resolved
abelgardep
abelgardep approved these changes Apr 25, 2022
View reviewed changes
Copy link
Contributor

@abelgardep abelgardep left a comment

Choose a reason for hiding this comment

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

Approved. Let's QA this @jesmrec

@jesmrec
Copy link
Collaborator

jesmrec commented Apr 27, 2022

This is working from my side. Since this change is transversal (affects everything that need some network operation), the way to do was checking same things as the network library switch.

From my side this is ready to go. Great job, @theScrabi!

Beta version should help to find any other corner case.

@jesmrec jesmrec added this to the 2.21-current milestone Apr 27, 2022
@abelgardep abelgardep merged commit c98ad69 into master Apr 27, 2022
@abelgardep abelgardep deleted the fix/okhttp_singleton branch April 27, 2022 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abelgardep abelgardep abelgardep approved these changes

Assignees

@theScrabi theScrabi

Labels
Approved by qa
Projects
None yet
Milestone
2.21-current
Development

Successfully merging this pull request may close these issues.

Remove okHttpClient singleton from library
3 participants
@theScrabi @jesmrec @abelgardep