Skip to content

Server container cleanup - #23249

Merged
rullzer merged 1 commit into
masterfrom
techdebt/server-container-cleanup
Oct 9, 2020
Merged

Server container cleanup#23249
rullzer merged 1 commit into
masterfrom
techdebt/server-container-cleanup

Conversation

@ChristophWurst

@ChristophWurstChristophWurst commented Oct 7, 2020

Copy link
Copy Markdown
Member

Follow-up to #23248

  • Replace most calls of the deprecated getter method with the direct query
  • Use the ContainerInterface where possible
  • Minor formatting enhancements

@ChristophWurstChristophWurst added the 1. to develop Accepted and waiting to be taken care of label Oct 7, 2020
@ChristophWurstChristophWurst added this to the Nextcloud 21 milestone Oct 7, 2020
@ChristophWurstChristophWurst self-assigned this Oct 7, 2020
@ChristophWurst

This comment has been minimized.

@ChristophWurst
ChristophWurstforce-pushed the techdebt/server-container-cleanup branch 2 times, most recently from 2b68811 to 352814eCompareOctober 7, 2020 16:16
@ChristophWurstChristophWurst added 3. to review Waiting for reviews technical debt 🧱 🤔🚀 and removed 1. to develop Accepted and waiting to be taken care of labels Oct 7, 2020
@ChristophWurst
ChristophWurst marked this pull request as ready for review October 7, 2020 16:20
});
$this->registerService(\OCP\ISession::class, function (ContainerInterface $c) {
return $c->get(\OCP\IUserSession::class)->getSession();
}, false);

@ChristophWurstChristophWurstOct 7, 2020

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

⚠️ 🤡 This little boolean here is critical. If we don't have it then Nextcloud just does 💥. The reason is that the old code usually queried the user session and then got the session from there. With the new code we always query the session. But the container remembers the result so it only builds once. And since the session object can change, the container will continue to deliver the previous object, while going the detour to the user session gives you the new one. It's a huge mess because this also means the ISession a class gets injected depends on when the object is build.

It was 💩 before. It is 💩 after. This needs some love, but not here.

$this->registerService(\OCP\Encryption\IManager::class, function (Server $c) {
$view = new View();
$util = new Encryption\Util(
$view,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can do the trick with the view (to specify that the CI container should always initialize a new object and not cache it) as well and then let it auto-wire all those hard coded things. But maybe in a followup PR to make it easier to roll it back.

@MorrisJobkeMorrisJobke left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

🚀

@ChristophWurst

Copy link
Copy Markdown
MemberAuthor

Bildschirmfoto von 2020-10-08 08-27-02

Unrelated

@juliusknorr

Copy link
Copy Markdown
Member

Conflicts 😉

@ChristophWurst
ChristophWurstforce-pushed the techdebt/server-container-cleanup branch 2 times, most recently from ba94571 to ab26862CompareOctober 8, 2020 14:42
@ChristophWurstChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Oct 8, 2020
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst
ChristophWurstforce-pushed the techdebt/server-container-cleanup branch from ab26862 to 56b67c8CompareOctober 9, 2020 06:26
@ChristophWurst

Copy link
Copy Markdown
MemberAuthor

Okay actually same failures in other PRs like #23295 so I guess this can be merged.

@faily-bot

faily-botBot commented Oct 9, 2020

Copy link
Copy Markdown

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 33896: failure

sqlite

Show full log
There were 2 warnings:
1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
--
There was 1 failure:
1) TrashbinTest::testExpireOldFilesShared
Failed asserting that 3 is identical to 2.
/drone/src/apps/files_trashbin/tests/TrashbinTest.php:304
/drone/src/apps/files_trashbin/tests/TrashbinTest.php:287

mariadb10.1-php7.3

mariadb10.4-php7.4

mysql8.0-php7.4

mysql5.6-php7.3

postgres9-php7.3

Show full log
There were 2 warnings:
1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static
--
There was 1 failure:
1) TrashbinTest::testExpireOldFilesShared
Failed asserting that 3 is identical to 2.
/drone/src/apps/files_trashbin/tests/TrashbinTest.php:304
/drone/src/apps/files_trashbin/tests/TrashbinTest.php:287

postgres11-php7.4

acceptance-app-files-sharing

  • tests/acceptance/features/app-files-sharing.feature:338
Show full log
 Scenario: sharee can revoke create permission from reshare after the sharer disabled it # /drone/src/tests/acceptance/features/app-files-sharing.feature:338
Given I act as John # ActorContext::iActAs()
And I am logged in as the admin # LoginPageContext::iAmLoggedInAsTheAdmin()
And I act as Jane # ActorContext::iActAs()
And I am logged in # LoginPageContext::iAmLoggedIn()
And I act as Jim # ActorContext::iActAs()
And I am logged in as "user1" # LoginPageContext::iAmLoggedInAs()
And I act as John # ActorContext::iActAs()
And I create a new folder named "Shared folder" # FileListContext::iCreateANewFolderNamed()
And I see that the file list contains a file named "Shared folder" # FileListContext::iSeeThatTheFileListContainsAFileNamed()
And I share "Shared folder" with "user0" # FilesAppSharingContext::iShareWith()
And I see that the file is shared with "user0" # FilesAppSharingContext::iSeeThatTheFileIsSharedWith()
And I act as Jane # ActorContext::iActAs()
And I open the Files app # FilesAppContext::iOpenTheFilesApp()
And I share "Shared folder" with "user1" # FilesAppSharingContext::iShareWith()
And I see that the file is shared with "user1" # FilesAppSharingContext::iSeeThatTheFileIsSharedWith()
And I act as John # ActorContext::iActAs()
And I set the share with "user0" as not creatable # FilesAppSharingContext::iSetTheShareWithAsNotCreatable()
And I see that "user0" can not create in the share # FilesAppSharingContext::iSeeThatCanNotCreateInTheShare()
And I act as Jim # ActorContext::iActAs()
And I open the Files app # FilesAppContext::iOpenTheFilesApp()
And I enter in the folder named "Shared folder" # FileListContext::iEnterInTheFolderNamed()
And I create a new folder named "Subfolder" # FileListContext::iCreateANewFolderNamed()
And I see that the file list contains a file named "Subfolder" # FileListContext::iSeeThatTheFileListContainsAFileNamed()
When I act as Jane # ActorContext::iActAs()
And I open the Files app # FilesAppContext::iOpenTheFilesApp()
And I open the details view for "Shared folder" # FileListContext::iOpenTheDetailsViewFor()
And I see that the details view is open # FilesAppContext::iSeeThatTheDetailsViewIsOpen()
And I open the "Sharing" tab in the details view # FilesAppContext::iOpenTheTabInTheDetailsView()
And I see that the "Sharing" tab in the details view is eventually loaded # FilesAppContext::iSeeThatTheTabInTheDetailsViewIsEventuallyLoaded()
And I set the share with "user1" as not creatable # FilesAppSharingContext::iSetTheShareWithAsNotCreatable()
Then I see that "user1" can not create in the share # FilesAppSharingContext::iSeeThatCanNotCreateInTheShare()
And I see that "user1" can not be allowed to create in the share # FilesAppSharingContext::iSeeThatCanNotBeAllowedToCreateInTheShare()
And I act as Jim # ActorContext::iActAs()
And I open the Files app # FilesAppContext::iOpenTheFilesApp()
And I enter in the folder named "Shared folder" # FileListContext::iEnterInTheFolderNamed()
And I see that it is not possible to create new files # FileListContext::iSeeThatItIsNotPossibleToCreateNewFiles()
Failed asserting that true is false.

@rullzer
rullzer merged commit 049d5b3 into masterOct 9, 2020
@rullzer
rullzer deleted the techdebt/server-container-cleanup branch October 9, 2020 07:19
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to releaseReady to be released and/or waiting for tests to finishtechnical debt🧱 🤔🚀

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@ChristophWurst@juliusknorr@MorrisJobke@rullzer