From 81f4186ea81f890769931322f4b36a3f6a0ae68d Mon Sep 17 00:00:00 2001
From: Leszek Manicki <leszek.manicki@wikimedia.de>
Date: Thu, 2 Nov 2017 12:57:09 +0100
Subject: [PATCH] [DNM] Optimize EntityId::getRepositoryName and ::getLocalPart

---
 src/Entity/EntityId.php              | 32 ++++++++++++++++++++++------
 src/Entity/ItemId.php                |  1 +
 src/Entity/PropertyId.php            |  1 +
 tests/fixtures/CustomEntityId.php    |  2 ++
 tests/unit/Entity/ItemIdTest.php     | 16 +++++++++++++-
 tests/unit/Entity/PropertyIdTest.php | 16 +++++++++++++-
 6 files changed, 59 insertions(+), 9 deletions(-)

diff --git a/src/Entity/EntityId.php b/src/Entity/EntityId.php
index 130df856..aa5c9498 100644
--- a/src/Entity/EntityId.php
+++ b/src/Entity/EntityId.php
@@ -16,6 +16,16 @@ abstract class EntityId implements Comparable, Serializable {
 
 	protected $serialization;
 
+	/**
+	 * @var string
+	 */
+	protected $repositoryName;
+
+	/**
+	 * @var string
+	 */
+	protected $localPart;
+
 	const PATTERN = '/^:?(\w+:)*[^:]+\z/';
 
 	/**
@@ -26,6 +36,8 @@ abstract class EntityId implements Comparable, Serializable {
 	public function __construct( $serialization ) {
 		self::assertValidSerialization( $serialization );
 		$this->serialization = self::normalizeIdSerialization( $serialization );
+
+		list ( $this->repositoryName, $this->localPart ) = $this->getRepositoryNameAndLocalPart( $serialization );
 	}
 
 	private static function assertValidSerialization( $serialization ) {
@@ -68,7 +80,11 @@ public function getSerialization() {
 	public static function splitSerialization( $serialization ) {
 		self::assertValidSerialization( $serialization );
 
-		$parts = explode( ':', self::normalizeIdSerialization( $serialization ) );
+		return self::getSerializationParts( self::normalizeIdSerialization( $serialization ) );
+	}
+
+	private static function getSerializationParts( $serialization ) {
+		$parts = explode( ':', $serialization );
 		$localPart = array_pop( $parts );
 		$repoName = array_shift( $parts );
 		$prefixRemainder = implode( ':', $parts );
@@ -112,9 +128,7 @@ public static function joinSerialization( array $parts ) {
 	 * @return string
 	 */
 	public function getRepositoryName() {
-		$parts = self::splitSerialization( $this->serialization );
-
-		return $parts[0];
+		return $this->repositoryName;
 	}
 
 	/**
@@ -125,9 +139,7 @@ public function getRepositoryName() {
 	 * @return string
 	 */
 	public function getLocalPart() {
-		$parts = self::splitSerialization( $this->serialization );
-
-		return self::joinSerialization( [ '', $parts[1], $parts[2] ] );
+		return $this->localPart;
 	}
 
 	/**
@@ -179,4 +191,10 @@ public function equals( $target ) {
 			&& $target->serialization === $this->serialization;
 	}
 
+	protected function getRepositoryNameAndLocalPart( $serialization ) {
+		list( $repoName, $prefixRemainder, $localId ) = self::getSerializationParts( $serialization );
+		$localPart = self::joinSerialization( [ '', $prefixRemainder, $localId ] );
+		return [ $repoName, $localPart ];
+	}
+
 }
diff --git a/src/Entity/ItemId.php b/src/Entity/ItemId.php
index 01a2200d..aa263883 100644
--- a/src/Entity/ItemId.php
+++ b/src/Entity/ItemId.php
@@ -83,6 +83,7 @@ public function serialize() {
 	public function unserialize( $serialized ) {
 		$array = json_decode( $serialized );
 		$this->serialization = is_array( $array ) ? $array[1] : $serialized;
+		list ( $this->repositoryName, $this->localPart ) = $this->getRepositoryNameAndLocalPart( $this->serialization );
 	}
 
 	/**
diff --git a/src/Entity/PropertyId.php b/src/Entity/PropertyId.php
index bd7e3d59..23b65233 100644
--- a/src/Entity/PropertyId.php
+++ b/src/Entity/PropertyId.php
@@ -83,6 +83,7 @@ public function serialize() {
 	public function unserialize( $serialized ) {
 		$array = json_decode( $serialized );
 		$this->serialization = is_array( $array ) ? $array[1] : $serialized;
+		list ( $this->repositoryName, $this->localPart ) = $this->getRepositoryNameAndLocalPart( $this->serialization );
 	}
 
 	/**
diff --git a/tests/fixtures/CustomEntityId.php b/tests/fixtures/CustomEntityId.php
index cea8b2fe..8a256747 100644
--- a/tests/fixtures/CustomEntityId.php
+++ b/tests/fixtures/CustomEntityId.php
@@ -28,6 +28,8 @@ public function serialize() {
 	 */
 	public function unserialize( $serialized ) {
 		$this->serialization = $serialized;
+		$this->repositoryName = '';
+		$this->localPart = $serialized;
 	}
 
 	/**
diff --git a/tests/unit/Entity/ItemIdTest.php b/tests/unit/Entity/ItemIdTest.php
index fd9e6a2c..80aa90ae 100644
--- a/tests/unit/Entity/ItemIdTest.php
+++ b/tests/unit/Entity/ItemIdTest.php
@@ -112,8 +112,22 @@ public function serializationProvider() {
 			// All these cases are kind of an injection vector and allow constructing invalid ids.
 			[ '["string","Q2"]', 'Q2' ],
 			[ '["","string"]', 'string' ],
-			[ '["",""]', '' ],
 			[ '["",2]', 2 ],
+		];
+	}
+
+	/**
+	 * @dataProvider invalidSerializationProvider
+	 */
+	public function testGivenInvalidSerialization_unserializeThrowsException( $serialization ) {
+		$id = new ItemId( 'Q1' );
+		$this->setExpectedException( InvalidArgumentException::class );
+		$id->unserialize( $serialization );
+	}
+
+	public function invalidSerializationProvider() {
+		return [
+			[ '["",""]', '' ],
 			[ '["",null]', null ],
 			[ '', '' ],
 		];
diff --git a/tests/unit/Entity/PropertyIdTest.php b/tests/unit/Entity/PropertyIdTest.php
index 6df7ea4b..4a6a3a19 100644
--- a/tests/unit/Entity/PropertyIdTest.php
+++ b/tests/unit/Entity/PropertyIdTest.php
@@ -112,8 +112,22 @@ public function serializationProvider() {
 			// All these cases are kind of an injection vector and allow constructing invalid ids.
 			[ '["string","P2"]', 'P2' ],
 			[ '["","string"]', 'string' ],
-			[ '["",""]', '' ],
 			[ '["",2]', 2 ],
+		];
+	}
+
+	/**
+	 * @dataProvider invalidSerializationProvider
+	 */
+	public function testGivenInvalidSerialization_unserializeThrowsException( $serialization ) {
+		$id = new PropertyId( 'P1' );
+		$this->setExpectedException( InvalidArgumentException::class );
+		$id->unserialize( $serialization );
+	}
+
+	public function invalidSerializationProvider() {
+		return [
+			[ '["",""]', '' ],
 			[ '["",null]', null ],
 			[ '', '' ],
 		];