Don't update library version if uploaded settings are unchanged

Since settings are written in a single transaction, the library version update on a write can be rolled back if nothing changed. Previously the reverted library version was being returned but the updated version was still being saved. For other object types, multi-writes happen in separate transactions, so a no-op still updates the library version (though this could possibly be fixed).
VivienDelmon · Jun 30, 2016 · ba7c5ee · ba7c5ee
1 parent 364f6e1
commit ba7c5ee
Show file tree

Hide file tree

Showing 3 changed files with 143 additions and 6 deletions.
diff --git a/controllers/SettingsController.php b/controllers/SettingsController.php
@@ -92,7 +92,6 @@ public function settings() {
 						$this->libraryVersion = Zotero_Libraries::getOriginalVersion(
 							$this->objectLibraryID
 						);
-
 						Zotero_DB::rollback();
 						$this->e204();
 					}
@@ -138,9 +137,11 @@ public function settings() {
 					$this->libraryVersion = Zotero_Libraries::getOriginalVersion(
 						$this->objectLibraryID
 					);
+					Zotero_DB::rollback();
+				}
+				else {
+					Zotero_DB::commit();
 				}
-
-				Zotero_DB::commit();
 				$this->e204();
 			}
 			// Display all settings

diff --git a/tests/remote/tests/API/3/CollectionTest.php b/tests/remote/tests/API/3/CollectionTest.php
@@ -180,11 +180,34 @@ public function testCreateKeyedCollections() {
 	}
 
 
-	public function testEditMultipleCollections() {
+	public function testUpdateMultipleCollections() {
 		$collection1Data = API::createCollection("Test 1", false, $this, 'jsonData');
 		$collection2Name = "Test 2";
 		$collection2Data = API::createCollection($collection2Name, false, $this, 'jsonData');
 
+		$libraryVersion = API::getLibraryVersion();
+
+		// Update with no change, which should still update library version (for now)
+		$response = API::userPost(
+			self::$config['userID'],
+			"collections",
+			json_encode([
+				$collection1Data,
+				$collection2Data
+			]),
+			[
+				"Content-Type: application/json"
+			]
+		);
+		$this->assert200($response);
+		// If this behavior changes, remove the pre-increment
+		$this->assertEquals(++$libraryVersion, $response->getHeader("Last-Modified-Version"));
+		$json = API::getJSONFromResponse($response);
+		$this->assertCount(2, $json['unchanged']);
+
+		$this->assertEquals($libraryVersion, API::getLibraryVersion());
+
+		// Update
 		$collection1NewName = "Test 1 Modified";
 		$collection2NewParentKey = API::createCollection("Test 3", false, $this, 'key');
 

diff --git a/tests/remote/tests/API/3/SettingsTest.php b/tests/remote/tests/API/3/SettingsTest.php
@@ -151,6 +151,7 @@ public function testAddUserSettingMultiple() {
 			array("Content-Type: application/json")
 		);
 		$this->assert204($response);
+		$this->assertEquals(++$libraryVersion, $response->getHeader("Last-Modified-Version"));
 
 		// Multi-object GET
 		$response = API::userGet(
@@ -164,7 +165,7 @@ public function testAddUserSettingMultiple() {
 		foreach ($settingKeys as $settingKey) {
 			$this->assertObjectHasAttribute($settingKey, $json2, "Object should have $settingKey property");
 			$this->assertEquals($json->$settingKey->value, $json2->$settingKey->value, "'$settingKey' value should match");
-			$this->assertEquals($libraryVersion + 1, $json2->$settingKey->version, "'$settingKey' version should match");
+			$this->assertEquals($libraryVersion, $json2->$settingKey->version, "'$settingKey' version should match");
 		}
 
 		// Single-object GET
@@ -178,7 +179,7 @@ public function testAddUserSettingMultiple() {
 			$json2 = json_decode($response->getBody());
 			$this->assertNotNull($json2);
 			$this->assertEquals($json->$settingKey->value, $json2->value);
-			$this->assertEquals($libraryVersion + 1, $json2->version);
+			$this->assertEquals($libraryVersion, $json2->version);
 		}
 	}
 
@@ -332,6 +333,118 @@ public function testUpdateUserSetting() {
 	}
 
 
+	public function testUpdateUserSettings() {
+		$settingKey = "tagColors";
+		$value = [
+			[
+				"name" => "_READ",
+				"color" => "#990000"
+			]
+		];
+
+		$libraryVersion = API::getLibraryVersion();
+
+		$json = [
+			"value" => $value,
+			"version" => 0
+		];
+
+		// Create
+		$response = API::userPut(
+			self::$config['userID'],
+			"settings/$settingKey",
+			json_encode($json),
+			[
+				"Content-Type: application/json"
+			]
+		);
+		$this->assert204($response);
+		$this->assertEquals(++$libraryVersion, $response->getHeader('Last-Modified-Version'));
+
+		$response = API::userGet(
+			self::$config['userID'],
+			"settings"
+		);
+		$this->assert200($response);
+		$this->assertContentType("application/json", $response);
+		$this->assertEquals($libraryVersion, $response->getHeader('Last-Modified-Version'));
+		$json = json_decode($response->getBody(), true);
+		$this->assertNotNull($json);
+		$this->assertArrayHasKey($settingKey, $json);
+		$this->assertEquals($value, $json[$settingKey]['value']);
+		$this->assertEquals($libraryVersion, $json[$settingKey]['version']);
+
+		// Update with no change
+		$response = API::userPost(
+			self::$config['userID'],
+			"settings",
+			json_encode([
+				$settingKey => [
+					"value" => $value
+				]
+			]),
+			[
+				"Content-Type: application/json",
+				"If-Unmodified-Since-Version: $libraryVersion"
+			]
+		);
+		$this->assert204($response);
+		$this->assertEquals($libraryVersion, $response->getHeader('Last-Modified-Version'));
+
+		// Check
+		$response = API::userGet(
+			self::$config['userID'],
+			"settings"
+		);
+		$this->assert200($response);
+		$this->assertContentType("application/json", $response);
+		$this->assertEquals($libraryVersion, $response->getHeader('Last-Modified-Version'));
+		$json = json_decode($response->getBody(), true);
+		$this->assertNotNull($json);
+		$this->assertArrayHasKey($settingKey, $json);
+		$this->assertEquals($value, $json[$settingKey]['value']);
+		$this->assertEquals($libraryVersion, $json[$settingKey]['version']);
+
+		$newValue = [
+			[
+				"name" => "_READ",
+				"color" => "#CC9933"
+			]
+		];
+
+		// Update
+		$response = API::userPost(
+			self::$config['userID'],
+			"settings",
+			json_encode([
+				$settingKey => [
+					"value" => $newValue
+				]
+			]),
+			[
+				"Content-Type: application/json",
+				"If-Unmodified-Since-Version: $libraryVersion"
+			]
+		);
+		$this->assert204($response);
+		$this->assertEquals(++$libraryVersion, $response->getHeader('Last-Modified-Version'));
+
+		// Check
+		$response = API::userGet(
+			self::$config['userID'],
+			"settings"
+		);
+		$this->assert200($response);
+		$this->assertContentType("application/json", $response);
+		$this->assertEquals($libraryVersion, $response->getHeader('Last-Modified-Version'));
+		$json = json_decode($response->getBody(), true);
+		$this->assertNotNull($json);
+		$this->assertArrayHasKey($settingKey, $json);
+		$this->assertEquals($newValue, $json[$settingKey]['value']);
+		$this->assertEquals($libraryVersion, $json[$settingKey]['version']);
+	}
+
+
 	public function testDeleteUserSetting() {
 		$settingKey = "tagColors";
 		$value = array(