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.

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

Merge sqlite check to database check #41535

Merged
merged 1 commit into from
Dec 11, 2023
+36 −25
Merged

Merge sqlite check to database check #41535

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions apps/settings/lib/Controller/CheckSetupController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,10 +401,6 @@ protected function getOpcacheSetupRecommendations(): array {
return $recommendations;
}

protected function isSqliteUsed() {
return str_contains($this->config->getSystemValue('dbtype'), 'sqlite');
}

protected function getSuggestedOverwriteCliURL(): string {
$currentOverwriteCliUrl = $this->config->getSystemValue('overwrite.cli.url', '');
$suggestedOverwriteCliUrl = $this->request->getServerProtocol() . '://' . $this->request->getInsecureServerHost() . \OC::$WEBROOT;
Expand Down Expand Up @@ -574,8 +570,6 @@ public function check() {
'codeIntegrityCheckerDocumentation' => $this->urlGenerator->linkToDocs('admin-code-integrity'),
'OpcacheSetupRecommendations' => $this->getOpcacheSetupRecommendations(),
'isSettimelimitAvailable' => $this->isSettimelimitAvailable(),
'isSqliteUsed' => $this->isSqliteUsed(),
'databaseConversionDocumentation' => $this->urlGenerator->linkToDocs('admin-db-conversion'),
'appDirsWithDifferentOwner' => $this->getAppDirsWithDifferentOwner(),
'isImagickEnabled' => $this->isImagickEnabled(),
'areWebauthnExtensionsEnabled' => $this->areWebauthnExtensionsEnabled(),
Expand Down
7 changes: 6 additions & 1 deletion apps/settings/lib/SetupChecks/SupportedDatabase.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,12 +33,14 @@
use Doctrine\DBAL\Platforms\SqlitePlatform;
use OCP\IDBConnection;
use OCP\IL10N;
use OCP\IURLGenerator;
use OCP\SetupCheck\ISetupCheck;
use OCP\SetupCheck\SetupResult;

class SupportedDatabase implements ISetupCheck {
public function __construct(
private IL10N $l10n,
private IURLGenerator $urlGenerator,
private IDBConnection $connection,
) {
}
Expand Down Expand Up @@ -81,7 +83,10 @@ public function run(): SetupResult {
} elseif ($databasePlatform instanceof OraclePlatform) {
$version = 'Oracle';
} elseif ($databasePlatform instanceof SqlitePlatform) {
$version = 'Sqlite';
return SetupResult::warning(
$this->l10n->t('SQLite is currently being used as the backend database. For larger installations we recommend that you switch to a different database backend. This is particularly recommended when using the desktop client for file synchronisation. To migrate to another database use the command line tool: "occ db:convert-type".'),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we add this warning only if the instance have more than 5 users?
I am thinking about a small arbitrary limit so an admin of a very small instance won't have this warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, but should we?
One thing that pops to my mind is that if the admin is setting up for a large instance he will have no users at the beginning and this warning will only be seen once growing, and migration can be hard?
But yeah maybe we can always put the same description, but set the status to success/info/warning depending if user number is <=5/>5/>20?

Copy link
Member

Choose a reason for hiding this comment

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

https://docs.nextcloud.com/server/latest/admin_manual/installation/system_requirements.html

SQLite (only recommended for testing and minimal-instances)

I think warning is totally fine for "production" instances. It always can have negative performance impact.
We also warn about int32 when you are alone, not having a memcache (info only), missing indexes, etc.

I think warning against SQLite is totally fine as it's really recommended to go with anything else.

$this->urlGenerator->linkToDocs('admin-db-conversion')
);
} else {
return SetupResult::error($this->l10n->t('Unknown database platform'));
}
Expand Down
6 changes: 0 additions & 6 deletions apps/settings/tests/Controller/CheckSetupControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,6 @@ protected function setUp(): void {
'getCurlVersion',
'isPhpOutdated',
'getOpcacheSetupRecommendations',
'isSqliteUsed',
'isPHPMailerUsed',
'getAppDirsWithDifferentOwner',
'isImagickEnabled',
Expand Down Expand Up @@ -213,9 +212,6 @@ public function testCheck() {
->expects($this->once())
->method('getOpcacheSetupRecommendations')
->willReturn(['recommendation1', 'recommendation2']);
$this->checkSetupController
->method('isSqliteUsed')
->willReturn(false);
$this->checkSetupController
->expects($this->once())
->method('getSuggestedOverwriteCliURL')
Expand Down Expand Up @@ -305,8 +301,6 @@ public function testCheck() {
'codeIntegrityCheckerDocumentation' => 'http://docs.example.org/server/go.php?to=admin-code-integrity',
'OpcacheSetupRecommendations' => ['recommendation1', 'recommendation2'],
'isSettimelimitAvailable' => true,
'isSqliteUsed' => false,
'databaseConversionDocumentation' => 'http://docs.example.org/server/go.php?to=admin-db-conversion',
'appDirsWithDifferentOwner' => [],
'isImagickEnabled' => false,
'areWebauthnExtensionsEnabled' => false,
Expand Down
33 changes: 30 additions & 3 deletions apps/settings/tests/SetupChecks/SupportedDatabaseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,18 +25,45 @@
*/
namespace OCA\Settings\Tests;

use Doctrine\DBAL\Platforms\SqlitePlatform;
use OCA\Settings\SetupChecks\SupportedDatabase;
use OCP\IDBConnection;
use OCP\IL10N;
use OCP\IUrlGenerator;
use OCP\SetupCheck\SetupResult;
use Test\TestCase;

/**
* @group DB
*/
class SupportedDatabaseTest extends TestCase {
private IL10N $l10n;
private IUrlGenerator $urlGenerator;
private IDBConnection $connection;

private SupportedDatabase $check;

protected function setUp(): void {
parent::setUp();

$this->l10n = $this->getMockBuilder(IL10N::class)->getMock();
$this->urlGenerator = $this->getMockBuilder(IUrlGenerator::class)->getMock();
$this->connection = \OCP\Server::get(IDBConnection::class);

$this->check = new SupportedDatabase(
$this->l10n,
$this->urlGenerator,
\OCP\Server::get(IDBConnection::class)
);
}

public function testPass(): void {
$l10n = $this->getMockBuilder(IL10N::class)->getMock();
$check = new SupportedDatabase($l10n, \OC::$server->getDatabaseConnection());
$this->assertEquals(SetupResult::SUCCESS, $check->run()->getSeverity());
$platform = $this->connection->getDatabasePlatform();
if ($platform instanceof SqlitePlatform) {
/** SQlite always gets a warning */
$this->assertEquals(SetupResult::WARNING, $this->check->run()->getSeverity());
} else {
$this->assertEquals(SetupResult::SUCCESS, $this->check->run()->getSeverity());
}
}
}
9 changes: 0 additions & 9 deletions core/js/setupchecks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,15 +282,6 @@
type: OC.SetupChecks.MESSAGE_TYPE_INFO
})
}
if (data.isSqliteUsed) {
messages.push({
msg: t('core', 'SQLite is currently being used as the backend database. For larger installations we recommend that you switch to a different database backend.') + ' ' + t('core', 'This is particularly recommended when using the desktop client for file synchronisation.') + ' ' +
t('core', 'To migrate to another database use the command line tool: "occ db:convert-type", or see the {linkstart}documentation ↗{linkend}.')
.replace('{linkstart}', '<a target="_blank" rel="noreferrer noopener" class="external" href="' + data.databaseConversionDocumentation + '">')
.replace('{linkend}', '</a>'),
type: OC.SetupChecks.MESSAGE_TYPE_WARNING
})
}

if(data.appDirsWithDifferentOwner && data.appDirsWithDifferentOwner.length > 0) {
var appDirsWithDifferentOwner = data.appDirsWithDifferentOwner.reduce(
Expand Down
Loading