Skip to content

Commit 3584712

Browse files
CarlSchwanCarlSchwan
committed
refactor: Use psalm:strict for AppFramework/ORM
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
1 parent beae168 commit 3584712

15 files changed

Lines changed: 158 additions & 95 deletions

File tree

‎build/rector-strict.php‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,8 @@
4747
$nextcloudDir . '/tests/Core/Sharing',
4848
$nextcloudDir . '/apps/files/lib/Sharing',
4949
$nextcloudDir . '/apps/files/tests/Sharing',
50+
$nextcloudDir . '/lib/public/AppFramework/ORM',
51+
$nextcloudDir . '/lib/private/AppFramework/ORM',
5052
])
5153
->withAutoloadPaths([
5254
// ensure rector properly autoload the public interfaces

‎lib/private/AppFramework/ORM/EntityInfo.php‎

Lines changed: 27 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -13,14 +13,15 @@
1313
use OCP\AppFramework\ORM\Attribute\JoinColumn;
1414
use OCP\AppFramework\ORM\Attribute\ManyToOne;
1515
use OCP\AppFramework\ORM\Attribute\OneToOne;
16+
use OCP\DB\Types;
1617

1718
/**
1819
* @template T as object
1920
*/
20-
class EntityInfo {
21+
final class EntityInfo {
2122
public readonly string $tableName;
2223

23-
/** @var array<string, string> */
24+
/** @var array<string, Types::*> */
2425
public array $mappingColumnToTypes = [];
2526

2627
/** @var array<string, string> */
@@ -77,55 +78,65 @@ public function __construct(
7778
}
7879
}
7980

80-
if ($propertyAttributes->id !== null && $propertyAttributes->column === null) {
81+
if ($propertyAttributes->id instanceof Id && !$propertyAttributes->column instanceof Column) {
8182
throw new \RuntimeException($this->entityClass . ' has a Id attribute on ' . $property->getName() . ' but not the corresponding required Column attribute.');
8283
}
8384

84-
if ($propertyAttributes->oneToOne !== null
85+
if ($propertyAttributes->oneToOne instanceof OneToOne
8586
&& $propertyAttributes->oneToOne->mappedBy !== null
86-
&& $propertyAttributes->joinColumn !== null
87+
&& $propertyAttributes->joinColumn instanceof JoinColumn
8788
&& $propertyAttributes->joinColumn->onDelete !== null) {
8889
throw new \RuntimeException($this->entityClass . '::' . $property->getName() . ' sets JoinColumn::$onDelete on the mappedBy (inverse) side of a OneToOne relation, where it has no effect. Set it on the owning (invertedBy) side instead.');
8990
}
9091

91-
if ($propertyAttributes->oneToOne !== null && $propertyAttributes->oneToOne->mappedBy !== null) {
92-
$this->validateMappedBy($property, $propertyAttributes->oneToOne);
92+
if ($propertyAttributes->oneToOne instanceof OneToOne && $propertyAttributes->oneToOne->mappedBy !== null) {
93+
$this->validateMappedBy($property, $propertyAttributes->oneToOne, $propertyAttributes->oneToOne->mappedBy);
9394
}
9495

9596
$this->propertiesAttributes[] = $propertyAttributes;
9697
}
9798

98-
if ($this->idProperty === null) {
99+
if (!$this->idProperty instanceof \ReflectionProperty) {
99100
throw new \RuntimeException($this->entityClass . ' does not have a primary key. This is not supported for repositories backed tables.');
100101
}
101102
}
102103

104+
public function getIdProperty(): \ReflectionProperty {
105+
if (!$this->idProperty instanceof \ReflectionProperty) {
106+
throw new \LogicException('Unreachable: the constructor already guarantees idProperty is set.');
107+
}
108+
109+
return $this->idProperty;
110+
}
111+
103112
/**
104113
* Checks that mappedBy actually points at the owning side of the relation, so a typo
105114
* fails loudly at construction time instead of silently resolving to null forever.
115+
*
116+
* @param non-empty-string $mappedBy Same value as $oneToOne->mappedBy, narrowed non-null by the caller.
106117
*/
107-
private function validateMappedBy(\ReflectionProperty $property, OneToOne $oneToOne): void {
108-
$prefix = $this->entityClass . '::' . $property->getName() . ' has mappedBy: \'' . $oneToOne->mappedBy . '\', but ';
118+
private function validateMappedBy(\ReflectionProperty $property, OneToOne $oneToOne, string $mappedBy): void {
119+
$prefix = $this->entityClass . '::' . $property->getName() . " has mappedBy: '" . $mappedBy . "', but ";
109120
$targetReflection = new \ReflectionClass($oneToOne->targetEntity);
110121

111-
if (!$targetReflection->hasProperty($oneToOne->mappedBy)) {
112-
throw new \RuntimeException($prefix . $oneToOne->targetEntity . ' has no property named \'' . $oneToOne->mappedBy . '\'.');
122+
if (!$targetReflection->hasProperty($mappedBy)) {
123+
throw new \RuntimeException($prefix . $oneToOne->targetEntity . " has no property named '" . $mappedBy . "'.");
113124
}
114125

115-
$targetProperty = $targetReflection->getProperty($oneToOne->mappedBy);
126+
$targetProperty = $targetReflection->getProperty($mappedBy);
116127
$targetOneToOneAttributes = $targetProperty->getAttributes(OneToOne::class, \ReflectionAttribute::IS_INSTANCEOF);
117128
$targetOneToOne = $targetOneToOneAttributes === [] ? null : $targetOneToOneAttributes[0]->newInstance();
118129

119130
if ($targetOneToOne === null || $targetOneToOne->invertedBy === null) {
120-
throw new \RuntimeException($prefix . $oneToOne->targetEntity . '::' . $oneToOne->mappedBy . ' is not the owning (invertedBy) side of a OneToOne relation.');
131+
throw new \RuntimeException($prefix . $oneToOne->targetEntity . '::' . $mappedBy . ' is not the owning (invertedBy) side of a OneToOne relation.');
121132
}
122133

123134
if ($targetOneToOne->invertedBy !== $property->getName()) {
124-
throw new \RuntimeException($prefix . $oneToOne->targetEntity . '::' . $oneToOne->mappedBy . '\'s invertedBy points at \'' . $targetOneToOne->invertedBy . '\' instead.');
135+
throw new \RuntimeException($prefix . $oneToOne->targetEntity . '::' . $mappedBy . "'s invertedBy points at '" . $targetOneToOne->invertedBy . "' instead.");
125136
}
126137

127138
if ($targetProperty->getAttributes(JoinColumn::class, \ReflectionAttribute::IS_INSTANCEOF) === []) {
128-
throw new \RuntimeException($prefix . $oneToOne->targetEntity . '::' . $oneToOne->mappedBy . ' has no JoinColumn attribute.');
139+
throw new \RuntimeException($prefix . $oneToOne->targetEntity . '::' . $mappedBy . ' has no JoinColumn attribute.');
129140
}
130141
}
131142
}

‎lib/private/AppFramework/ORM/EntityManager.php‎

Lines changed: 40 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,9 @@
1010
use Doctrine\DBAL\Schema\Table;
1111
use OC\DB\SchemaWrapper;
1212
use OCP\AppFramework\ORM\Attribute\Column;
13+
use OCP\AppFramework\ORM\Attribute\Id;
1314
use OCP\AppFramework\ORM\Attribute\JoinColumn;
15+
use OCP\AppFramework\ORM\Attribute\OneToOne;
1416
use OCP\AppFramework\ORM\Repository;
1517
use OCP\DB\Exception;
1618
use OCP\DB\QueryBuilder\IQueryBuilder;
@@ -19,7 +21,7 @@
1921
use OCP\Server;
2022
use OCP\Snowflake\ISnowflakeGenerator;
2123

22-
class EntityManager {
24+
final class EntityManager {
2325
public function __construct(
2426
private readonly IDBConnection $connection,
2527
) {
@@ -37,6 +39,7 @@ public function getEntityInfo(string $entityClass): EntityInfo {
3739
if (!isset($this->entitiesInfo[$entityClass])) {
3840
$this->entitiesInfo[$entityClass] = new EntityInfo($entityClass);
3941
}
42+
4043
/** @var EntityInfo<T> $entityInfo */
4144
$entityInfo = $this->entitiesInfo[$entityClass];
4245
return $entityInfo;
@@ -53,11 +56,12 @@ public function getEntityInfo(string $entityClass): EntityInfo {
5356
* @return Repository<T>
5457
*/
5558
public function getRepository(string $entityClass): Repository {
59+
/** @psalm-suppress InternalMethod both are private */
5660
return new Repository($this->connection, $this, $entityClass);
5761
}
5862

5963
/**
60-
* @template T
64+
* @template T of object
6165
* @psalm-param T $entity
6266
* @return T
6367
*/
@@ -66,23 +70,21 @@ public function insert(object $entity): object {
6670
$insert = $this->connection->getQueryBuilder();
6771

6872
$isSnowflake = false;
69-
$primaryProperty = null;
7073
$values = [];
7174

7275
foreach ($entityInfo->propertiesAttributes as $propertyAttributes) {
7376
$property = $propertyAttributes->property;
7477
if ($propertyAttributes->id !== null && $propertyAttributes->column !== null) {
75-
$primaryProperty = $property;
7678
$generatorClass = $propertyAttributes->id->generatorClass;
7779
if ($generatorClass) {
7880
if ($generatorClass === ISnowflakeGenerator::class) {
7981
$generator = Server::get($generatorClass);
8082
$isSnowflake = true;
81-
/** @psalm-suppress UndefinedClass */
8283
$values[$propertyAttributes->column->name] = $generator->nextId();
8384
$property->setValue($entity, $insert->createNamedParameter($values[$propertyAttributes->column->name]));
8485
}
8586
}
87+
8688
continue;
8789
}
8890

@@ -92,17 +94,18 @@ public function insert(object $entity): object {
9294
if ($property->getValue($entity) !== null) {
9395
throw new \LogicException($entity::class . '::' . $property->getName() . ' is the mappedBy (inverse) side of a OneToOne relation and cannot be persisted directly; set it from the owning (invertedBy) side instead.');
9496
}
97+
9598
continue;
9699
}
97100

98101
$joinColumn = $propertyAttributes->joinColumn;
99-
/** @var object $object */
102+
/** @var object|null $targetEntity */
100103
$targetEntity = $property->getValue($entity);
101104
$targetEntityInfo = $this->getEntityInfo($targetEntityClass);
102105
if ($targetEntity === null) {
103106
$values[$joinColumn->name] = $insert->createNamedParameter(null);
104107
} else {
105-
$values[$joinColumn->name] = $insert->createNamedParameter($targetEntityInfo->idProperty->getValue($targetEntity));
108+
$values[$joinColumn->name] = $insert->createNamedParameter($targetEntityInfo->getIdProperty()->getValue($targetEntity));
106109
}
107110

108111
continue;
@@ -119,33 +122,35 @@ public function insert(object $entity): object {
119122
->executeStatement();
120123

121124
if (!$isSnowflake) {
122-
$primaryProperty->setValue($entity, $insert->getLastInsertId());
125+
$entityInfo->getIdProperty()->setValue($entity, $insert->getLastInsertId());
123126
}
127+
124128
return $entity;
125129
}
126130

127131
/**
128-
* @template T
132+
* @template T of object
129133
* @psalm-param T $entity
130134
* @return T
131135
*/
132136
public function update(object $entity): object {
133-
$entityClass = get_class($entity);
137+
$entityClass = $entity::class;
134138
$entityInfo = $this->getEntityInfo($entityClass);
135139

136140
$update = $this->connection->getQueryBuilder();
137141
$update->update($entityInfo->tableName);
138142

139143
foreach ($entityInfo->propertiesAttributes as $propertyAttributes) {
140144
$property = $propertyAttributes->property;
145+
/** @psalm-suppress MixedAssignment */
141146
$value = $property->getValue($entity);
142147

143148
if ($propertyAttributes->id !== null && $propertyAttributes->column !== null) {
144149
if ($value === null) {
145150
throw new \LogicException('Trying to update an entity with no primary key set.');
146151
}
147152

148-
$update->andWhere($update->expr()->eq($entityInfo->mappingPropertyToColumn[$entityInfo->idProperty->getName()], $update->createNamedParameter($property->getValue($entity))));
153+
$update->andWhere($update->expr()->eq($entityInfo->mappingPropertyToColumn[$entityInfo->getIdProperty()->getName()], $update->createNamedParameter($property->getValue($entity))));
149154
// don't update the id
150155
continue;
151156
}
@@ -156,15 +161,14 @@ public function update(object $entity): object {
156161
continue;
157162
}
158163

159-
/** @var JoinColumn $joinColumn */
160164
$joinColumn = $propertyAttributes->joinColumn;
161-
/** @var object $object */
165+
/** @var object|null $targetEntity */
162166
$targetEntity = $value;
163167
$targetEntityInfo = $this->getEntityInfo($targetEntityClass);
164168
if ($targetEntity === null) {
165169
$update->set($joinColumn->name, $update->createNamedParameter(null));
166170
} else {
167-
$update->set($joinColumn->name, $update->createNamedParameter($targetEntityInfo->idProperty->getValue($targetEntity)));
171+
$update->set($joinColumn->name, $update->createNamedParameter($targetEntityInfo->getIdProperty()->getValue($targetEntity)));
168172
}
169173

170174
continue;
@@ -181,11 +185,11 @@ public function update(object $entity): object {
181185
}
182186

183187
/**
184-
* @template T
188+
* @template T of object
185189
* @psalm-param T $entity
186190
*/
187191
public function delete(object $entity): void {
188-
$entityClass = get_class($entity);
192+
$entityClass = $entity::class;
189193
$entityInfo = $this->getEntityInfo($entityClass);
190194

191195
$delete = $this->connection->getQueryBuilder();
@@ -195,6 +199,7 @@ public function delete(object $entity): void {
195199
foreach ($entityInfo->propertiesAttributes as $propertyAttributes) {
196200
if ($propertyAttributes->id !== null && $propertyAttributes->column !== null) {
197201
$property = $propertyAttributes->property;
202+
/** @var int|string $value */
198203
$value = $property->getValue($entity);
199204

200205
$delete->andWhere($delete->expr()->eq($propertyAttributes->column->name, $delete->createNamedParameter($value)));
@@ -208,11 +213,12 @@ public function delete(object $entity): void {
208213

209214
try {
210215
$delete->executeStatement();
211-
} catch (Exception $e) {
212-
if ($e->getReason() === Exception::REASON_FOREIGN_KEY_VIOLATION) {
213-
throw new \LogicException($entityClass . ' cannot be deleted: another entity still references it. Delete the related entity first, or set onDelete: \'CASCADE\' on the owning JoinColumn.', 0, $e);
216+
} catch (Exception $exception) {
217+
if ($exception->getReason() === Exception::REASON_FOREIGN_KEY_VIOLATION) {
218+
throw new \LogicException($entityClass . " cannot be deleted: another entity still references it. Delete the related entity first, or set onDelete: 'CASCADE' on the owning JoinColumn.", 0, $exception);
214219
}
215-
throw $e;
220+
221+
throw $exception;
216222
}
217223
}
218224

@@ -222,14 +228,16 @@ public function delete(object $entity): void {
222228
*/
223229
public function getParameterType(string $type, bool $isArray): string|int {
224230
if ($isArray) {
231+
/** @psalm-suppress DeprecatedConstant Types::JSON is only discouraged in WHERE clauses; mapping it is still supported. */
225232
return match ($type) {
226233
Types::INTEGER, Types::SMALLINT => IQueryBuilder::PARAM_INT_ARRAY,
227234
Types::STRING => IQueryBuilder::PARAM_STR_ARRAY,
228235
Types::JSON => IQueryBuilder::PARAM_JSON,
229-
default => throw new \LogicException("Parameter type '$type' is not supported as an array."),
236+
default => throw new \LogicException(sprintf("Parameter type '%s' is not supported as an array.", $type)),
230237
};
231238
}
232239

240+
/** @psalm-suppress DeprecatedConstant Types::JSON is only discouraged in WHERE clauses; mapping it is still supported. */
233241
return match ($type) {
234242
Types::INTEGER, Types::SMALLINT => IQueryBuilder::PARAM_INT,
235243
Types::STRING => IQueryBuilder::PARAM_STR,
@@ -262,53 +270,56 @@ public function createTable(string $entityClass, SchemaWrapper $schema): void {
262270

263271
$this->createRelationColumn($propertyAttributes, $table, $schema);
264272
}
265-
266-
$schema->getWrappedSchema()->toSql($this->connection->getDatabasePlatform());
267273
}
268274

275+
/**
276+
* @param class-string $entityClass
277+
*/
269278
public function dropTable(string $entityClass, string $prefix): void {
270279
$entityInfo = $this->getEntityInfo($entityClass);
271280
$this->connection->dropTable($prefix . $entityInfo->tableName);
272281
}
273282

274283
private function createProperty(PropertyAttributes $attributes, Table $table): void {
275-
if ($attributes->column === null) {
284+
if (!$attributes->column instanceof Column) {
276285
return;
277286
}
278-
/** @var Column $columnAttribute */
287+
279288
$columnAttribute = $attributes->column;
280289
$options = [
281290
'notnull' => !$columnAttribute->nullable,
282291
];
283292
if ($columnAttribute->length !== null) {
284293
$options['length'] = $columnAttribute->length;
285294
}
295+
286296
if ($columnAttribute->default !== null) {
297+
/** @psalm-suppress MixedAssignment default can be anything */
287298
$options['default'] = $columnAttribute->default;
288299
}
289300

290-
if ($attributes->id !== null && $attributes->id->generatorClass === null) {
301+
if ($attributes->id instanceof Id && $attributes->id->generatorClass === null) {
291302
$options['autoincrement'] = true;
292303
}
293304

294305
$table->addColumn($columnAttribute->name, $columnAttribute->type, $options);
295306

296-
if ($attributes->id !== null) {
307+
if ($attributes->id instanceof Id) {
297308
$table->setPrimaryKey([$columnAttribute->name]);
298309
}
299310
}
300311

301312
private function createRelationColumn(PropertyAttributes $attributes, Table $table, SchemaWrapper $schema): void {
302313
$targetEntityClass = $attributes->getOwningRelationTarget();
303-
if ($attributes->joinColumn === null || $targetEntityClass === null) {
314+
if (!$attributes->joinColumn instanceof JoinColumn || $targetEntityClass === null) {
304315
return;
305316
}
306317

307318
$table->addColumn($attributes->joinColumn->name, Types::BIGINT, [
308319
'notnull' => !$attributes->joinColumn->nullable,
309320
]);
310321

311-
if ($attributes->oneToOne !== null) {
322+
if ($attributes->oneToOne instanceof OneToOne) {
312323
// Enforces the "one" in OneToOne; ManyToOne intentionally allows duplicates.
313324
$table->addUniqueIndex([$attributes->joinColumn->name]);
314325
}

0 commit comments

Comments
 (0)