From 80645a591a5ed0892179e042001f32de79c4ebba Mon Sep 17 00:00:00 2001 From: Simon L Date: Mon, 17 Apr 2023 11:46:22 +0200 Subject: [PATCH 1/2] add an admin check for db file locking Signed-off-by: Simon L --- .../lib/Controller/CheckSetupController.php | 6 + .../Controller/CheckSetupControllerTest.php | 4 + core/js/setupchecks.js | 8 + core/js/tests/specs/setupchecksSpec.js | 140 ++++++++++++++++++ 4 files changed, 158 insertions(+) diff --git a/apps/settings/lib/Controller/CheckSetupController.php b/apps/settings/lib/Controller/CheckSetupController.php index 81a112cac55fb..cdf660bde865d 100644 --- a/apps/settings/lib/Controller/CheckSetupController.php +++ b/apps/settings/lib/Controller/CheckSetupController.php @@ -59,6 +59,7 @@ use OC\DB\SchemaWrapper; use OC\IntegrityCheck\Checker; use OC\Lock\NoopLockingProvider; +use OC\Lock\DBLockingProvider; use OC\MemoryInfo; use OCA\Settings\SetupChecks\CheckUserCertificates; use OCA\Settings\SetupChecks\LdapInvalidUuids; @@ -619,6 +620,10 @@ protected function hasWorkingFileLocking(): bool { return !($this->lockingProvider instanceof NoopLockingProvider); } + protected function hasDBFileLocking(): bool { + return ($this->lockingProvider instanceof DBLockingProvider); + } + protected function getSuggestedOverwriteCliURL(): string { $currentOverwriteCliUrl = $this->config->getSystemValue('overwrite.cli.url', ''); $suggestedOverwriteCliUrl = $this->request->getServerProtocol() . '://' . $this->request->getInsecureServerHost() . \OC::$WEBROOT; @@ -875,6 +880,7 @@ public function check() { 'wasEmailTestSuccessful' => $this->wasEmailTestSuccessful(), 'hasFileinfoInstalled' => $this->hasFileinfoInstalled(), 'hasWorkingFileLocking' => $this->hasWorkingFileLocking(), + 'hasDBFileLocking' => $this->hasDBFileLocking(), 'suggestedOverwriteCliURL' => $this->getSuggestedOverwriteCliURL(), 'cronInfo' => $this->getLastCronInfo(), 'cronErrors' => $this->getCronErrors(), diff --git a/apps/settings/tests/Controller/CheckSetupControllerTest.php b/apps/settings/tests/Controller/CheckSetupControllerTest.php index 81aa7af0b21c5..2678d5ce9ee63 100644 --- a/apps/settings/tests/Controller/CheckSetupControllerTest.php +++ b/apps/settings/tests/Controller/CheckSetupControllerTest.php @@ -502,6 +502,10 @@ public function testCheck() { ->expects($this->once()) ->method('hasWorkingFileLocking') ->willReturn(true); + $this->checkSetupController + ->expects($this->once()) + ->method('hasDBileLocking') + ->willReturn(true); $this->checkSetupController ->expects($this->once()) ->method('getSuggestedOverwriteCliURL') diff --git a/core/js/setupchecks.js b/core/js/setupchecks.js index b2d021c626515..6d88389699978 100644 --- a/core/js/setupchecks.js +++ b/core/js/setupchecks.js @@ -223,6 +223,14 @@ type: OC.SetupChecks.MESSAGE_TYPE_WARNING }); } + if(data.hasDBFileLocking) { + messages.push({ + msg: t('core', 'The database is used for transactional file locking. This will lead to performance issues. Use redis for transactional file locking to improve the performance. See the {linkstart}documentation ↗{linkend} for more information.') + .replace('{linkstart}', '') + .replace('{linkend}', ''), + type: OC.SetupChecks.MESSAGE_TYPE_INFO + }); + } if (data.suggestedOverwriteCliURL !== '') { messages.push({ msg: t('core', 'Please make sure to set the "overwrite.cli.url" option in your config.php file to the URL that your users mainly use to access this Nextcloud. Suggestion: "{suggestedOverwriteCliURL}". Otherwise there might be problems with the URL generation via cron. (It is possible though that the suggested URL is not the URL that your users mainly use to access this Nextcloud. Best is to double check this in any case.)', {suggestedOverwriteCliURL: data.suggestedOverwriteCliURL}), diff --git a/core/js/tests/specs/setupchecksSpec.js b/core/js/tests/specs/setupchecksSpec.js index 57536c595692b..16434e8077266 100644 --- a/core/js/tests/specs/setupchecksSpec.js +++ b/core/js/tests/specs/setupchecksSpec.js @@ -228,6 +228,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -289,6 +290,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -351,6 +353,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -410,6 +413,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: false, @@ -468,6 +472,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -526,6 +531,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: false, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -570,6 +576,124 @@ describe('OC.SetupChecks tests', function() { }); }); + it('should return an info if transactional file locking is not set up', function(done) { + var async = OC.SetupChecks.checkSetup(); + + suite.server.requests[0].respond( + 200, + { + 'Content-Type': 'application/json' + }, + JSON.stringify({ + hasFileinfoInstalled: true, + isGetenvServerWorking: true, + isReadOnlyConfig: false, + wasEmailTestSuccessful: true, + hasWorkingFileLocking: false, + hasDBFileLocking: false, + hasValidTransactionIsolationLevel: true, + suggestedOverwriteCliURL: '', + isRandomnessSecure: true, + securityDocs: 'https://docs.nextcloud.com/myDocs.html', + isFairUseOfFreePushService: true, + serverHasInternetConnectionProblems: false, + isMemcacheConfigured: true, + forwardedForHeadersWorking: true, + isCorrectMemcachedPHPModuleInstalled: true, + hasPassedCodeIntegrityCheck: true, + OpcacheSetupRecommendations: [], + isSettimelimitAvailable: true, + hasFreeTypeSupport: true, + missingIndexes: [], + missingPrimaryKeys: [], + missingColumns: [], + cronErrors: [], + cronInfo: { + diffInSeconds: 0 + }, + isMemoryLimitSufficient: true, + appDirsWithDifferentOwner: [], + isImagickEnabled: true, + areWebauthnExtensionsEnabled: true, + is64bit: true, + recommendedPHPModules: [], + pendingBigIntConversionColumns: [], + isMysqlUsedWithoutUTF8MB4: false, + isDefaultPhoneRegionSet: true, + isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, + reverseProxyGeneratedURL: 'https://server', + temporaryDirectoryWritable: true, + }) + ); + + async.done(function( data, s, x ){ + expect(data).toEqual([{ + msg: 'Transactional file locking is disabled, this might lead to issues with race conditions. Enable "filelocking.enabled" in config.php to avoid these problems. See the documentation ↗ for more information.', + type: OC.SetupChecks.MESSAGE_TYPE_WARNING + }]); + done(); + }); + }); + + it('should return an info if database file locking is used', function(done) { + var async = OC.SetupChecks.checkSetup(); + + suite.server.requests[0].respond( + 200, + { + 'Content-Type': 'application/json' + }, + JSON.stringify({ + hasFileinfoInstalled: true, + isGetenvServerWorking: true, + isReadOnlyConfig: false, + wasEmailTestSuccessful: true, + hasWorkingFileLocking: true, + hasDBFileLocking: true, + hasValidTransactionIsolationLevel: true, + suggestedOverwriteCliURL: '', + isRandomnessSecure: true, + securityDocs: 'https://docs.nextcloud.com/myDocs.html', + isFairUseOfFreePushService: true, + serverHasInternetConnectionProblems: false, + isMemcacheConfigured: true, + forwardedForHeadersWorking: true, + isCorrectMemcachedPHPModuleInstalled: true, + hasPassedCodeIntegrityCheck: true, + OpcacheSetupRecommendations: [], + isSettimelimitAvailable: true, + hasFreeTypeSupport: true, + missingIndexes: [], + missingPrimaryKeys: [], + missingColumns: [], + cronErrors: [], + cronInfo: { + diffInSeconds: 0 + }, + isMemoryLimitSufficient: true, + appDirsWithDifferentOwner: [], + isImagickEnabled: true, + areWebauthnExtensionsEnabled: true, + is64bit: true, + recommendedPHPModules: [], + pendingBigIntConversionColumns: [], + isMysqlUsedWithoutUTF8MB4: false, + isDefaultPhoneRegionSet: true, + isEnoughTempSpaceAvailableIfS3PrimaryStorageIsUsed: true, + reverseProxyGeneratedURL: 'https://server', + temporaryDirectoryWritable: true, + }) + ); + + async.done(function( data, s, x ){ + expect(data).toEqual([{ + msg: 'The database is used for transactional file locking. This will lead to performance issues. Use redis for transactional file locking to improve the performance. See the documentation ↗ for more information.', + type: OC.SetupChecks.MESSAGE_TYPE_INFO + }]); + done(); + }); + }); + it('should return a warning if there are app directories with wrong permissions', function(done) { var async = OC.SetupChecks.checkSetup(); @@ -584,6 +708,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -644,6 +769,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -702,6 +828,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -760,6 +887,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -838,6 +966,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -897,6 +1026,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -955,6 +1085,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -1013,6 +1144,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -1075,6 +1207,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -1134,6 +1267,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -1190,6 +1324,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -1249,6 +1384,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -1308,6 +1444,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -1366,6 +1503,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -1424,6 +1562,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, @@ -1482,6 +1621,7 @@ describe('OC.SetupChecks tests', function() { isReadOnlyConfig: false, wasEmailTestSuccessful: true, hasWorkingFileLocking: true, + hasDBFileLocking: false, hasValidTransactionIsolationLevel: true, suggestedOverwriteCliURL: '', isRandomnessSecure: true, From e2ab9bf4af3d9c801c4586b843cbc5419d6840fd Mon Sep 17 00:00:00 2001 From: Simon L Date: Tue, 18 Apr 2023 11:33:53 +0200 Subject: [PATCH 2/2] address review and fix tests Signed-off-by: Simon L --- apps/settings/tests/Controller/CheckSetupControllerTest.php | 4 +++- core/js/setupchecks.js | 2 +- core/js/tests/specs/setupchecksSpec.js | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/apps/settings/tests/Controller/CheckSetupControllerTest.php b/apps/settings/tests/Controller/CheckSetupControllerTest.php index 2678d5ce9ee63..2b074d24c3913 100644 --- a/apps/settings/tests/Controller/CheckSetupControllerTest.php +++ b/apps/settings/tests/Controller/CheckSetupControllerTest.php @@ -186,6 +186,7 @@ protected function setUp(): void { 'hasValidTransactionIsolationLevel', 'hasFileinfoInstalled', 'hasWorkingFileLocking', + 'hasDBFileLocking', 'getLastCronInfo', 'getSuggestedOverwriteCliURL', 'getCurlVersion', @@ -504,7 +505,7 @@ public function testCheck() { ->willReturn(true); $this->checkSetupController ->expects($this->once()) - ->method('hasDBileLocking') + ->method('hasDBFileLocking') ->willReturn(true); $this->checkSetupController ->expects($this->once()) @@ -608,6 +609,7 @@ public function testCheck() { 'hasValidTransactionIsolationLevel' => true, 'hasFileinfoInstalled' => true, 'hasWorkingFileLocking' => true, + 'hasDBFileLocking' => true, 'suggestedOverwriteCliURL' => '', 'cronInfo' => [ 'diffInSeconds' => 123, diff --git a/core/js/setupchecks.js b/core/js/setupchecks.js index 6d88389699978..8cb8f76e40c3f 100644 --- a/core/js/setupchecks.js +++ b/core/js/setupchecks.js @@ -225,7 +225,7 @@ } if(data.hasDBFileLocking) { messages.push({ - msg: t('core', 'The database is used for transactional file locking. This will lead to performance issues. Use redis for transactional file locking to improve the performance. See the {linkstart}documentation ↗{linkend} for more information.') + msg: t('core', 'The database is used for transactional file locking. To enhance performance, please configure memcache, if available. See the {linkstart}documentation ↗{linkend} for more information.') .replace('{linkstart}', '') .replace('{linkend}', ''), type: OC.SetupChecks.MESSAGE_TYPE_INFO diff --git a/core/js/tests/specs/setupchecksSpec.js b/core/js/tests/specs/setupchecksSpec.js index 16434e8077266..a9a66970f3339 100644 --- a/core/js/tests/specs/setupchecksSpec.js +++ b/core/js/tests/specs/setupchecksSpec.js @@ -687,7 +687,7 @@ describe('OC.SetupChecks tests', function() { async.done(function( data, s, x ){ expect(data).toEqual([{ - msg: 'The database is used for transactional file locking. This will lead to performance issues. Use redis for transactional file locking to improve the performance. See the documentation ↗ for more information.', + msg: 'The database is used for transactional file locking. To enhance performance, please configure memcache, if available. See the documentation ↗ for more information.', type: OC.SetupChecks.MESSAGE_TYPE_INFO }]); done();