Merge pull request #47933 from nextcloud/bugfix/noid/throw-precondition-failure-when-not-matching

fix(config): Throw PreconditionException always when it didn't match

Author: nickvergessen

lib/private/AllConfig.php

@@ -242,10 +242,10 @@ public function setUserValue($userId, $appName, $key, $value, $preCondition = nu
 		$prevValue = $this->getUserValue($userId, $appName, $key, null);
 
 		if ($prevValue !== null) {
-			if ($prevValue === (string)$value) {
-				return;
-			} elseif ($preCondition !== null && $prevValue !== (string)$preCondition) {
+			if ($preCondition !== null && $prevValue !== (string)$preCondition) {
 				throw new PreConditionNotMetException();
+			} elseif ($prevValue === (string)$value) {
+				return;
 			} else {
 				$qb = $this->connection->getQueryBuilder();
 				$qb->update('preferences')

tests/lib/AllConfigTest.php

@@ -182,6 +182,42 @@ public function testSetUserValueWithPreConditionFailure() {
 		$config->deleteUserValue('userPreCond1', 'appPreCond', 'keyPreCond');
 	}
 
+	public function testSetUserValueWithPreConditionFailureWhenResultStillMatches(): void {
+		$this->expectException(\OCP\PreConditionNotMetException::class);
+
+		$config = $this->getConfig();
+
+		$selectAllSQL = 'SELECT `userid`, `appid`, `configkey`, `configvalue` FROM `*PREFIX*preferences` WHERE `userid` = ?';
+
+		$config->setUserValue('userPreCond1', 'appPreCond', 'keyPreCond', 'valuePreCond');
+
+		$result = $this->connection->executeQuery($selectAllSQL, ['userPreCond1'])->fetchAll();
+
+		$this->assertCount(1, $result);
+		$this->assertEquals([
+			'userid' => 'userPreCond1',
+			'appid' => 'appPreCond',
+			'configkey' => 'keyPreCond',
+			'configvalue' => 'valuePreCond'
+		], $result[0]);
+
+		// test if the method throws with invalid precondition when the value is the same
+		$config->setUserValue('userPreCond1', 'appPreCond', 'keyPreCond', 'valuePreCond', 'valuePreCond3');
+
+		$result = $this->connection->executeQuery($selectAllSQL, ['userPreCond1'])->fetchAll();
+
+		$this->assertCount(1, $result);
+		$this->assertEquals([
+			'userid' => 'userPreCond1',
+			'appid' => 'appPreCond',
+			'configkey' => 'keyPreCond',
+			'configvalue' => 'valuePreCond'
+		], $result[0]);
+
+		// cleanup
+		$config->deleteUserValue('userPreCond1', 'appPreCond', 'keyPreCond');
+	}
+
 	public function testSetUserValueUnchanged() {
 		// TODO - FIXME until the dependency injection is handled properly (in AllConfig)
 		$this->markTestSkipped('Skipped because this is just testable if database connection can be injected');