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

[full-ci] change default sharing driver to cs3 #3697

Merged
merged 2 commits into from
Jun 20, 2022

Conversation

butonic
Copy link
Member

@butonic butonic commented May 5, 2022

we now use the cs3 drivers to persist user shares and public links

@update-docs
Copy link

update-docs bot commented May 5, 2022

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@ownclouders
Copy link
Contributor

ownclouders commented May 5, 2022

💥 Acceptance test Core-API-Tests-ocis-storage-6 failed. Further test are cancelled...

@butonic butonic force-pushed the change-default-sharing-driver-to-cs3 branch from 18cdf24 to afb7829 Compare May 5, 2022 13:13
wkloucek
wkloucek previously approved these changes May 5, 2022
@butonic
Copy link
Member Author

butonic commented May 5, 2022

the problem is that when sharing a folder with a user and a group the POST response when accepting the share returns two responses:

{
    "ocs": {
        "meta": {
            "status": "ok",
            "statuscode": 200,
            "message": "OK"
        },
        "data": [
            {
                "id": "bb43a564-a813-4610-baeb-a63d3af3ec53",
                "share_type": 0,
                "uid_owner": "richard",
                "displayname_owner": "Richard Phillips Feynman",
                "additional_info_owner": "[email protected]",
                "permissions": 1,
                "stime": 1651762956,
                "parent": "",
                "expiration": "",
                "token": "",
                "uid_file_owner": "richard",
                "displayname_file_owner": "Richard Phillips Feynman",
                "additional_info_file_owner": "[email protected]",
                "state": 0,
                "path": "/Shares/r1",
                "item_type": "folder",
                "mimetype": "httpd/unix-directory",
                "storage_id": "shared::/Shares/r1",
                "storage": 0,
                "item_source": "1284d238-aa92-42ce-bdc4-0b0000009157$932b4540-8d16-481e-8ef4-588e4b6b151c!00c43841-640b-485d-aea0-22a6d95e49a1",
                "file_source": "1284d238-aa92-42ce-bdc4-0b0000009157$932b4540-8d16-481e-8ef4-588e4b6b151c!00c43841-640b-485d-aea0-22a6d95e49a1",
                "file_parent": "",
                "file_target": "/Shares/r1",
                "share_with": "einstein",
                "share_with_user_type": 0,
                "share_with_displayname": "Albert Einstein",
                "share_with_additional_info": "[email protected]",
                "mail_send": 0,
                "name": ""
            }
        ]
    }
}{
    "ocs": {
        "meta": {
            "status": "ok",
            "statuscode": 200,
            "message": "OK"
        },
        "data": [
            {
                "id": "84632304-f56b-48f1-ab50-cddd65ccd013",
                "share_type": 1,
                "uid_owner": "richard",
                "displayname_owner": "Richard Phillips Feynman",
                "additional_info_owner": "[email protected]",
                "permissions": 1,
                "stime": 1651762966,
                "parent": "",
                "expiration": "",
                "token": "",
                "uid_file_owner": "richard",
                "displayname_file_owner": "Richard Phillips Feynman",
                "additional_info_file_owner": "[email protected]",
                "state": 0,
                "path": "/Shares/r1",
                "item_type": "folder",
                "mimetype": "httpd/unix-directory",
                "storage_id": "shared::/Shares/r1",
                "storage": 0,
                "item_source": "1284d238-aa92-42ce-bdc4-0b0000009157$932b4540-8d16-481e-8ef4-588e4b6b151c!00c43841-640b-485d-aea0-22a6d95e49a1",
                "file_source": "1284d238-aa92-42ce-bdc4-0b0000009157$932b4540-8d16-481e-8ef4-588e4b6b151c!00c43841-640b-485d-aea0-22a6d95e49a1",
                "file_parent": "",
                "file_target": "/Shares/r1",
                "share_with": "physics-lovers",
                "share_with_user_type": 0,
                "share_with_displayname": "physics-lovers",
                "share_with_additional_info": "",
                "mail_send": 0,
                "name": ""
            }
        ]
    }
}

the response object is duplicated 🤔

maybe because the cs3 api returns more than one result? needs more debugging

@butonic
Copy link
Member Author

butonic commented May 5, 2022

the JSON driver explicitly hides one of the duplicate shares:

			// When we arrive here there was already a share for this resource.
			// if there is a mix-up of shares of type group and shares of type user we need to deduplicate them, since it points
			// to the same resource. Leave the more explicit and hide the less explicit. In this case we hide the group shares
			// and return the user share to the user.

IMO this might not be the best behavior ... the web ui should decide how to deduplicate.

In any case this does not explain why the code renders the object twice ...

@butonic
Copy link
Member Author

butonic commented May 5, 2022

ok we iterate over all shares

	for id := range sharesToAccept {
		h.updateReceivedShare(w, r, id, false, mount)
	}

and then updateReceivedShare produces a full response for every share, including meta etc:

	response.WriteOCSSuccess(w, r, []*conversions.ShareData{data})

@butonic
Copy link
Member Author

butonic commented May 6, 2022

shares listing fixed with cs3org/reva#2830

@butonic butonic force-pushed the change-default-sharing-driver-to-cs3 branch from 716f783 to 8013e8d Compare May 9, 2022 15:01
@butonic
Copy link
Member Author

butonic commented May 10, 2022

depending on the timing one of two scenarios are the cause for the failing testsuite:

  1. Core-API-Tests-ocis-storage-4, apiShareOperationsToShares1/gettingSharesPendingFiltered.feature:44
  Scenario Outline: getting shares received from users                            # /srv/app/testrunner/tests/acceptance/features/apiShareOperationsToShares1/gettingSharesReceivedFiltered.feature:37
    Given using OCS API version "<ocs_api_version>"                               # FeatureContext::usingOcsApiVersion()
    When user "Brian" gets the user shares shared with him using the sharing API  # FeatureContext::userGetsFilteredSharesSharedWithHimUsingTheSharingApi()
    Then the OCS status code should be "<ocs_status_code>"                        # OCSContext::theOCSStatusCodeShouldBe()
    And the HTTP status code should be "200"                                      # FeatureContext::thenTheHTTPStatusCodeShouldBe()
    And exactly 2 files or folders should be included in the response             # FeatureContext::checkCountFilesFoldersInResponse()
    And folder "/Shares/folderToShareWithUser" should be included in the response # FeatureContext::checkSharedFileInResponse()
    And file "/Shares/fileToShareWithUser.txt" should be included in the response # FeatureContext::checkSharedFileInResponse()

    Examples:
      | ocs_api_version | ocs_status_code |
      | 1               | 100             |
        Sharing::checkCountFilesFoldersInResponse the response does not contain 2 entries
        Failed asserting that actual size 4 matches expected size 2.
      | 2               | 200             |
        Sharing::checkCountFilesFoldersInResponse the response does not contain 2 entries
        Failed asserting that actual size 4 matches expected size 2.
  1. Core-API-Tests-ocis-storage-6, apiShareUpdateToShares/updateShare.feature:113 ff
Scenario Outline: Cannot set permissions to zero                      # /srv/app/testrunner/tests/acceptance/features/apiShareUpdateToShares/updateShare.feature:102
    Given using OCS API version "<ocs_api_version>"                     # FeatureContext::usingOcsApiVersion()
    And group "grp1" has been created                                   # FeatureContext::groupHasBeenCreated()
    And user "Alice" has created folder "/FOLDER"                       # FeatureContext::userHasCreatedFolder()
    And user "Alice" has shared folder "/FOLDER" with group "grp1"      # FeatureContext::userHasSharedFileWithGroupUsingTheSharingApi()
    When user "Alice" updates the last share using the sharing API with # FeatureContext::userUpdatesTheLastShareWith()
      | permissions | 0 |
    Then the OCS status code should be "400"                            # OCSContext::theOCSStatusCodeShouldBe()
    And the HTTP status code should be "<http_status_code>"             # FeatureContext::thenTheHTTPStatusCodeShouldBe()

    Examples:
      | ocs_api_version | http_status_code |
      | 1               | 200              |
        OCS status code is not any of the expected values 400 got 100
        Failed asserting that an array contains '100'.
      | 2               | 400              |
        OCS status code is not any of the expected values 400 got 200
        Failed asserting that an array contains '200'.

@micbar micbar mentioned this pull request May 10, 2022
45 tasks
@butonic
Copy link
Member Author

butonic commented May 10, 2022

needs cs3org/reva#2853

@butonic butonic self-assigned this May 11, 2022
@butonic butonic force-pushed the change-default-sharing-driver-to-cs3 branch from 8013e8d to 60207ff Compare May 11, 2022 14:37
@micbar micbar force-pushed the change-default-sharing-driver-to-cs3 branch from 60207ff to 07e6176 Compare May 11, 2022 19:29
@micbar
Copy link
Contributor

micbar commented May 11, 2022

The expected failures are back to normal.

Let us see if the share manager is stable in the CI

@micbar micbar changed the title change default sharing driver to cs3 [full-ci] change default sharing driver to cs3 May 11, 2022
@micbar micbar force-pushed the change-default-sharing-driver-to-cs3 branch from 07e6176 to 371c6db Compare May 11, 2022 19:31
@micbar
Copy link
Contributor

micbar commented May 12, 2022

Test is still flaky https://drone.owncloud.com/owncloud/ocis/11751/44/6
One time it passes, next time it fails.

@micbar
Copy link
Contributor

micbar commented May 16, 2022

@butonic Please bring this to a mergable state.

@aduffeck JFYI

@aduffeck aduffeck force-pushed the change-default-sharing-driver-to-cs3 branch 2 times, most recently from ec92d50 to 874d075 Compare May 19, 2022 09:23
@butonic butonic assigned aduffeck and unassigned butonic May 19, 2022
@micbar
Copy link
Contributor

micbar commented May 20, 2022

Still the same scenario

apiShareReshareToShares3/reShareUpdate.feature:152

Seems like it still passes "sometimes"

@micbar
Copy link
Contributor

micbar commented May 30, 2022

@phil-davis @aduffeck Any progress on the testsuite?

@phil-davis
Copy link
Contributor

@phil-davis @aduffeck Any progress on the testsuite?

owncloud/core#40095 is coming... Ask me again tomorrow!

@aduffeck
Copy link
Contributor

aduffeck commented Jun 2, 2022

@phil-davis please let me know if there's anything I can help you with

@phil-davis
Copy link
Contributor

@phil-davis please let me know if there's anything I can help you with

PR #3908 has been merged. That gets the new API test code running in oCIS CI.

Rebase this PR and see what happens. @aduffeck @butonic

Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@aduffeck aduffeck force-pushed the change-default-sharing-driver-to-cs3 branch from 874d075 to 7667506 Compare June 2, 2022 14:00
Signed-off-by: Jörn Friedrich Dreyer <[email protected]>
@aduffeck aduffeck force-pushed the change-default-sharing-driver-to-cs3 branch from 7667506 to 379a7fd Compare June 2, 2022 14:45
@sonarcloud
Copy link

sonarcloud bot commented Jun 2, 2022

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 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@butonic
Copy link
Member Author

butonic commented Jun 14, 2022

What is the status of this? Do we need a migration? Or should we merge as is? Maybe add a Change changelog to document that shares will vanish unless they are migrated or the driver is set back to json?

@wkloucek wkloucek self-requested a review June 14, 2022 08:28
@wkloucek wkloucek dismissed their stale review June 14, 2022 08:28

review was before beta phase

@butonic butonic merged commit 09b89f7 into master Jun 20, 2022
@delete-merged-branch delete-merged-branch bot deleted the change-default-sharing-driver-to-cs3 branch June 20, 2022 07:55
ownclouders pushed a commit that referenced this pull request Jun 20, 2022
Merge: 2e913fa 379a7fd
Author: Jörn Friedrich Dreyer <[email protected]>
Date:   Mon Jun 20 07:55:12 2022 +0000

    Merge pull request #3697 from owncloud/change-default-sharing-driver-to-cs3

    [full-ci] change default sharing driver to cs3
@aduffeck aduffeck mentioned this pull request Jun 20, 2022
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants