Skip to content

Conversation

@MorrisJobke
Copy link
Member

@rullzer Please have a good review of this :)

And first let's see what the CI thinks about it.

@MorrisJobke MorrisJobke added 3. to review Waiting for reviews downstream labels Oct 24, 2016
@MorrisJobke MorrisJobke added this to the Nextcloud 11.0 milestone Oct 24, 2016
@mention-bot
Copy link

@MorrisJobke, thanks for your PR! By analyzing the history of the files in this pull request, we identified @PVince81, @SergioBertolinSG and @rullzer to be potential reviewers.

@MorrisJobke MorrisJobke changed the title Downstream 26235 Integration tests refactoring Oct 24, 2016
@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 26, 2016
Adapted sharing and external features, random failures in sharing:857

Added weddav related feature

Removed forgotten leftovers
Signed-off-by: Morris Jobke <[email protected]>
@MorrisJobke MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 2, 2016
Signed-off-by: Roeland Jago Douma <[email protected]>
@rullzer
Copy link
Member

rullzer commented Nov 4, 2016

Mmmm I have a feeling that failing test might be a timing issue...

@rullzer
Copy link
Member

rullzer commented Nov 4, 2016

Ok indeed. Race condition. I can explain.
It is about

Scenario: Merging shares for recipient when shared from outside with group then user and recipient renames in between
Given As an "admin"
And user "user0" exists
And user "user1" exists
And group "group1" exists
And user "user1" belongs to group "group1"
And user "user0" created a folder "/merge-test-outside-groups-renamebeforesecondshare"
When folder "/merge-test-outside-groups-renamebeforesecondshare" of user "user0" is shared with group "group1"
And User "user1" moved folder "/merge-test-outside-groups-renamebeforesecondshare" to "/merge-test-outside-groups-renamebeforesecondshare-renamed"
And folder "/merge-test-outside-groups-renamebeforesecondshare" of user "user0" is shared with user "user1"
Then as "user1" gets properties of folder "/merge-test-outside-groups-renamebeforesecondshare-renamed" with
|{http://owncloud.org/ns}permissions|
And the single response should contain a property "{http://owncloud.org/ns}permissions" with value "SRDNVCK"
And as "user1" the folder "/merge-test-outside-groups-renamebeforesecondshare" does not exist
Scenario: Merging shares for recipient when shared from outside with user then group and recipient renames in between
Given using old dav path
Given As an "admin"
And user "user0" exists
And user "user1" exists
And group "group1" exists
And user "user1" belongs to group "group1"
And user "user0" created a folder "/merge-test-outside-groups-renamebeforesecondshare"
When folder "/merge-test-outside-groups-renamebeforesecondshare" of user "user0" is shared with user "user1"
And User "user1" moved folder "/merge-test-outside-groups-renamebeforesecondshare" to "/merge-test-outside-groups-renamebeforesecondshare-renamed"
And folder "/merge-test-outside-groups-renamebeforesecondshare" of user "user0" is shared with group "group1"
Then as "user1" gets properties of folder "/merge-test-outside-groups-renamebeforesecondshare-renamed" with
|{http://owncloud.org/ns}permissions|
And the single response should contain a property "{http://owncloud.org/ns}permissions" with value "SRDNVCK"
And as "user1" the folder "/merge-test-outside-groups-renamebeforesecondshare" does not exist

That sometimes fails.

Pre:

  • user0 exists
  • user1 exists
  • user1 is part of group1
  • user0 has a folder before

Now if the following happens in the same second

  1. user0 shares before with user1
  2. user1 renames before to after
  3. user0 shares before with group1

Now at this point the share table will have 2 entries.

  1. user share that has as target /after
  2. group share that has as target /before

However since they have the same time the merging logic kicks in. Which takes the group share as primary share.

Now if we flip the logic (which we could) then the reverse scenario would break (first group share then user share). Since we can't really compare id's properly here (abstracted away). The sharetime is really the only thing we have to go on.

Now all that said and done. I really think this scenario is not one that will pop up in real life. And if it does. Well bad luck you have to rename again. So I propose to just add a sleep of a second before sharing it to the group.

@MorrisJobke agreed?

Signed-off-by: Roeland Jago Douma <[email protected]>
@rullzer
Copy link
Member

rullzer commented Nov 4, 2016

All good now from me. LGTM

@MorrisJobke
Copy link
Member Author

@MorrisJobke agreed?

👍

I'm fine with this now 👍

@MorrisJobke MorrisJobke merged commit 6a156b7 into master Nov 4, 2016
@MorrisJobke MorrisJobke deleted the downstream-26235 branch November 4, 2016 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants