Skip to content

Server container cleanup #23249

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 9, 2020
Merged

Server container cleanup #23249

merged 1 commit into from
Oct 9, 2020

Conversation

@ChristophWurst
Copy link
Member

@ChristophWurst ChristophWurst commented Oct 7, 2020 •
edited
Loading

Follow-up to #23248

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

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

This comment has been minimized.

Sign in to view
@ChristophWurst ChristophWurst force-pushed the techdebt/server-container-cleanup branch 2 times, most recently from 2b68811 to 352814e Compare October 7, 2020 16:16
@ChristophWurst ChristophWurst 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
ChristophWurst
ChristophWurst commented Oct 7, 2020
View reviewed changes
lib/private/Server.php
});
$this->registerService(\OCP\ISession::class, function (ContainerInterface $c) {
return $c->get(\OCP\IUserSession::class)->getSession();
}, false);
Copy link
Member Author

@ChristophWurst ChristophWurst Oct 7, 2020 •
edited
Loading

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.

MorrisJobke
MorrisJobke reviewed Oct 7, 2020
View reviewed changes
lib/private/Server.php
$this->registerService(\OCP\Encryption\IManager::class, function (Server $c) {
$view = new View();
$util = new Encryption\Util(
$view,
Copy link
Member

@MorrisJobke MorrisJobke Oct 7, 2020

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.

MorrisJobke
MorrisJobke approved these changes Oct 7, 2020
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

🚀

@ChristophWurst
Copy link
Member Author

ChristophWurst commented Oct 8, 2020

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

Unrelated

@juliusknorr
Copy link
Member

juliusknorr commented Oct 8, 2020

Conflicts 😉

@ChristophWurst ChristophWurst force-pushed the techdebt/server-container-cleanup branch 2 times, most recently from ba94571 to ab26862 Compare October 8, 2020 14:42
@ChristophWurst ChristophWurst 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
@ChristophWurst
Clean up the server
56b67c8 
Signed-off-by: Christoph Wurst <christoph@winzerhof-wurst.at>
@ChristophWurst ChristophWurst force-pushed the techdebt/server-container-cleanup branch from ab26862 to 56b67c8 Compare October 9, 2020 06:26
@ChristophWurst
Copy link
Member Author

ChristophWurst commented Oct 9, 2020

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

@faily-bot
Copy link

faily-bot bot commented Oct 9, 2020

🤖 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 master Oct 9, 2020
@rullzer rullzer deleted the techdebt/server-container-cleanup branch October 9, 2020 07:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@MorrisJobke MorrisJobke MorrisJobke approved these changes

@juliusknorr juliusknorr juliusknorr approved these changes

@nickvergessen nickvergessen Awaiting requested review from nickvergessen

@rullzer rullzer Awaiting requested review from rullzer

@kesselb kesselb Awaiting requested review from kesselb

Assignees

@ChristophWurst ChristophWurst

Labels

4. to release Ready to be released and/or waiting for tests to finish technical debt

Projects

None yet

Milestone

Nextcloud 21

Development

Successfully merging this pull request may close these issues.

5 participants

@ChristophWurst @juliusknorr @MorrisJobke @rullzer