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

feat(nextcloud): Add low-level helpers for WebDavResponse #2561

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

provokateurin
Copy link
Member

Necessary refactor for #718.
Extension helpers are still there to offload the small part of logic that is useful for consumers (while the rest was just proxy variables).

Copy link

codecov bot commented Oct 9, 2024

Codecov Report

Attention: Patch coverage is 0% with 9 lines in your changes missing coverage. Please review.

Project coverage is 28.71%. Comparing base (548176b) to head (efc8823).

Files with missing lines Patch % Lines
.../src/api/webdav/utils/webdav_response_helpers.dart 0.00% 9 Missing ⚠️
Additional details and impacted files
@@                         Coverage Diff                         @@
##           test/nextcloud/all-props-parsed    #2561      +/-   ##
===================================================================
- Coverage                            28.71%   28.71%   -0.01%     
===================================================================
  Files                                  366      366              
  Lines                               136274   136280       +6     
===================================================================
  Hits                                 39128    39128              
- Misses                               97146    97152       +6     
Flag Coverage Δ *Carryforward flag
account_repository 98.76% <ø> (ø)
cookie_store 99.48% <ø> (ø) Carriedforward from 548176b
dashboard_app 96.05% <ø> (ø)
dynamite 31.05% <ø> (ø) Carriedforward from 548176b
dynamite_end_to_end_test 61.69% <ø> (ø) Carriedforward from 548176b
dynamite_runtime 85.40% <ø> (ø) Carriedforward from 548176b
interceptor_http_client 97.18% <ø> (ø) Carriedforward from 548176b
neon_dashboard 96.05% <ø> (ø) Carriedforward from 548176b
neon_framework 59.20% <ø> (ø)
neon_http_client 97.50% <ø> (ø)
neon_notifications 100.00% <ø> (ø) Carriedforward from 548176b
neon_storage 94.66% <ø> (ø)
neon_talk 99.45% <ø> (ø) Carriedforward from 548176b
nextcloud 24.31% <0.00%> (-0.01%) ⬇️
notifications_app 97.40% <ø> (ø)
notifications_push_repository 98.59% <ø> (ø)
sort_box 90.90% <ø> (ø) Carriedforward from 548176b
talk_app 98.94% <ø> (ø)

*This pull request uses carry forward flags. Click here to find out more.

Files with missing lines Coverage Δ
.../src/api/webdav/utils/webdav_response_helpers.dart 0.00% <0.00%> (ø)

@Leptopoda
Copy link
Member

I was also thinking a lot about removing the webdav file abstraction and rather have something based on package:file in a separate repo.

Is there a way we can do this without breaking the api?
We should deprecate the webdav file abstractions and give devs time to migrate (given that this is the most used code of the nextcloud package).

@provokateurin
Copy link
Member Author

I was also thinking a lot about removing the webdav file abstraction and rather have something based on package:file in a separate repo.

Sounds good, but the nextcloud package wouldn't know about this abstraction, right?

Is there a way we can do this without breaking the api?
We should deprecate the webdav file abstractions and give devs time to migrate (given that this is the most used code of the nextcloud package).

I need to get rid of this for CalDAV/CardDAV support, so if we deprecate it now we will have to wait a lot longer until we can support those as well.
While deprecating it for now and removing it later would be nice, I think it is fine since there is an easy path to migrate.

@Leptopoda
Copy link
Member

Sounds good, but the nextcloud package wouldn't know about this abstraction, right?

For the idea I head yes.

I need to get rid of this for CalDAV/CardDAV support, so if we deprecate it now we will have to wait a lot longer until we can support those as well.

Then just duplicate the code and implement the new low level api separately.
You can also keep the low level api internally for now and just base CalDav and CardDav on it, deprecating the WebdavFile at a later point.

@provokateurin provokateurin force-pushed the refactor/nextcloud/remove-webdav-file branch from efc8823 to 81ab415 Compare October 20, 2024 13:51
@provokateurin provokateurin changed the base branch from test/nextcloud/all-props-parsed to main October 20, 2024 13:51
@provokateurin provokateurin changed the title refactor(nextcloud)!: Remove WebDavFile in favor of low-level WebDavResponse feat(nextcloud): Add low-level helpers for WebDavResponse Oct 20, 2024
@provokateurin provokateurin marked this pull request as ready for review October 20, 2024 13:51
Copy link
Member

@Leptopoda Leptopoda left a comment

Choose a reason for hiding this comment

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

Should we even keep the PathUri abstraction or just make it work on strings?

/// Helpers for WebDavResponse
extension WebDavResponseHelpers on WebDavResponse {
/// All available props.
WebDavProp get props => propstats.singleWhere((propstat) => propstat.status.contains('200 OK')).prop;
Copy link
Member

Choose a reason for hiding this comment

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

What is the typical prop count on a file? How expensive is this operation?

The second commit calls file.props multiple times subsequently.
I don't have any issue in keeping it like this and require the user to cache the prop themselves.

Copy link
Member Author

Choose a reason for hiding this comment

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

There are usually two propstats (one 200, one 404). This could also be changed to a firstWhere to be slightly cheaper.

import 'package:nextcloud/src/api/webdav/models/models.dart';

/// Helpers for WebDavResponse
extension WebDavResponseHelpers on WebDavResponse {
Copy link
Member

Choose a reason for hiding this comment

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

Why don't we just make these helpers available on the WebDavResponse directly?
I understand that this will make the generation a bit harder, but it also allows us to cache these operations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, I can give it a try. Caching is indeed a good reason to move it inline.


/// The path of the resource.
PathUri get path {
final href = PathUri.parse(Uri.decodeFull(this.href!));
Copy link
Member

Choose a reason for hiding this comment

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

Why the null assertion?

Copy link
Member Author

Choose a reason for hiding this comment

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

There can be responses without a href which are not valid. Consumers need to filter these out (we already did in toWebDavFiles()) and then we need to prevent null hrefs here.

Copy link
Member

Choose a reason for hiding this comment

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

But this means that unfiltered responses will throw here.
I'd rather return a null when there is no href.

Then the filtering can be done based on the path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants