From 9d524b9f53d410dbb3fcefe706fcccdc65695bb4 Mon Sep 17 00:00:00 2001 From: Thomas Ploch Date: Thu, 26 Oct 2017 09:20:26 +0200 Subject: [PATCH 1/2] [FEATURE][BC-BREAK] Add possibility to cache schema IDs by hash. This is needed because when running serializers the cost of hashing the schemas' string representations is way less than going to the registry over HTTP. This adds 3 new methods to the `CacheAdapter` API: `cacheSchemaIdByHash(int $schemaId, string $schemaHash)` `getIdWithHash(string $hash)` `hasSchemaIdForHash(string $schemaHash)` Unfortunately this is a BC breaking change which requires clients that possibly have own implementations of the `CacheAdapterInterface` to implement those 3 methods. Clients that use the default shipped adapters are OK. --- src/Registry/Cache/AvroObjectCacheAdapter.php | 30 ++++ src/Registry/Cache/DoctrineCacheAdapter.php | 27 ++++ src/Registry/CacheAdapter.php | 29 ++++ .../Cache/AbstractCacheAdapterTestCase.php | 82 +++++++++++ .../Cache/AvroObjectCacheAdapterTest.php | 51 +------ .../Cache/DoctrineCacheAdapterTest.php | 128 +----------------- 6 files changed, 177 insertions(+), 170 deletions(-) create mode 100644 test/Registry/Cache/AbstractCacheAdapterTestCase.php diff --git a/src/Registry/Cache/AvroObjectCacheAdapter.php b/src/Registry/Cache/AvroObjectCacheAdapter.php index 1ffab53..0aec240 100644 --- a/src/Registry/Cache/AvroObjectCacheAdapter.php +++ b/src/Registry/Cache/AvroObjectCacheAdapter.php @@ -17,6 +17,11 @@ class AvroObjectCacheAdapter implements CacheAdapter */ private $idToSchema = []; + /** + * @var int[] + */ + private $hashToSchemaId = []; + /** * @var AvroSchema[] */ @@ -30,6 +35,14 @@ public function cacheSchemaWithId(AvroSchema $schema, int $schemaId) $this->idToSchema[$schemaId] = $schema; } + /** + * {@inheritdoc} + */ + public function cacheSchemaIdByHash(int $schemaId, string $schemaHash) + { + $this->hashToSchemaId[$schemaHash] = $schemaId; + } + /** * {@inheritdoc} */ @@ -50,6 +63,15 @@ public function getWithId(int $schemaId) return $this->idToSchema[$schemaId]; } + public function getIdWithHash(string $hash) + { + if (!$this->hasSchemaIdForHash($hash)) { + return null; + } + + return $this->hashToSchemaId[$hash]; + } + /** * {@inheritdoc} */ @@ -72,6 +94,14 @@ public function hasSchemaForId(int $schemaId): bool return array_key_exists($schemaId, $this->idToSchema); } + /** + * {@inheritdoc} + */ + public function hasSchemaIdForHash(string $schemaHash): bool + { + return array_key_exists($schemaHash, $this->hashToSchemaId); + } + /** * {@inheritdoc} */ diff --git a/src/Registry/Cache/DoctrineCacheAdapter.php b/src/Registry/Cache/DoctrineCacheAdapter.php index be0fef6..6c10a88 100644 --- a/src/Registry/Cache/DoctrineCacheAdapter.php +++ b/src/Registry/Cache/DoctrineCacheAdapter.php @@ -31,6 +31,11 @@ public function cacheSchemaWithId(AvroSchema $schema, int $schemaId) $this->doctrineCache->save($schemaId, (string) $schema); } + public function cacheSchemaIdByHash(int $schemaId, string $schemaHash) + { + $this->doctrineCache->save($schemaHash, $schemaId); + } + /** * {@inheritdoc} */ @@ -56,6 +61,20 @@ public function getWithId(int $schemaId) return AvroSchema::parse($rawSchema); } + /** + * {@inheritdoc} + */ + public function getIdWithHash(string $hash) + { + $schemaId = $this->doctrineCache->fetch($hash); + + if (!$schemaId) { + return null; + } + + return $schemaId; + } + /** * {@inheritdoc} */ @@ -82,6 +101,14 @@ public function hasSchemaForId(int $schemaId): bool return $this->doctrineCache->contains($schemaId); } + /** + * {@inheritdoc} + */ + public function hasSchemaIdForHash(string $schemaHash): bool + { + return $this->doctrineCache->contains($schemaHash); + } + /** * {@inheritdoc} */ diff --git a/src/Registry/CacheAdapter.php b/src/Registry/CacheAdapter.php index c7cdee9..89dab0d 100644 --- a/src/Registry/CacheAdapter.php +++ b/src/Registry/CacheAdapter.php @@ -32,6 +32,16 @@ public function cacheSchemaWithId(AvroSchema $schema, int $schemaId); */ public function cacheSchemaWithSubjectAndVersion(AvroSchema $schema, string $subject, int $version); + /** + * Caches a schema id by a hash (i.e. the hash of the Avro schema string representation) + * + * @param int $schemaId + * @param string $schemaHash + * + * @return void + */ + public function cacheSchemaIdByHash(int $schemaId, string $schemaHash); + /** * Tries to fetch a cache with the global schema id. * Returns either the AvroSchema when found or `null` when not. @@ -42,6 +52,16 @@ public function cacheSchemaWithSubjectAndVersion(AvroSchema $schema, string $sub */ public function getWithId(int $schemaId); + /** + * Tries to fetch a cached schema id with a given hash. + * Either returns the schema id as int or `null` when none is found. + * + * @param string $hash + * + * @return int|null + */ + public function getIdWithHash(string $hash); + /** * Tries to fetch a cache with a given subject and version. * Returns either the AvroSchema when found or `null` when not. @@ -62,6 +82,15 @@ public function getWithSubjectAndVersion(string $subject, int $version); */ public function hasSchemaForId(int $schemaId): bool; + /** + * Checks if a schema id exists for the given hash + * + * @param string $schemaHash + * + * @return bool + */ + public function hasSchemaIdForHash(string $schemaHash): bool; + /** * Checks if the cache engine has a cached schema for a given subject and version. * diff --git a/test/Registry/Cache/AbstractCacheAdapterTestCase.php b/test/Registry/Cache/AbstractCacheAdapterTestCase.php new file mode 100644 index 0000000..1aeeba2 --- /dev/null +++ b/test/Registry/Cache/AbstractCacheAdapterTestCase.php @@ -0,0 +1,82 @@ +cacheAdapter = $this->getAdapter(); + } + + /** + * @test + */ + public function it_should_store_and_fetch_schemas_with_ids() + { + $schemaId = 3; + $invalidSchemaId = 2; + $schema = AvroSchema::parse('{"type": "string"}'); + + $this->cacheAdapter->cacheSchemaWithId($schema, $schemaId); + + $this->assertFalse($this->cacheAdapter->hasSchemaForId($invalidSchemaId)); + $this->assertTrue($this->cacheAdapter->hasSchemaForId($schemaId)); + + $this->assertNull($this->cacheAdapter->getWithId($invalidSchemaId)); + $this->assertEquals($schema, $this->cacheAdapter->getWithId($schemaId)); + } + + /** + * @test + */ + public function it_should_store_and_fetch_schemas_with_subject_and_version() + { + $subject = 'test'; + $version = 2; + $invalidSubject = 'none'; + $schema = AvroSchema::parse('{"type": "string"}'); + + $this->cacheAdapter->cacheSchemaWithSubjectAndVersion($schema, $subject, $version); + + $this->assertFalse($this->cacheAdapter->hasSchemaForSubjectAndVersion($invalidSubject, $version)); + $this->assertTrue($this->cacheAdapter->hasSchemaForSubjectAndVersion($subject, $version)); + + $this->assertNull($this->cacheAdapter->getWithSubjectAndVersion($invalidSubject, $version)); + $this->assertEquals($schema, $this->cacheAdapter->getWithSubjectAndVersion($subject, $version)); + } + + /** + * @test + */ + public function it_should_store_and_fetch_schema_ids_with_schema_hashes() + { + $schemaId = 3; + $hash = 'hash'; + $anotherHash = 'another'; + + $this->assertFalse($this->cacheAdapter->hasSchemaIdForHash($hash)); + $this->assertFalse($this->cacheAdapter->hasSchemaIdForHash($anotherHash)); + + $this->cacheAdapter->cacheSchemaIdByHash($schemaId, $hash); + + $this->assertTrue($this->cacheAdapter->hasSchemaIdForHash($hash)); + $this->assertFalse($this->cacheAdapter->hasSchemaIdForHash($anotherHash)); + + $this->assertNull($this->cacheAdapter->getIdWithHash($anotherHash)); + $this->assertSame($schemaId, $this->cacheAdapter->getIdWithHash($hash)); + } +} diff --git a/test/Registry/Cache/AvroObjectCacheAdapterTest.php b/test/Registry/Cache/AvroObjectCacheAdapterTest.php index 253e966..91e6dc3 100644 --- a/test/Registry/Cache/AvroObjectCacheAdapterTest.php +++ b/test/Registry/Cache/AvroObjectCacheAdapterTest.php @@ -4,56 +4,13 @@ namespace FlixTech\SchemaRegistryApi\Test\Registry\Cache; -use AvroSchema; use FlixTech\SchemaRegistryApi\Registry\Cache\AvroObjectCacheAdapter; -use PHPUnit\Framework\TestCase; +use FlixTech\SchemaRegistryApi\Registry\CacheAdapter; -class AvroObjectCacheAdapterTest extends TestCase +class AvroObjectCacheAdapterTest extends AbstractCacheAdapterTestCase { - /** - * @var \FlixTech\SchemaRegistryApi\Registry\Cache\AvroObjectCacheAdapter - */ - private $cacheAdapter; - - protected function setUp() - { - $this->cacheAdapter = new AvroObjectCacheAdapter(); - } - - /** - * @test - */ - public function it_should_store_and_fetch_schemas_with_ids() + protected function getAdapter(): CacheAdapter { - $schemaId = 3; - $invalidSchemaId = 2; - $schema = AvroSchema::parse('{"type": "string"}'); - - $this->cacheAdapter->cacheSchemaWithId($schema, $schemaId); - - $this->assertFalse($this->cacheAdapter->hasSchemaForId($invalidSchemaId)); - $this->assertTrue($this->cacheAdapter->hasSchemaForId($schemaId)); - - $this->assertNull($this->cacheAdapter->getWithId($invalidSchemaId)); - $this->assertEquals($schema, $this->cacheAdapter->getWithId($schemaId)); - } - - /** - * @test - */ - public function it_should_store_and_fetch_schemas_with_subject_and_version() - { - $subject = 'test'; - $version = 2; - $invalidSubject = 'none'; - $schema = AvroSchema::parse('{"type": "string"}'); - - $this->cacheAdapter->cacheSchemaWithSubjectAndVersion($schema, $subject, $version); - - $this->assertFalse($this->cacheAdapter->hasSchemaForSubjectAndVersion($invalidSubject, $version)); - $this->assertTrue($this->cacheAdapter->hasSchemaForSubjectAndVersion($subject, $version)); - - $this->assertNull($this->cacheAdapter->getWithSubjectAndVersion($invalidSubject, $version)); - $this->assertEquals($schema, $this->cacheAdapter->getWithSubjectAndVersion($subject, $version)); + return new AvroObjectCacheAdapter(); } } diff --git a/test/Registry/Cache/DoctrineCacheAdapterTest.php b/test/Registry/Cache/DoctrineCacheAdapterTest.php index 5d9e1ca..541f944 100644 --- a/test/Registry/Cache/DoctrineCacheAdapterTest.php +++ b/test/Registry/Cache/DoctrineCacheAdapterTest.php @@ -4,132 +4,14 @@ namespace FlixTech\SchemaRegistryApi\Test\Registry\Cache; -use AvroSchema; -use Doctrine\Common\Cache\Cache; +use Doctrine\Common\Cache\ArrayCache; use FlixTech\SchemaRegistryApi\Registry\Cache\DoctrineCacheAdapter; -use PHPUnit\Framework\TestCase; +use FlixTech\SchemaRegistryApi\Registry\CacheAdapter; -class DoctrineCacheAdapterTest extends TestCase +class DoctrineCacheAdapterTest extends AbstractCacheAdapterTestCase { - /** - * @var Cache|\PHPUnit_Framework_MockObject_MockObject - */ - private $doctrineCache; - - /** - * @var DoctrineCacheAdapter - */ - private $cacheAdapter; - - protected function setUp() - { - $this->doctrineCache = $this->getMockForAbstractClass(Cache::class); - $this->cacheAdapter = new DoctrineCacheAdapter($this->doctrineCache); - } - - /** - * @test - */ - public function it_should_store_schemas_with_ids() - { - $schemaId = 3; - $schema = AvroSchema::parse('{"type": "string"}'); - - $this->doctrineCache - ->expects($this->once()) - ->method('save') - ->with(3, (string) $schema); - - $this->cacheAdapter->cacheSchemaWithId($schema, $schemaId); - } - - /** - * @test - */ - public function it_should_fetch_schemas_with_id() - { - $schemaId = 3; - $schema = AvroSchema::parse('{"type": "string"}'); - - $this->doctrineCache - ->expects($this->exactly(2)) - ->method('fetch') - ->with($schemaId) - ->willReturnOnConsecutiveCalls((string) $schema, null); - - $this->assertEquals($schema, $this->cacheAdapter->getWithId($schemaId)); - $this->assertNull($this->cacheAdapter->getWithId($schemaId)); - } - - /** - * @test - */ - public function it_can_check_if_a_schema_exists_for_id() - { - $schemaId = 3; - - $this->doctrineCache - ->expects($this->once()) - ->method('contains') - ->with($schemaId) - ->willReturn(true); - - $this->assertTrue($this->cacheAdapter->hasSchemaForId($schemaId)); - } - - /** - * @test - */ - public function it_can_cache_schemas_by_subject_and_version() - { - $version = 3; - $subject = 'test'; - $schema = AvroSchema::parse('{"type": "string"}'); - - $this->doctrineCache - ->expects($this->once()) - ->method('save') - ->with($subject . '_' . $version, (string) $schema); - - $this->cacheAdapter->cacheSchemaWithSubjectAndVersion($schema, $subject, $version); - } - - /** - * @test - */ - public function it_can_fetch_schemas_by_subject_and_version() - { - $version = 3; - $subject = 'test'; - $schema = AvroSchema::parse('{"type": "string"}'); - - $this->doctrineCache - ->expects($this->exactly(2)) - ->method('fetch') - ->with($subject . '_' . $version) - ->willReturnOnConsecutiveCalls($schema, null); - - $this->assertEquals( - $schema, - $this->cacheAdapter->getWithSubjectAndVersion($subject, $version) - ); - $this->assertNull($this->cacheAdapter->getWithSubjectAndVersion($subject, $version)); - } - - /** - * @test - */ - public function it_can_check_if_a_schema_exists_for_subject_and_version() + protected function getAdapter(): CacheAdapter { - $version = 3; - $subject = 'test'; - - $this->doctrineCache - ->expects($this->once()) - ->method('contains') - ->with($subject . '_' . $version) - ->willReturn(true); - - $this->assertTrue($this->cacheAdapter->hasSchemaForSubjectAndVersion($subject, $version)); + return new DoctrineCacheAdapter(new ArrayCache()); } } From 6fb5fc398e3b25b740f6870a839a919250e1cacb Mon Sep 17 00:00:00 2001 From: Thomas Ploch Date: Thu, 26 Oct 2017 09:20:57 +0200 Subject: [PATCH 2/2] Implement the new caching features in the `CachedRegistry` --- src/Registry/CachedRegistry.php | 31 +++++++- test/Registry/CachedRegistryTest.php | 106 +++++++++++++++++++++++++++ 2 files changed, 135 insertions(+), 2 deletions(-) diff --git a/src/Registry/CachedRegistry.php b/src/Registry/CachedRegistry.php index 977f3f9..40c9acd 100644 --- a/src/Registry/CachedRegistry.php +++ b/src/Registry/CachedRegistry.php @@ -23,10 +23,23 @@ class CachedRegistry implements Registry */ private $cacheAdapter; - public function __construct(Registry $registry, CacheAdapter $cacheAdapter) + /** + * @var callable + */ + private $hashAlgoFunc; + + public function __construct(Registry $registry, CacheAdapter $cacheAdapter, callable $hashAlgoFunc = null) { $this->registry = $registry; $this->cacheAdapter = $cacheAdapter; + + if (!$hashAlgoFunc) { + $hashAlgoFunc = function (AvroSchema $schema) { + return md5((string) $schema); + }; + } + + $this->hashAlgoFunc = $hashAlgoFunc; } /** @@ -36,6 +49,7 @@ public function register(string $subject, AvroSchema $schema, callable $requestC { $closure = function (int $schemaId) use ($schema) { $this->cacheAdapter->cacheSchemaWithId($schema, $schemaId); + $this->cacheAdapter->cacheSchemaIdByHash($schemaId, $this->getSchemaHash($schema)); return $schemaId; }; @@ -74,8 +88,15 @@ function (PromiseInterface $promise) use ($closure) { */ public function schemaId(string $subject, AvroSchema $schema, callable $requestCallback = null) { - $closure = function (int $schemaId) use ($schema) { + $schemaHash = $this->getSchemaHash($schema); + + if ($this->cacheAdapter->hasSchemaIdForHash($schemaHash)) { + return $this->cacheAdapter->getIdWithHash($schemaHash); + } + + $closure = function (int $schemaId) use ($schema, $schemaHash) { $this->cacheAdapter->cacheSchemaWithId($schema, $schemaId); + $this->cacheAdapter->cacheSchemaIdByHash($schemaId, $schemaHash); return $schemaId; }; @@ -100,6 +121,7 @@ public function schemaForId(int $schemaId, callable $requestCallback = null) $closure = function (AvroSchema $schema) use ($schemaId) { $this->cacheAdapter->cacheSchemaWithId($schema, $schemaId); + $this->cacheAdapter->cacheSchemaIdByHash($schemaId, $this->getSchemaHash($schema)); return $schema; }; @@ -159,4 +181,9 @@ private function applyValueHandlers($value, callable $promiseHandler, callable $ return $valueHandler($value); } + + private function getSchemaHash(AvroSchema $schema): string + { + return call_user_func($this->hashAlgoFunc, $schema); + } } diff --git a/test/Registry/CachedRegistryTest.php b/test/Registry/CachedRegistryTest.php index 2a7c3a9..7660476 100644 --- a/test/Registry/CachedRegistryTest.php +++ b/test/Registry/CachedRegistryTest.php @@ -39,12 +39,21 @@ class CachedRegistryTest extends TestCase */ private $schema; + /** + * @var callable + */ + private $hashFunction; + protected function setUp() { $this->schema = AvroSchema::parse('{"type": "string"}'); $this->registryMock = $this->getMockForAbstractClass(Registry::class); $this->cacheAdapter = $this->getMockForAbstractClass(CacheAdapter::class); + $this->hashFunction = function (AvroSchema $schema) { + return md5((string) $schema); + }; + $this->cachedRegistry = new CachedRegistry($this->registryMock, $this->cacheAdapter); } @@ -66,6 +75,11 @@ public function it_should_cache_from_register_responses() ->method('cacheSchemaWithId') ->with($this->schema, 4); + $this->cacheAdapter + ->expects($this->exactly(2)) + ->method('cacheSchemaIdByHash') + ->with(4, call_user_func($this->hashFunction, $this->schema)); + /** @var PromiseInterface $promise */ $promise = $this->cachedRegistry->register($this->subject, $this->schema); @@ -122,6 +136,11 @@ public function it_should_cache_from_schema_id_responses() ->method('cacheSchemaWithId') ->with($this->schema, 1); + $this->cacheAdapter + ->expects($this->exactly(2)) + ->method('cacheSchemaIdByHash') + ->with(1, call_user_func($this->hashFunction, $this->schema)); + /** @var PromiseInterface $promise */ $promise = $this->cachedRegistry->schemaId($this->subject, $this->schema); @@ -132,6 +151,93 @@ public function it_should_cache_from_schema_id_responses() $this->assertEquals(1, $version); } + /** + * @test + */ + public function it_should_return_schema_id_from_the_cache_for_schema_hash() + { + $this->registryMock + ->expects($this->never()) + ->method('schemaId'); + + $this->cacheAdapter + ->expects($this->once()) + ->method('hasSchemaIdForHash') + ->with(call_user_func($this->hashFunction, $this->schema)) + ->willReturn(true); + + $this->cacheAdapter + ->expects($this->once()) + ->method('getIdWithHash') + ->with(call_user_func($this->hashFunction, $this->schema)) + ->willReturn(3); + + $this->assertEquals(3, $this->cachedRegistry->schemaId($this->subject, $this->schema)); + } + + /** + * @test + */ + public function it_should_cache_schema_id_for_hash_if_cache_is_stale() + { + $promise = new FulfilledPromise(3); + + $this->registryMock + ->expects($this->exactly(2)) + ->method('schemaId') + ->with($this->subject, $this->schema) + ->willReturnOnConsecutiveCalls($promise, 3); + + $this->cacheAdapter + ->expects($this->exactly(2)) + ->method('hasSchemaIdForHash') + ->with(call_user_func($this->hashFunction, $this->schema)) + ->willReturn(false); + + $this->cacheAdapter + ->expects($this->never()) + ->method('getIdWithHash'); + + /** @var PromiseInterface $promise */ + $promise = $this->cachedRegistry->schemaId($this->subject, $this->schema); + + $this->assertInstanceOf(PromiseInterface::class, $promise); + $this->assertEquals(3, $promise->wait()); + + $id = $this->cachedRegistry->schemaId($this->subject, $this->schema); + $this->assertEquals(3, $id); + } + + /** + * @test + */ + public function it_should_accept_different_hash_algo_functions() + { + $sha1HashFunction = function (AvroSchema $schema) { + return sha1((string) $schema); + }; + + $this->cachedRegistry = new CachedRegistry($this->registryMock, $this->cacheAdapter, $sha1HashFunction); + + $this->registryMock + ->expects($this->never()) + ->method('schemaId'); + + $this->cacheAdapter + ->expects($this->once()) + ->method('hasSchemaIdForHash') + ->with($sha1HashFunction($this->schema)) + ->willReturn(true); + + $this->cacheAdapter + ->expects($this->once()) + ->method('getIdWithHash') + ->with($sha1HashFunction($this->schema)) + ->willReturn(3); + + $this->cachedRegistry->schemaId($this->subject, $this->schema); + } + /** * @test */