From 3c1080adffc185d0de58ef18312bc1dd974f70c3 Mon Sep 17 00:00:00 2001 From: Bogdan Abaev Date: Mon, 18 Aug 2025 20:29:31 +0000 Subject: [PATCH] remove bidirectional nature of related items Related items have been treated as bidirectional after https://github.com/zotero/dataserver/commit/fbaa602bdd32b0f43b8997f51aa13686986eab01. This means that if item A is one of related items for item B but not the other way around, item B would still be included in the list of related items for item A. This caused sync issue when one of two related items was deleted. Modifications to the non-deleted item are synced first, followed by the deletion. The dataserver would return the relation to the item that was about to be deleted after modification of the non-deleted item is handled. This would trick Zotero on the client side into believing that the relation to the deleted item still existed. More context: https://github.com/zotero/zotero/pull/5486 --- model/Item.inc.php | 24 -------- tests/remote/tests/API/2/RelationTest.php | 23 +++++--- tests/remote/tests/API/3/RelationTest.php | 72 ++++++++++++++++++++--- 3 files changed, 78 insertions(+), 41 deletions(-) diff --git a/model/Item.inc.php b/model/Item.inc.php index 3a76fb3aa..6732c125a 100644 --- a/model/Item.inc.php +++ b/model/Item.inc.php @@ -5020,18 +5020,6 @@ protected function loadRelations($reload = false) { return [$rel->predicate, $rel->object]; }, $relations); - // Related items are bidirectional, so include any with this item as the object - $reverseRelations = Zotero_Relations::getByURIs( - $this->libraryID, false, Zotero_Relations::$relatedItemPredicate, $itemURI - ); - foreach ($reverseRelations as $rel) { - $r = [$rel->predicate, $rel->subject]; - // Only add if not already added in other direction - if (!in_array($r, $relations)) { - $relations[] = $r; - } - } - // Also include any owl:sameAs relations with this item as the object // (as sent by client via classic sync) $reverseRelations = Zotero_Relations::getByURIs( @@ -5055,18 +5043,6 @@ protected function loadRelations($reload = false) { $relations[] = [$predicate, $prefix . $key]; } } - // Reverse as well - $sql = "SELECT `key` FROM itemRelated IR JOIN items I USING (itemID) WHERE IR.linkedItemID=?"; - $reverseRelatedItemKeys = Zotero_DB::columnQuery( - $sql, $this->id, Zotero_Shards::getByLibraryID($this->libraryID) - ); - if ($reverseRelatedItemKeys) { - $prefix = Zotero_URI::getLibraryURI($this->libraryID) . "/items/"; - $predicate = Zotero_Relations::$relatedItemPredicate; - foreach ($reverseRelatedItemKeys as $key) { - $relations[] = [$predicate, $prefix . $key]; - } - } $this->relations = $relations; $this->loaded['relations'] = true; diff --git a/tests/remote/tests/API/2/RelationTest.php b/tests/remote/tests/API/2/RelationTest.php index 3d9021f9f..3648b218c 100644 --- a/tests/remote/tests/API/2/RelationTest.php +++ b/tests/remote/tests/API/2/RelationTest.php @@ -102,24 +102,30 @@ public function testRelatedItemRelations() { $this->assertEquals($object, $json['relations'][$predicate]); } - // And item 2, since related items are bidirectional + // It does not yet exist on item 2 $xml = API::getItemXML($item2JSON['itemKey']); $data = API::parseDataFromAtomEntry($xml); $item2JSON = json_decode($data['content'], true); - $this->assertCount(1, $item2JSON['relations']); - $this->assertEquals($item1URI, $item2JSON["relations"]["dc:relation"]); + $this->assertCount(0, $item2JSON['relations']); - // Sending item 2's unmodified JSON back up shouldn't cause the item to be updated. - // Even though we're sending a relation that's technically not part of the item, - // when it loads the item it will load the reverse relations too and therefore not - // add a relation that it thinks already exists. + // Update item 2 + $relationsTwo["dc:relation"] = $item1URI; + $item2JSON["relations"] = $relationsTwo; $response = API::userPut( self::$config['userID'], "items/{$item2JSON['itemKey']}?key=" . self::$config['apiKey'], json_encode($item2JSON) ); $this->assert204($response); - $this->assertEquals($item2JSON['itemVersion'], $response->getHeader("Last-Modified-Version")); + + // It should now exist on item 2 + $xml = API::getItemXML($item2JSON['itemKey']); + $data = API::parseDataFromAtomEntry($xml); + $json = json_decode($data['content'], true); + $this->assertCount(sizeOf($relationsTwo), $json['relations']); + foreach ($relationsTwo as $predicate => $object) { + $this->assertEquals($object, $json['relations'][$predicate]); + } } @@ -141,6 +147,7 @@ public function testRelatedItemRelationsSingleRequest() { $item2JSON = API::getItemTemplate('book'); $item2JSON->itemKey = $item2Key; $item2JSON->itemVersion = 0; + $item2JSON->relations->{'dc:relation'} = $item1URI; $response = API::postItems([$item1JSON, $item2JSON]); $this->assert200($response); diff --git a/tests/remote/tests/API/3/RelationTest.php b/tests/remote/tests/API/3/RelationTest.php index 6f47f7d49..77bd3f34a 100644 --- a/tests/remote/tests/API/3/RelationTest.php +++ b/tests/remote/tests/API/3/RelationTest.php @@ -98,22 +98,24 @@ public function testRelatedItemRelations() { $this->assertEquals($object, $json['relations'][$predicate]); } - // And item 2, since related items are bidirectional + // Relation does not yet exist on item 2 $item2JSON = API::getItem($item2JSON['key'], $this, 'json')['data']; - $this->assertCount(1, $item2JSON['relations']); - $this->assertEquals($item1URI, $item2JSON["relations"]["dc:relation"]); + $this->assertCount(0, $item2JSON['relations']); - // Sending item 2's unmodified JSON back up shouldn't cause the item to be updated. - // Even though we're sending a relation that's technically not part of the item, - // when it loads the item it will load the reverse relations too and therefore not - // add a relation that it thinks already exists. + // Update item 2 + $relationsTwo["dc:relation"] = $item1URI; + $item2JSON["relations"] = $relationsTwo; $response = API::userPut( self::$config['userID'], "items/{$item2JSON['key']}", json_encode($item2JSON) ); $this->assert204($response); - $this->assertEquals($item2JSON['version'], $response->getHeader("Last-Modified-Version")); + + // Relation now exists on item 2 + $item2JSON = API::getItem($item2JSON['key'], $this, 'json')['data']; + $this->assertCount(1, $item2JSON['relations']); + $this->assertEquals($item1URI, $item2JSON["relations"]["dc:relation"]); } @@ -134,6 +136,7 @@ public function testRelatedItemRelationsSingleRequest() { $item2JSON = API::getItemTemplate('book'); $item2JSON->key = $item2Key; $item2JSON->version = 0; + $item2JSON->relations->{'dc:relation'} = $item1URI; $response = API::postItems([$item1JSON, $item2JSON]); $this->assert200($response); @@ -149,6 +152,57 @@ public function testRelatedItemRelationsSingleRequest() { $this->assertCount(1, $json['relations']); $this->assertEquals($item1URI, $json['relations']['dc:relation']); } + + public function testUnrelateTwoItems() { + $uriPrefix = "http://zotero.org/users/" . self::$config['userID'] . "/items/"; + + // Create two items related to one another + $item1JSON = API::createItem("book", null, $this, 'jsonData'); + $item2JSON = API::createItem("book", null, $this, 'jsonData'); + + $item1JSON["relations"] = [ + "dc:relation" => $uriPrefix . $item2JSON['key'] + ]; + $item2JSON["relations"] = [ + "dc:relation" => $uriPrefix . $item1JSON['key'] + ]; + $response = API::userPut( + self::$config['userID'], + "items/{$item1JSON['key']}", + json_encode($item1JSON) + ); + $this->assert204($response); + + $item1JSON = API::getItem($item1JSON['key'], $this, 'json')['data']; + $item2JSON['version'] = $item1JSON['version']; + $response = API::userPut( + self::$config['userID'], + "items/{$item2JSON['key']}", + json_encode($item2JSON) + ); + $t = $response->getBody(); + $this->assert204($response); + + // Remove relation from one of the items + $this->assertCount(1, $item1JSON['relations']); + $item1JSON['relations'] = []; + $item2JSON = API::getItem($item2JSON['key'], $this, 'json')['data']; + $item1JSON['version'] = $item2JSON['version']; + $response = API::userPut( + self::$config['userID'], + "items/{$item1JSON['key']}", + json_encode($item1JSON) + ); + + // Ensure it is gone from item 1 + $this->assert204($response); + $item1JSON = API::getItem($item1JSON['key'], $this, 'json')['data']; + $this->assertCount(0, $item1JSON['relations']); + + // But not yet removed from item 2 + $item2JSON = API::getItem($item2JSON['key'], $this, 'json')['data']; + $this->assertCount(1, $item2JSON['relations']); + } public function test_should_add_a_URL_to_a_relation_with_PATCH() { @@ -254,7 +308,7 @@ public function testCircularItemRelations() { ]; $response = API::postItems([$item1Data, $item2Data]); $this->assert200ForObject($response, false, 0); - $this->assertUnchangedForObject($response, 1); + $this->assert200ForObject($response, false, 1); }