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

Allow getting the share list filtered by share type via API #38000

Merged
merged 7 commits into from
Oct 28, 2020

Conversation

jvillafanez
Copy link
Member

Description

Allow filtering by share type while getting the shares. This should speed up a bit the web UI because the server will need to fetch and send less data.
Changes are backwards-compatible: if no filter is sent, all the shares will be returned, same as before.

Related Issue

https://github.com/owncloud/enterprise/issues/4107

Motivation and Context

The "share by link" view is expected to perform faster since there are less requests done to the DB to fetch data. Furthermore, the view doesn't need to ask for remote shares, which could slow down things if there are problems with the remote server.

How Has This Been Tested?

  • test environment:
  • test case 1:
  • test case 2:
  • ...

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@jvillafanez
Copy link
Member Author

jvillafanez commented Oct 9, 2020

Missing items:

  • Add new unit tests for the new cases
  • Add changelog entry

Code has been checked via web UI and partially via API

@phil-davis
Copy link
Contributor

Could also add a couple of API acceptance tests. There is a step:
When user "Alice" gets all shares shared by him using the sharing API

That could have more specific steps to get just the user, group or public link shares.

The scenario(s) could create a few shares of different types, then do a "get" filtering by share type.

@micbar
Copy link
Contributor

micbar commented Oct 19, 2020

@jvillafanez Let us include this in 10.6

@phil-davis
Copy link
Contributor

@jvillafanez there are some JavaScript tests that failed: https://drone.owncloud.com/owncloud/core/27205/8/6
Are those real fails that need to be fixed?

@jvillafanez
Copy link
Member Author

Failing test fixed, and new unit tests added. This should be ready now.

@micbar micbar requested a review from VicDeo October 19, 2020 14:06
@phil-davis phil-davis force-pushed the filter_by_sharetype branch 2 times, most recently from f748fcd to 6d873a9 Compare October 20, 2020 03:35
@phil-davis
Copy link
Contributor

phil-davis commented Oct 20, 2020

  1. I added some API acceptance tests for filtering the list of shares that are "shared by me" - those pass.

  2. filtering for "shared with me" does not seem to work:
    As user admin, I shared a folder f1 with user phil and folder f2 with group grp1. phil id a member of grp1

When I query as phil to get the group shares that are shared with me, I get both shares returned:

curl -u phil:phil http://172.17.0.1:8080/ocs/v2.php/apps/files_sharing/api/v1/shares?share_types=1\&shared_with_me=true
<?xml version="1.0"?>
<ocs>
 <meta>
  <status>ok</status>
  <statuscode>200</statuscode>
  <message/>
  <totalitems></totalitems>
  <itemsperpage></itemsperpage>
 </meta>
 <data>
  <element>
   <id>344</id>
   <share_type>0</share_type>
   <uid_owner>admin</uid_owner>
   <displayname_owner>I am admin</displayname_owner>
   <permissions>31</permissions>
   <stime>1603167789</stime>
   <parent/>
   <expiration/>
   <token/>
   <uid_file_owner>admin</uid_file_owner>
   <displayname_file_owner>I am admin</displayname_file_owner>
   <additional_info_owner/>
   <additional_info_file_owner/>
   <state>0</state>
   <path>/f1</path>
   <item_type>folder</item_type>
   <mimetype>httpd/unix-directory</mimetype>
   <storage_id>shared::/f1</storage_id>
   <storage>739</storage>
   <item_source>2147525538</item_source>
   <file_source>2147525538</file_source>
   <file_parent>2147525335</file_parent>
   <file_target>/f1</file_target>
   <share_with>phil</share_with>
   <share_with_displayname>Phil Ji</share_with_displayname>
   <share_with_additional_info/>
   <mail_send>0</mail_send>
   <attributes/>
  </element>
  <element>
   <id>499</id>
   <share_type>1</share_type>
   <uid_owner>admin</uid_owner>
   <displayname_owner>I am admin</displayname_owner>
   <permissions>31</permissions>
   <stime>1603175000</stime>
   <parent/>
   <expiration/>
   <token/>
   <uid_file_owner>admin</uid_file_owner>
   <displayname_file_owner>I am admin</displayname_file_owner>
   <additional_info_owner/>
   <additional_info_file_owner/>
   <state>0</state>
   <path>/f2</path>
   <item_type>folder</item_type>
   <mimetype>httpd/unix-directory</mimetype>
   <storage_id>shared::/f2</storage_id>
   <storage>739</storage>
   <item_source>2147525539</item_source>
   <file_source>2147525539</file_source>
   <file_parent>2147525335</file_parent>
   <file_target>/f2</file_target>
   <share_with>z111</share_with>
   <share_with_displayname>z111</share_with_displayname>
   <mail_send>0</mail_send>
   <attributes/>
  </element>
 </data>
</ocs>

The filtering did not happen.

But when I query as admin for the group shares that have been shared by me, I correctly get just the group share:

curl -u admin:admin http://172.17.0.1:8080/ocs/v2.php/apps/files_sharing/api/v1/shares?share_types=1
<?xml version="1.0"?>
<ocs>
 <meta>
  <status>ok</status>
  <statuscode>200</statuscode>
  <message/>
  <totalitems></totalitems>
  <itemsperpage></itemsperpage>
 </meta>
 <data>
  <element>
   <id>499</id>
   <share_type>1</share_type>
   <uid_owner>admin</uid_owner>
   <displayname_owner>I am admin</displayname_owner>
   <permissions>31</permissions>
   <stime>1603175000</stime>
   <parent/>
   <expiration/>
   <token/>
   <uid_file_owner>admin</uid_file_owner>
   <displayname_file_owner>I am admin</displayname_file_owner>
   <additional_info_owner/>
   <additional_info_file_owner/>
   <path>/f2</path>
   <item_type>folder</item_type>
   <mimetype>httpd/unix-directory</mimetype>
   <storage_id>home::admin</storage_id>
   <storage>739</storage>
   <item_source>2147525539</item_source>
   <file_source>2147525539</file_source>
   <file_parent>2147525254</file_parent>
   <file_target>/f2</file_target>
   <share_with>z111</share_with>
   <share_with_displayname>z111</share_with_displayname>
   <mail_send>0</mail_send>
   <attributes/>
  </element>
 </data>
</ocs>

Is this filtering-by-share-type supposed to work for shared_with_me ?

@phil-davis phil-davis force-pushed the filter_by_sharetype branch 2 times, most recently from b3a51c4 to c7fc31c Compare October 20, 2020 10:56
@phil-davis
Copy link
Contributor

phil-davis commented Oct 20, 2020

ToDo: add some API acceptance tests for filtering the shared_with_me list.

  • done

@phil-davis
Copy link
Contributor

How will clients know that the server is capable of providing a filtered list of shares by share type?
Should this capability be advertised in the capabilities endpoint?

@jvillafanez
Copy link
Member Author

I'd vote for API docs.
For the clients, I think the easiest options are:

  • Ignore this feature and keep requesting all the shares. This means they won't need to change anything on their end.
  • Request specific share types but keep filtering locally. Local filtering will still be required because the servers prior 10.6 will ignore the share type filtering and return all the share types

I think those options are the ones that require the least changes both in the client and the server.

@jvillafanez
Copy link
Member Author

Doc ticket opened in owncloud/docs#2825

@owncloud owncloud deleted a comment from update-docs bot Oct 21, 2020
Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

API acceptance tests are done for the interesting combinations - looks good.

@micbar micbar requested a review from karakayasemi October 26, 2020 09:24
Copy link
Contributor

@karakayasemi karakayasemi left a comment

Choose a reason for hiding this comment

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

Generally, code looks okay. All the changes are in controller and all tests are passed. So, affected parts should be limited.
I just only suggested a small change that can simplify some foreach blocks. @jvillafanez I would like to hear your thoughts about that.

Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

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

Syntax error needs fixing - @jvillafanez

apps/files_sharing/lib/Controller/Share20OcsController.php Outdated Show resolved Hide resolved
Copy link
Contributor

@karakayasemi karakayasemi left a comment

Choose a reason for hiding this comment

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

LGTM!

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

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

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

Successfully merging this pull request may close these issues.

4 participants