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.

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

Switching to dav4jvm #881

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Switching to dav4jvm #881

wants to merge 3 commits into from

Conversation

tobiasKaminsky
Copy link
Member

Heavily under construction 🚧
If you have any feedback, please comment :)

Signed-off-by: tobiasKaminsky tobias@kaminsky.me

@tobiasKaminsky
Copy link
Member Author

@AlvaroBrey if you have a bit of time, any feedback would be great 💙

Copy link
Member

@AlvaroBrey AlvaroBrey left a comment

Choose a reason for hiding this comment

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

  • Unused submodule?

@@ -253,4 +318,19 @@ public static String getEtagFromResponse(HttpMethod method) {
return result;
}

public static void registerCustomFactories() {
Copy link
Member

Choose a reason for hiding this comment

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

As we're working on a static singleton (PropertyRegistry.INSTANCE), I guess it would be better to initialize this list in a static block in order to avoid having to call it explicitly on clients.

Comment on lines 203 to 207
OkHttpClient disabledRedirectClient = client.getClient()
.newBuilder()
.followRedirects(false)
.authenticator(new NextcloudAuthenticator(client.getCredentials(), "Authorization"))
.build();
Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's a good idea to create a new OkHttpClient for every request... the NextcloudClient passed to run should already be authenticated IMO. Not sure about the redirects or why they are disabled.

Comment on lines +241 to +212
if (searchMethod != null) {
searchMethod.releaseConnection(); // let the connection available for other methods
}
Copy link
Member

Choose a reason for hiding this comment

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

This does not seem necessary, as the search method is never called (only used to get the query string)

@nextcloud nextcloud deleted a comment from github-actions bot Aug 20, 2024
@nextcloud nextcloud deleted a comment from github-actions bot Aug 20, 2024
@nextcloud nextcloud deleted a comment from github-actions bot Aug 20, 2024
@nextcloud nextcloud deleted a comment from github-actions bot Aug 20, 2024
@nextcloud nextcloud deleted a comment from github-actions bot Aug 20, 2024
@nextcloud nextcloud deleted a comment from github-actions bot Aug 20, 2024
@nextcloud nextcloud deleted a comment from github-actions bot Aug 20, 2024
@nextcloud nextcloud deleted a comment from github-actions bot Aug 20, 2024
@nextcloud nextcloud deleted a comment from github-actions bot Aug 20, 2024
@nextcloud nextcloud deleted a comment from github-actions bot Aug 20, 2024
@nextcloud nextcloud deleted a comment from github-actions bot Aug 20, 2024
@nextcloud nextcloud deleted a comment from github-actions bot Aug 20, 2024
@nextcloud nextcloud deleted a comment from github-actions bot Aug 20, 2024
@nextcloud nextcloud deleted a comment from github-actions bot Aug 20, 2024
Copy link
Contributor

SpotBugs

CategoryBaseNew
Bad practice3535
Correctness3431
Dodgy code2627
Internationalization64
Malicious code vulnerability4944
Multithreaded correctness33
Performance88
Security02
Total161154

Copy link

codecov bot commented Aug 20, 2024

Codecov Report

Attention: Patch coverage is 66.07362% with 553 lines in your changes missing coverage. Please review.

Project coverage is 51.19%. Comparing base (bf33890) to head (4f7f56d).

Files Patch % Lines
...oid/lib/common/operations/RemoteOperationResult.kt 52.03% 94 Missing and 59 partials ⚠️
...ud/android/lib/resources/files/model/RemoteFile.kt 25.71% 23 Missing and 3 partials ⚠️
...owncloud/android/lib/common/network/WebdavEntry.kt 62.50% 20 Missing and 4 partials ⚠️
...ncloud/android/lib/common/utils/WebDavFileUtils.kt 72.22% 9 Missing and 11 partials ⚠️
...c/main/java/com/nextcloud/operations/MoveMethod.kt 0.00% 17 Missing ⚠️
...ud/android/lib/resources/files/webdav/NCSharees.kt 70.21% 2 Missing and 12 partials ⚠️
...owncloud/android/lib/common/network/WebdavUtils.kt 92.44% 6 Missing and 7 partials ⚠️
...cloud/android/lib/resources/files/webdav/NCTags.kt 70.00% 7 Missing and 5 partials ⚠️
...b/resources/files/webdav/NCTrashbinDeletionTime.kt 23.07% 10 Missing ⚠️
...d/lib/resources/files/webdav/NCTrashbinFilename.kt 23.07% 10 Missing ⚠️
... and 59 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #881      +/-   ##
============================================
+ Coverage     49.16%   51.19%   +2.03%     
- Complexity     1000     1147     +147     
============================================
  Files           208      256      +48     
  Lines          7819     8372     +553     
  Branches       1017     1107      +90     
============================================
+ Hits           3844     4286     +442     
- Misses         3421     3464      +43     
- Partials        554      622      +68     
Files Coverage Δ
...ces/groupfolders/GetGroupfoldersRemoteOperation.kt 61.29% <100.00%> (ø)
...b/resources/profile/GetHoverCardRemoteOperation.kt 57.14% <100.00%> (ø)
...ry/src/main/java/com/nextcloud/common/DavMethod.kt 100.00% <100.00%> (ø)
.../main/java/com/nextcloud/common/NextcloudClient.kt 50.45% <100.00%> (+3.92%) ⬆️
...main/java/com/nextcloud/common/OkHttpMethodBase.kt 64.81% <ø> (-1.26%) ⬇️
.../java/com/nextcloud/common/UserAgentInterceptor.kt 100.00% <100.00%> (ø)
...n/java/com/nextcloud/extensions/ArrayExtensions.kt 100.00% <100.00%> (ø)
...in/java/com/nextcloud/operations/PropFindResult.kt 100.00% <100.00%> (ø)
...rc/main/java/com/nextcloud/operations/PutMethod.kt 83.33% <100.00%> (+11.90%) ⬆️
...d/android/lib/common/network/ExtendedProperties.kt 100.00% <100.00%> (ø)
... and 83 more

... and 5 files with indirect coverage changes

Copy link
Contributor

SpotBugs

CategoryBaseNew
Bad practice3535
Correctness3431
Dodgy code2627
Internationalization64
Malicious code vulnerability4944
Multithreaded correctness33
Performance88
Security02
Total161154

Copy link
Contributor

SpotBugs

CategoryBaseNew
Bad practice3535
Correctness3431
Dodgy code2627
Internationalization64
Malicious code vulnerability4944
Multithreaded correctness33
Performance88
Security02
Total161154

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
Copy link
Contributor

SpotBugs

CategoryBaseNew
Bad practice3535
Correctness3431
Dodgy code2627
Internationalization64
Malicious code vulnerability4944
Multithreaded correctness33
Performance88
Security02
Total161154

Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
Signed-off-by: tobiasKaminsky <tobias@kaminsky.me>
Copy link
Contributor

SpotBugs

CategoryBaseNew
Bad practice3535
Correctness3431
Dodgy code2627
Internationalization64
Malicious code vulnerability4944
Multithreaded correctness33
Performance88
Security02
Total161154

@nextcloud-android-bot
Copy link
Collaborator

@nextcloud-android-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 📄 To do (max 2 entries / member)
Development

Successfully merging this pull request may close these issues.

5 participants