From 8b59f12c326a6e3f25ecb5eeae5fddfab0d682f8 Mon Sep 17 00:00:00 2001 From: Remo Laubacher Date: Thu, 2 Apr 2026 14:44:24 +0200 Subject: [PATCH 1/3] perf: cache SQL schema descriptors and add operation discovery fast-path Item 1 (SQLSchema): Replace the old two-step approach (cache raw DBAL Table, re-process columns every request) with a fully cached pipeline. The new buildPropertyDescriptors() stores a serializable array of type names, nullability, and default values. discoverProperties() hydrates these into OData DeclaredProperty objects without touching the database again on cache hits. Item 5 (Operation): Add an early-exit in Operation::discover() that checks for any LodataOperation attributes before doing the full per-method reflection scan. Models without operations (the common case) return immediately. --- src/Drivers/SQL/SQLSchema.php | 193 ++++++++++++++++++++++++++++------ src/Operation.php | 16 ++- tests/Setup/DiscoveryTest.php | 10 +- 3 files changed, 181 insertions(+), 38 deletions(-) diff --git a/src/Drivers/SQL/SQLSchema.php b/src/Drivers/SQL/SQLSchema.php index d44a9062d..3f3a1e2ef 100644 --- a/src/Drivers/SQL/SQLSchema.php +++ b/src/Drivers/SQL/SQLSchema.php @@ -25,26 +25,72 @@ trait SQLSchema { /** - * Discover SQL fields on this entity set as OData properties + * Discover SQL fields on this entity set as OData properties. + * Uses Discovery::remember() to cache the full processed property descriptors + * (not just the raw DBAL Table), avoiding per-request column-to-property conversion. * @return $this */ public function discoverProperties() { - $table = (new Discovery)->remember( - sprintf("sql.%s.%s", $this->getConnection()->getName(), $this->getTable()), - function () { - return $this->getDatabase()->listTableDetails($this->getTable()); + $cacheKey = sprintf("sql.properties.%s.%s", $this->getConnection()->getName(), $this->getTable()); + + $propertyDescriptors = (new Discovery)->remember($cacheKey, function () { + return $this->buildPropertyDescriptors(); + }); + + $type = $this->getType(); + + // Hydrate cached descriptors into actual OData DeclaredProperty objects + if (isset($propertyDescriptors['key'])) { + $keyDesc = $propertyDescriptors['key']; + $key = new DeclaredProperty($keyDesc['name'], $this->resolveType($keyDesc['type'])); + if ($keyDesc['computed']) { + $key->addAnnotation(new Computed); + } + $type->setKey($key); + } + + foreach ($propertyDescriptors['properties'] as $propDesc) { + $property = new DeclaredProperty($propDesc['name'], $this->resolveType($propDesc['type'])); + $property->setNullable($propDesc['nullable']); + + if ($propDesc['has_default']) { + $property->addAnnotation(new ComputedDefaultValue); + if ($propDesc['default_value'] !== null) { + $property->setDefaultValue($propDesc['default_value']); + } + if ($propDesc['default_is_carbon_now'] ?? false) { + $property->setDefaultValue([Carbon::class, 'now']); + } } - ); + + if (isset($propDesc['source_name'])) { + $this->setPropertySourceName($property, $propDesc['source_name']); + } + + $type->addProperty($property); + } + + return $this; + } + + /** + * Build a serializable array of property descriptors from the database schema. + * This is the expensive part that we cache. + * @return array + */ + protected function buildPropertyDescriptors(): array + { + $table = $this->getDatabase()->listTableDetails($this->getTable()); $columns = $table->getColumns(); $indexes = $table->getIndexes(); + $blacklist = config('lodata.discovery.blacklist', []); + $platform = $this->getDatabase()->getDatabasePlatform(); - $type = $this->getType(); - - /** @var DeclaredProperty $key */ - $key = null; + $result = ['key' => null, 'properties' => []]; + // Find primary key foreach ($indexes as $index) { if (!$index->isPrimary()) { continue; @@ -59,71 +105,152 @@ function () { continue; } - $key = $this->columnToDeclaredProperty($column); - - if (null === $key) { + $typeName = $this->columnToTypeName($column); + if ($typeName === null) { throw new ConfigurationException( 'missing_key', sprintf('The table %s had no resolvable key', $this->getTable()) ); } - if ($column->getAutoincrement()) { - $key->addAnnotation(new Computed); - } - - $type->setKey($key); + $result['key'] = [ + 'name' => $this->resolveColumnName($column), + 'type' => $typeName, + 'computed' => $column->getAutoincrement(), + ]; } - $blacklist = config('lodata.discovery.blacklist', []); - $platform = $this->getDatabase()->getDatabasePlatform(); + // Process remaining columns + $keyName = $result['key'] ? $result['key']['name'] : null; foreach ($columns as $column) { - $columnName = $column->getName(); + $columnName = $this->resolveColumnName($column); - if ($key && $columnName === $key->getName()) { + if ($keyName && $columnName === $keyName) { continue; } - if (in_array($columnName, $blacklist)) { + if (in_array($column->getName(), $blacklist)) { continue; } - $property = $this->columnToDeclaredProperty($column); - - if (null === $property) { + $typeName = $this->columnToTypeName($column); + if ($typeName === null) { continue; } - $property->setNullable(!$column->getNotnull()); + $propDesc = [ + 'name' => $columnName, + 'type' => $typeName, + 'nullable' => !$column->getNotnull(), + 'has_default' => (bool) $column->getDefault(), + 'default_value' => null, + 'default_is_carbon_now' => false, + ]; + + // Track source name if it differs from the resolved column name + if ($columnName !== $column->getName()) { + $propDesc['source_name'] = $column->getName(); + } if ($column->getDefault()) { - $property->addAnnotation(new ComputedDefaultValue); $default = $column->getDefault(); switch (true) { // DBAL 4.x returns DefaultExpression objects instead of strings case !is_string($default): - $property->setDefaultValue([Carbon::class, 'now']); + $propDesc['default_is_carbon_now'] = true; break; case $default === $platform->getCurrentTimestampSQL(): - $property->setDefaultValue([Carbon::class, 'now']); + $propDesc['default_is_carbon_now'] = true; break; case $platform->getReservedKeywordsList()->isKeyword($default): break; default: - $property->setDefaultValue($default); + $propDesc['default_value'] = $default; break; } } - $type->addProperty($property); + $result['properties'][] = $propDesc; } - return $this; + return $result; + } + + /** + * Map a DBAL column to an OData type name string (for caching). + * @param Column $column + * @return string|null + */ + protected function columnToTypeName(Column $column): ?string + { + $columnType = $column->getType(); + + switch (true) { + case $columnType instanceof Types\BooleanType: + return 'boolean'; + case $columnType instanceof Types\DateType: + return 'date'; + case $columnType instanceof Types\DateTimeType: + return 'datetimeoffset'; + case $columnType instanceof Types\DecimalType: + case $columnType instanceof Types\FloatType: + return 'decimal'; + case $columnType instanceof Types\SmallIntType: + return $column->getUnsigned() ? 'uint16' : 'int16'; + case $columnType instanceof Types\IntegerType: + return $column->getUnsigned() ? 'uint32' : 'int32'; + case $columnType instanceof Types\BigIntType: + return $column->getUnsigned() ? 'uint64' : 'int64'; + case $columnType instanceof Types\TimeType: + return 'timeofday'; + case $columnType instanceof Types\StringType: + default: + return 'string'; + } + } + + /** + * Resolve a type name string back to an OData Type instance. + * @param string $typeName + * @return Type + */ + protected function resolveType(string $typeName): Type + { + switch ($typeName) { + case 'boolean': return Type::boolean(); + case 'date': return Type::date(); + case 'datetimeoffset': return Type::datetimeoffset(); + case 'decimal': return Type::decimal(); + case 'uint16': + return Lodata::getTypeDefinition(Type\UInt16::identifier) ? Type::uint16() : Type::int16(); + case 'int16': return Type::int16(); + case 'uint32': + return Lodata::getTypeDefinition(Type\UInt32::identifier) ? Type::uint32() : Type::int32(); + case 'int32': return Type::int32(); + case 'uint64': + return Lodata::getTypeDefinition(Type\UInt64::identifier) ? Type::uint64() : Type::int64(); + case 'int64': return Type::int64(); + case 'timeofday': return Type::timeofday(); + case 'string': + default: return Type::string(); + } + } + + /** + * Resolve the name of a column, accounting for namespaced columns. + * @param Column $column + * @return string + */ + protected function resolveColumnName(Column $column): string + { + return $column->getNamespaceName() + ? ($column->getNamespaceName().'_'.$column->getShortestName($column->getNamespaceName())) + : $column->getName(); } /** diff --git a/src/Operation.php b/src/Operation.php index 89b106997..e786b4707 100644 --- a/src/Operation.php +++ b/src/Operation.php @@ -809,9 +809,23 @@ public static function discover($discoverable) return; } - $namespace = null; $reflectionClass = new ReflectionClass($discoverable); + // Fast path: skip full discovery if no methods have LodataOperation attributes. + // This avoids expensive reflection loops for Eloquent models that don't define operations. + $hasAnyOperations = false; + foreach ($reflectionClass->getMethods(ReflectionMethod::IS_PUBLIC) as $method) { + if ($method->getAttributes(LodataOperation::class, ReflectionAttribute::IS_INSTANCEOF)) { + $hasAnyOperations = true; + break; + } + } + if (!$hasAnyOperations) { + return; + } + + $namespace = null; + /** @var ReflectionAttribute $namespaceAttribute */ $namespaceAttribute = Arr::first($reflectionClass->getAttributes(LodataNamespace::class)); if ($namespaceAttribute) { diff --git a/tests/Setup/DiscoveryTest.php b/tests/Setup/DiscoveryTest.php index 3b0ac548d..15454a06e 100644 --- a/tests/Setup/DiscoveryTest.php +++ b/tests/Setup/DiscoveryTest.php @@ -4,7 +4,6 @@ namespace Flat3\Lodata\Tests\Setup; -use Doctrine\DBAL\Schema\Table; use Flat3\Lodata\Drivers\MongoEntitySet; use Flat3\Lodata\Drivers\MongoEntityType; use Flat3\Lodata\Facades\Lodata; @@ -62,9 +61,12 @@ public function test_cache_null() ]); Lodata::discoverEloquentModel(Airport::class); - $table = Cache::store('redis')->get('lodata.discovery.sql.testing.airports'); - $this->assertInstanceOf(Table::class, $table); - $this->assertEquals(-2, Cache::store('redis')->getRedis()->ttl('lodata.discovery.sql.testing.airports')); + $cacheKey = 'lodata.discovery.sql.properties.testing.airports'; + $cached = Cache::store('redis')->get($cacheKey); + $this->assertIsArray($cached); + $this->assertArrayHasKey('key', $cached); + $this->assertArrayHasKey('properties', $cached); + $this->assertEquals(-2, Cache::store('redis')->getRedis()->ttl($cacheKey)); } public function test_default_config() From f97e9215b7819e610bb6b5189706dee6f2a0a1be Mon Sep 17 00:00:00 2001 From: Remo Laubacher Date: Thu, 7 May 2026 11:46:54 +0200 Subject: [PATCH 2/3] fix: keep legacy discovery flow in EloquentEntitySet MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The descriptor cache in SQLSchema bypasses columnToDeclaredProperty(), but EloquentEntitySet overrides that method to apply hidden/visible filters and Eloquent cast type overrides (e.g. 'array' → collection). Without that hook, array-cast columns reach String_::set() and trip "Array to string conversion". Override discoverProperties() in EloquentEntitySet with the upstream per-column flow so the cast and visibility logic runs. Pure SQLEntitySet still uses the new descriptor cache. Revert the redis discovery test to expect the legacy Table-cache key for Eloquent. --- src/Drivers/EloquentEntitySet.php | 106 ++++++++++++++++++++++++++++++ tests/Setup/DiscoveryTest.php | 10 ++- 2 files changed, 110 insertions(+), 6 deletions(-) diff --git a/src/Drivers/EloquentEntitySet.php b/src/Drivers/EloquentEntitySet.php index 72fcdd0c0..bcc657532 100644 --- a/src/Drivers/EloquentEntitySet.php +++ b/src/Drivers/EloquentEntitySet.php @@ -4,10 +4,12 @@ namespace Flat3\Lodata\Drivers; +use Carbon\Carbon; use Doctrine\DBAL\Schema\Column; use Exception; use Flat3\Lodata\Annotation\Capabilities\V1\DeepInsertSupport; use Flat3\Lodata\Annotation\Core\V1\Computed; +use Flat3\Lodata\Annotation\Core\V1\ComputedDefaultValue; use Flat3\Lodata\Annotation\Core\V1\Description; use Flat3\Lodata\Attributes\LodataIdentifier; use Flat3\Lodata\Attributes\LodataProperty; @@ -65,6 +67,7 @@ use Illuminate\Database\Eloquent\Relations\HasOneOrMany; use Illuminate\Database\Eloquent\Relations\HasOneThrough; use Illuminate\Database\Eloquent\Relations\Relation; +use Illuminate\Support\Arr; use Illuminate\Support\Collection; use Illuminate\Support\Facades\App; use Illuminate\Support\Facades\DB; @@ -821,6 +824,109 @@ public function columnToDeclaredProperty(Column $column): ?DeclaredProperty return $property; } + /** + * Eloquent's columnToDeclaredProperty applies hidden/visible filtering and + * cast-based type overrides. The descriptor cache in SQLSchema bypasses + * that hook, so use the legacy per-column flow here to preserve casts. + */ + public function discoverProperties() + { + $table = (new Discovery)->remember( + sprintf("sql.%s.%s", $this->getConnection()->getName(), $this->getTable()), + function () { + return $this->getDatabase()->listTableDetails($this->getTable()); + } + ); + + $columns = $table->getColumns(); + $indexes = $table->getIndexes(); + + $type = $this->getType(); + + /** @var DeclaredProperty $key */ + $key = null; + + foreach ($indexes as $index) { + if (!$index->isPrimary()) { + continue; + } + + /** @var Column $column */ + $column = Arr::first($columns, function (Column $column) use ($index) { + return $column->getName() === $index->getColumns()[0]; + }); + + if (!$column) { + continue; + } + + $key = $this->columnToDeclaredProperty($column); + + if (null === $key) { + throw new ConfigurationException( + 'missing_key', + sprintf('The table %s had no resolvable key', $this->getTable()) + ); + } + + if ($column->getAutoincrement()) { + $key->addAnnotation(new Computed); + } + + $type->setKey($key); + } + + $blacklist = config('lodata.discovery.blacklist', []); + $platform = $this->getDatabase()->getDatabasePlatform(); + + foreach ($columns as $column) { + $columnName = $column->getName(); + + if ($key && $columnName === $key->getName()) { + continue; + } + + if (in_array($columnName, $blacklist)) { + continue; + } + + $property = $this->columnToDeclaredProperty($column); + + if (null === $property) { + continue; + } + + $property->setNullable(!$column->getNotnull()); + + if ($column->getDefault()) { + $property->addAnnotation(new ComputedDefaultValue); + $default = $column->getDefault(); + + switch (true) { + // DBAL 4.x returns DefaultExpression objects instead of strings + case !is_string($default): + $property->setDefaultValue([Carbon::class, 'now']); + break; + + case $default === $platform->getCurrentTimestampSQL(): + $property->setDefaultValue([Carbon::class, 'now']); + break; + + case $platform->getReservedKeywordsList()->isKeyword($default): + break; + + default: + $property->setDefaultValue($default); + break; + } + } + + $type->addProperty($property); + } + + return $this; + } + /** * Discover elements on this entity set model * @return $this diff --git a/tests/Setup/DiscoveryTest.php b/tests/Setup/DiscoveryTest.php index 15454a06e..3b0ac548d 100644 --- a/tests/Setup/DiscoveryTest.php +++ b/tests/Setup/DiscoveryTest.php @@ -4,6 +4,7 @@ namespace Flat3\Lodata\Tests\Setup; +use Doctrine\DBAL\Schema\Table; use Flat3\Lodata\Drivers\MongoEntitySet; use Flat3\Lodata\Drivers\MongoEntityType; use Flat3\Lodata\Facades\Lodata; @@ -61,12 +62,9 @@ public function test_cache_null() ]); Lodata::discoverEloquentModel(Airport::class); - $cacheKey = 'lodata.discovery.sql.properties.testing.airports'; - $cached = Cache::store('redis')->get($cacheKey); - $this->assertIsArray($cached); - $this->assertArrayHasKey('key', $cached); - $this->assertArrayHasKey('properties', $cached); - $this->assertEquals(-2, Cache::store('redis')->getRedis()->ttl($cacheKey)); + $table = Cache::store('redis')->get('lodata.discovery.sql.testing.airports'); + $this->assertInstanceOf(Table::class, $table); + $this->assertEquals(-2, Cache::store('redis')->getRedis()->ttl('lodata.discovery.sql.testing.airports')); } public function test_default_config() From a4fb975d0993c0db3974ae30e425623e260b046c Mon Sep 17 00:00:00 2001 From: Remo Laubacher Date: Thu, 7 May 2026 14:14:11 +0200 Subject: [PATCH 3/3] chore: drop stale phpstan-ignore in RedisTest::test_delete phpstan no longer reports an error on the suppressed line, so the ignore directive trips the non-ignorable ignore.unmatchedLine rule and fails the Analysis CI job. --- tests/EntityUpdate/RedisTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/EntityUpdate/RedisTest.php b/tests/EntityUpdate/RedisTest.php index 8fa2544b8..831cc72c0 100644 --- a/tests/EntityUpdate/RedisTest.php +++ b/tests/EntityUpdate/RedisTest.php @@ -27,7 +27,6 @@ public function test_delete() { parent::test_delete(); - // @phpstan-ignore-next-line $this->assertNull(Redis::get($this->entityId)); } }