From 2f67f256164bffb7a72f812248fbfb21ea9ac2a0 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Fri, 16 Oct 2026 01:28:29 +0200 Subject: [PATCH 1/2] fix(magic-tables): a property named with an underscore is read as a property, and a config field may be a dot path The rehearsal R2 install logged 16,038 'meta is not a valid attribute' errors: shillinq's _meta property has a _meta column, and every magic-table reader took each _ column for object metadata, so MagicStatisticsHandler failed the whole row and the other readers dropped the property. MagicMetadataColumn now decides: a property the schema declares is a property unless ObjectEntity has that attribute. An unknown metadata column is skipped and noted once at debug. opencatalogi's publiccode schema was rejected on every import because the save-time check compared description.nl.longDescription with the top-level keys; the reader walks dot paths, so the check now accepts a path whose first segment is a property. example-set --check counted changes into the per-schema object count (12 dossiq cases against 10 declared); changes are now counted and printed apart. --- lib/Command/ExampleSetInstallCommand.php | 4 + lib/Db/MagicMapper.php | 13 +- lib/Db/MagicMapper/MagicBulkHandler.php | 10 +- lib/Db/MagicMapper/MagicMetadataColumn.php | 89 ++++++++++++ lib/Db/MagicMapper/MagicSearchHandler.php | 7 +- lib/Db/MagicMapper/MagicStatisticsHandler.php | 54 ++++++-- lib/Db/SchemaMapper.php | 33 ++++- .../ExampleSet/ExampleSetInstaller.php | 13 +- .../example-sets/specs/example-sets/spec.md | 7 + .../proposal.md | 22 +++ .../specs/magic-table-reads/spec.md | 19 +++ .../specs/schema-configuration-paths/spec.md | 19 +++ .../tasks.md | 17 +++ .../MagicMapper/MagicMetadataColumnTest.php | 68 +++++++++ tests/Unit/Db/MagicStatisticsHandlerTest.php | 52 +++++++ .../Db/SchemaMapperConfigurationPathTest.php | 130 ++++++++++++++++++ .../ExampleSet/ExampleSetInstallerTest.php | 1 + .../ExampleSet/ExampleSetLiveCheckTest.php | 24 ++++ 18 files changed, 550 insertions(+), 32 deletions(-) create mode 100644 lib/Db/MagicMapper/MagicMetadataColumn.php create mode 100644 openspec/changes/underscore-properties-and-config-paths/proposal.md create mode 100644 openspec/changes/underscore-properties-and-config-paths/specs/magic-table-reads/spec.md create mode 100644 openspec/changes/underscore-properties-and-config-paths/specs/schema-configuration-paths/spec.md create mode 100644 openspec/changes/underscore-properties-and-config-paths/tasks.md create mode 100644 tests/Unit/Db/MagicMapper/MagicMetadataColumnTest.php create mode 100644 tests/Unit/Db/SchemaMapperConfigurationPathTest.php diff --git a/lib/Command/ExampleSetInstallCommand.php b/lib/Command/ExampleSetInstallCommand.php index 50e5894e07..b9c1fa2670 100644 --- a/lib/Command/ExampleSetInstallCommand.php +++ b/lib/Command/ExampleSetInstallCommand.php @@ -174,6 +174,10 @@ private function check(array $plan, array $adopt, OutputInterface $output): int $output->writeln($type.': '.$count.' checked'); } + foreach ($result['checkedChanges'] as $type => $count) { + $output->writeln($type.': '.$count.' change(s) checked'); + } + if ($result['refusals'] !== []) { $this->printRefusals(refusals: $result['refusals'], output: $output); $output->writeln(''.count($result['refusals']).' refusal(s). No account, group or object of the set was written.'); diff --git a/lib/Db/MagicMapper.php b/lib/Db/MagicMapper.php index e99068690b..eb640e5eb4 100644 --- a/lib/Db/MagicMapper.php +++ b/lib/Db/MagicMapper.php @@ -47,6 +47,7 @@ use RuntimeException; use OCA\OpenRegister\Db\MagicMapper\MagicBulkHandler; use OCA\OpenRegister\Db\MagicMapper\MagicFacetHandler; +use OCA\OpenRegister\Db\MagicMapper\MagicMetadataColumn; use OCA\OpenRegister\Db\MagicMapper\MagicMetadataColumnHealer; use OCA\OpenRegister\Db\MagicMapper\MagicOrganizationHandler; use OCA\OpenRegister\Db\MagicMapper\MagicRbacHandler; @@ -9646,6 +9647,8 @@ public function getAllRegisterSchemaPairs(): array { * @param array $row The database row * * @return ObjectEntity|null The ObjectEntity or null if conversion fails + * + * @spec openspec/changes/underscore-properties-and-config-paths/specs/magic-table-reads/spec.md#requirement-a-property-whose-name-starts-with-an-underscore-is-read-as-a-property-req-mtr-001 */ private function rowToObjectEntity(array $row): ?ObjectEntity { // Check if we have the minimum required fields. @@ -9689,9 +9692,11 @@ private function rowToObjectEntity(array $row): ?ObjectEntity { // path produces (buildTableColumnsFromSchema), keyed by // property name, so collision-resolved columns round-trip // back to their original property instead of being lost. + $schemaProperties = ($schema->getProperties() ?? []); foreach ($this->buildTableColumnsFromSchema(schema: $schema) as $propertyName => $columnDef) { - // Skip metadata columns (handled separately above). - if (str_starts_with($propertyName, '_') === true) { + // Skip metadata columns (handled separately above); a schema + // property whose name starts with an underscore stays. + if (str_starts_with($propertyName, '_') === true && isset($schemaProperties[$propertyName]) === false) { continue; } @@ -9718,8 +9723,8 @@ private function rowToObjectEntity(array $row): ?ObjectEntity { // Build the object data from non-metadata columns. $objectData = []; foreach ($row as $column => $value) { - // Skip metadata columns (those starting with _). - if (str_starts_with($column, '_') === true) { + // Skip metadata columns; a property column with the prefix is read. + if (MagicMetadataColumn::field(column: $column, columnToPropertyMap: $columnToPropertyMap) !== null) { continue; } diff --git a/lib/Db/MagicMapper/MagicBulkHandler.php b/lib/Db/MagicMapper/MagicBulkHandler.php index 4bc36f6bd9..2222680f7d 100644 --- a/lib/Db/MagicMapper/MagicBulkHandler.php +++ b/lib/Db/MagicMapper/MagicBulkHandler.php @@ -139,10 +139,13 @@ private function getFullTableName(string $tableName): string { * @psalm-return list> * * @SuppressWarnings(PHPMD.CyclomaticComplexity) Bulk preparation requires handling multiple data type scenarios + * + * @spec openspec/changes/underscore-properties-and-config-paths/specs/magic-table-reads/spec.md#requirement-a-property-whose-name-starts-with-an-underscore-is-read-as-a-property-req-mtr-001 */ private function prepareObjectsForDynamicTable(array $objects, Register $register, Schema $schema): array { $prepared = []; $now = new DateTime(); + $schemaProperties = ($schema->getProperties() ?? []); foreach ($objects as $object) { $preparedObject = []; @@ -211,8 +214,11 @@ private function prepareObjectsForDynamicTable(array $objects, Register $registe // Properties can be at top level OR in 'object' key (structured format). $propertySource = $object['object'] ?? $object; foreach ($propertySource as $propertyName => $value) { - // Skip metadata (already handled) and @self. - if ($propertyName === '@self' || str_starts_with($propertyName, '_') === true) { + // Skip metadata (already handled) and @self; a schema property + // whose name starts with an underscore is written. + if ($propertyName === '@self' + || (str_starts_with($propertyName, '_') === true && isset($schemaProperties[$propertyName]) === false) + ) { continue; } diff --git a/lib/Db/MagicMapper/MagicMetadataColumn.php b/lib/Db/MagicMapper/MagicMetadataColumn.php new file mode 100644 index 0000000000..834c102231 --- /dev/null +++ b/lib/Db/MagicMapper/MagicMetadataColumn.php @@ -0,0 +1,89 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @link https://OpenRegister.app + * + * @spec openspec/changes/underscore-properties-and-config-paths/specs/magic-table-reads/spec.md#requirement-a-property-whose-name-starts-with-an-underscore-is-read-as-a-property-req-mtr-001 + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Db\MagicMapper; + +use OCA\OpenRegister\Db\ObjectEntity; + +/** + * Classify a magic-table column as metadata or as a schema property. + * + * @spec openspec/changes/underscore-properties-and-config-paths/specs/magic-table-reads/spec.md#requirement-a-property-whose-name-starts-with-an-underscore-is-read-as-a-property-req-mtr-001 + */ +final class MagicMetadataColumn { + + private const PREFIX = '_'; + + /** + * The metadata field a column carries, or null when it carries a property. + * + * A column without the prefix is a property. A prefixed column the schema + * maps to one of its properties is that property, unless ObjectEntity has + * an attribute of the same name: the metadata column then wins, because + * the table holds one column of that name and the metadata is what it was + * made for. + * + * @param string $column The column name. + * @param array $columnToPropertyMap The schema's column-to-property map. + * + * @return string|null The metadata field without its prefix (`schema_version`), or null. + * + * @spec openspec/changes/underscore-properties-and-config-paths/specs/magic-table-reads/spec.md#requirement-a-property-whose-name-starts-with-an-underscore-is-read-as-a-property-req-mtr-001 + */ + public static function field(string $column, array $columnToPropertyMap): ?string { + if (str_starts_with($column, self::PREFIX) === false) { + return null; + } + + $field = substr($column, strlen(self::PREFIX)); + if (isset($columnToPropertyMap[$column]) === true && self::attribute(field: $field) === null) { + return null; + } + + return $field; + }//end field() + + /** + * The ObjectEntity attribute a metadata field sets, or null when it has none. + * + * @param string $field The metadata field (`uuid`, `schema_version`). + * + * @return string|null The attribute name (`uuid`, `schemaVersion`), or null. + * + * @spec openspec/changes/underscore-properties-and-config-paths/specs/magic-table-reads/spec.md#requirement-a-property-whose-name-starts-with-an-underscore-is-read-as-a-property-req-mtr-001 + */ + public static function attribute(string $field): ?string { + $attribute = lcfirst(str_replace('_', '', ucwords($field, '_'))); + if ($attribute === '' || property_exists(ObjectEntity::class, $attribute) === false) { + return null; + } + + return $attribute; + }//end attribute() +}//end class diff --git a/lib/Db/MagicMapper/MagicSearchHandler.php b/lib/Db/MagicMapper/MagicSearchHandler.php index 68a16893d7..531d45233d 100644 --- a/lib/Db/MagicMapper/MagicSearchHandler.php +++ b/lib/Db/MagicMapper/MagicSearchHandler.php @@ -4065,6 +4065,8 @@ private function getSchemaColumnMaps(Schema $schema): array { * @SuppressWarnings(PHPMD.CyclomaticComplexity) * @SuppressWarnings(PHPMD.ExcessiveMethodLength) * @SuppressWarnings(PHPMD.NPathComplexity) Row to entity conversion requires many field mappings + * + * @spec openspec/changes/underscore-properties-and-config-paths/specs/magic-table-reads/spec.md#requirement-a-property-whose-name-starts-with-an-underscore-is-read-as-a-property-req-mtr-001 */ private function convertRowToObjectEntity( array $row, @@ -4090,9 +4092,8 @@ private function convertRowToObjectEntity( $columnToPropertyMap = $schemaMaps['columns']; foreach ($row as $column => $value) { - if (str_starts_with($column, '_') === true) { - // Metadata column - remove prefix and map to ObjectEntity. - $metadataField = substr($column, 1); + $metadataField = MagicMetadataColumn::field(column: $column, columnToPropertyMap: $columnToPropertyMap); + if ($metadataField !== null) { $metadataData[$metadataField] = $value; continue; } diff --git a/lib/Db/MagicMapper/MagicStatisticsHandler.php b/lib/Db/MagicMapper/MagicStatisticsHandler.php index e87f006062..65f9401865 100644 --- a/lib/Db/MagicMapper/MagicStatisticsHandler.php +++ b/lib/Db/MagicMapper/MagicStatisticsHandler.php @@ -92,6 +92,13 @@ class MagicStatisticsHandler { */ private const METADATA_PREFIX = '_'; + /** + * Metadata fields already noted as skipped in this process, by field. + * + * @var array + */ + private static array $skippedMetadataFields = []; + /** * Callable for counting objects in a register-schema table. * @@ -579,6 +586,8 @@ public function getSchemaChartData(?int $registerId = null, ?int $schemaId = nul * @SuppressWarnings(PHPMD.CyclomaticComplexity) Row to entity conversion requires many field mappings * @SuppressWarnings(PHPMD.NPathComplexity) Row to entity conversion requires many field mappings * @SuppressWarnings(PHPMD.ExcessiveMethodLength) Complete field mapping requires comprehensive handling + * + * @spec openspec/changes/underscore-properties-and-config-paths/specs/magic-table-reads/spec.md#requirement-a-property-whose-name-starts-with-an-underscore-is-read-as-a-property-req-mtr-001 */ public function convertRowToObjectEntity(array $row, Register $_register, Schema $_schema): ?ObjectEntity { try { @@ -610,9 +619,8 @@ public function convertRowToObjectEntity(array $row, Register $_register, Schema $objectData = []; foreach ($row as $columnName => $value) { - if (str_starts_with($columnName, self::METADATA_PREFIX) === true) { - // This is a metadata field. - $metadataField = substr($columnName, strlen(self::METADATA_PREFIX)); + $metadataField = MagicMetadataColumn::field(column: $columnName, columnToPropertyMap: $columnToPropertyMap); + if ($metadataField !== null) { // Handle datetime fields. if (in_array( @@ -713,21 +721,16 @@ public function convertRowToObjectEntity(array $row, Register $_register, Schema continue; } - $method = 'set' . ucfirst($field); - // Use is_callable() instead of method_exists() to support magic methods. - // Entity base class uses __call() for property setters. - if (is_callable([$objectEntity, $method]) === false) { - $this->logger->warning( - message: '[MagicStatisticsHandler] Method is not callable for metadata field', - context: ['file' => __FILE__, 'line' => __LINE__, 'field' => $field, 'method' => $method] - ); + // Entity setters are magic (__call), so is_callable() is true for + // any name; ask whether ObjectEntity has the attribute instead. + $attribute = MagicMetadataColumn::attribute(field: $field); + if ($attribute === null) { + $this->skipUnknownMetadataField(field: $field); continue; } + $method = 'set' . ucfirst($attribute); $objectEntity->$method($value); - // Debug critical fields. - if (in_array($field, ['id', 'uuid', 'owner'], true) === true) { - } }//end foreach // Set object data. @@ -762,6 +765,29 @@ public function convertRowToObjectEntity(array $row, Register $_register, Schema }//end try }//end convertRowToObjectEntity() + /** + * Note, once per field and process, a metadata column ObjectEntity has no attribute for. + * + * Such a column is skipped, not an error: the row is still read. + * + * @param string $field The metadata field without its prefix. + * + * @return void + * + * @spec openspec/changes/underscore-properties-and-config-paths/specs/magic-table-reads/spec.md#requirement-a-property-whose-name-starts-with-an-underscore-is-read-as-a-property-req-mtr-001 + */ + private function skipUnknownMetadataField(string $field): void { + if (isset(self::$skippedMetadataFields[$field]) === true) { + return; + } + + self::$skippedMetadataFields[$field] = true; + $this->logger->debug( + message: '[MagicStatisticsHandler] Skipped a metadata column ObjectEntity has no attribute for', + context: ['file' => __FILE__, 'line' => __LINE__, 'field' => $field] + ); + }//end skipUnknownMetadataField() + /** * Sanitize column name for safe database usage * diff --git a/lib/Db/SchemaMapper.php b/lib/Db/SchemaMapper.php index e0be5bfded..8f34596d59 100644 --- a/lib/Db/SchemaMapper.php +++ b/lib/Db/SchemaMapper.php @@ -2819,6 +2819,8 @@ private function validateConfigurationFields(Schema $schema): void { * - Simple property names: "name" * - Twig-style templates: "{{ voornaam }} {{ tussenvoegsel }} {{ achternaam }}" * - Pipe-separated fallbacks: "name | identifier | type" (uses first available) + * - Dot-notation paths into an object property: "description.nl.longDescription", + * valid when the first segment is a property (the reader walks the rest) * * @param string $fieldValue The field value to validate * @param array $propertyKeys Array of valid property keys @@ -2827,6 +2829,8 @@ private function validateConfigurationFields(Schema $schema): void { * @throws \Exception If field references non-existent properties * * @return void + * + * @spec openspec/changes/underscore-properties-and-config-paths/specs/schema-configuration-paths/spec.md#requirement-a-name-or-description-field-may-be-a-dot-notation-path-req-scp-001 */ private function validateConfigField(string $fieldValue, array $propertyKeys, string $fieldName): void { // Check if it's a Twig-style template (contains {{ ... }}). @@ -2860,7 +2864,7 @@ private function validateConfigField(string $fieldValue, array $propertyKeys, st // Validate that at least one fallback field exists in properties. $hasValidField = false; foreach ($fallbackFields as $field) { - if (in_array($field, $propertyKeys, true) === true) { + if ($this->isPropertyPath(path: $field, propertyKeys: $propertyKeys) === true) { $hasValidField = true; break; } @@ -2875,14 +2879,37 @@ private function validateConfigField(string $fieldValue, array $propertyKeys, st return; }//end if - // Simple property name - must exist in property keys. - if (in_array($fieldValue, $propertyKeys, true) === false) { + // Simple property name or dot-notation path - its first segment must be a property. + if ($this->isPropertyPath(path: $fieldValue, propertyKeys: $propertyKeys) === false) { throw new Exception( "The value for {$fieldName} ('{$fieldValue}') does not exist as a property in the schema." ); } }//end validateConfigField() + /** + * Whether a configuration path names a property of the schema. + * + * A plain name must be a property; a dot-notation path must start with one, + * as MetadataHydrationHandler::getValueFromPath() reads it. + * + * @param string $path The property name or dot-notation path. + * @param array $propertyKeys The schema's property keys. + * + * @return bool + * + * @spec openspec/changes/underscore-properties-and-config-paths/specs/schema-configuration-paths/spec.md#requirement-a-name-or-description-field-may-be-a-dot-notation-path-req-scp-001 + */ + private function isPropertyPath(string $path, array $propertyKeys): bool { + if (in_array($path, $propertyKeys, true) === true) { + return true; + } + + $first = strstr($path, '.', true); + + return $first !== false && $first !== '' && in_array($first, $propertyKeys, true) === true; + }//end isPropertyPath() + /** * Build required fields array from schema or property flags * diff --git a/lib/Service/ExampleSet/ExampleSetInstaller.php b/lib/Service/ExampleSet/ExampleSetInstaller.php index 1328011d70..0967980de9 100644 --- a/lib/Service/ExampleSet/ExampleSetInstaller.php +++ b/lib/Service/ExampleSet/ExampleSetInstaller.php @@ -91,9 +91,9 @@ public function __construct( * @param array $adopt Account ids the administrator lets the set use although it did not create them. * @param DateTimeImmutable|null $today The install day; now in the set's time zone when null. * - * @return array{refusals: array, checked: array, registers: array} - * Every refusal, the number of objects and changes checked per register and schema, and per required - * register `imported` or `kept`. + * @return array{refusals: array, checked: array, checkedChanges: array, registers: array} + * Every refusal, the number of objects (`checked`) and of changes (`checkedChanges`) checked per register + * and schema, counted apart as an install reports them, and per required register `imported` or `kept`. * * @spec openspec/changes/example-sets/specs/example-sets/spec.md#requirement-check-validates-every-object-against-its-real-schema-without-writing-req-exs-004 */ @@ -104,6 +104,7 @@ public function check(array $plan, array $adopt=[], ?DateTimeImmutable $today=nu $base = $this->withIds(plan: $plan, base: $this->prerequisites->context(plan: $plan, today: $today)); $refusals = array_merge($ready['refusals'], $this->people->refusals(plan: $plan, record: $record, adopt: $adopt)); $checked = []; + $changes = []; $planned = []; $users = $this->people->declaredUids(plan: $plan); foreach ($plan['fragments'] as $fragment) { @@ -140,13 +141,13 @@ public function check(array $plan, array $adopt=[], ?DateTimeImmutable $today=nu $context = (['objects' => $objects] + $base); $refusals = array_merge( $refusals, - $this->checkChanges(fragment: $fragment, context: $context, checked: $checked, plannedUsers: $users), + $this->checkChanges(fragment: $fragment, context: $context, checked: $changes, plannedUsers: $users), $this->checkAppConfig(fragment: $fragment, context: $context), $this->checkUserPreferences(fragment: $fragment, context: $context, users: $users) ); }//end foreach - return ['refusals' => $refusals, 'checked' => $checked, 'registers' => $ready['registers']]; + return ['refusals' => $refusals, 'checked' => $checked, 'checkedChanges' => $changes, 'registers' => $ready['registers']]; }//end check() /** @@ -243,7 +244,7 @@ private function withIds(array $plan, array $base): array { * * @param array $fragment The fragment. * @param array $context `cast`, `objects` and `today`. - * @param array $checked Counts per register and schema; changes are added. + * @param array $checked Counts of changes per register and schema; this fragment's are added. * @param array $plannedUsers Account ids the set creates, valid for a `user` format. * * @return array Refusals. diff --git a/openspec/changes/example-sets/specs/example-sets/spec.md b/openspec/changes/example-sets/specs/example-sets/spec.md index 42417adf16..ce521c6911 100644 --- a/openspec/changes/example-sets/specs/example-sets/spec.md +++ b/openspec/changes/example-sets/specs/example-sets/spec.md @@ -79,6 +79,13 @@ The loader SHALL create a Nextcloud account for every cast person with a `uid`, - **GIVEN** a set whose objects all satisfy their schemas - **WHEN** the check runs - **THEN** it SHALL exit 0 and report the number of objects checked per register and schema +- **AND** that number SHALL equal the number the install reports as declared for the same register and schema; changes SHALL be counted and reported apart + +#### Scenario: A change does not raise the object count +- **GIVEN** a fragment with ten `dossiq case` objects and two changes on `dossiq case` +- **WHEN** the set is checked and then installed +- **THEN** the check SHALL report `dossiq case: 10 checked` and `dossiq case: 2 change(s) checked`, and the install `dossiq case: 10 declared` +- @e2e exclude {console output; covered by ExampleSetLiveCheckTest} #### Scenario: An account the set creates passes a user format before it exists - **GIVEN** a ticket whose `assignee` (format `user`) is `{{cast:daan.uid}}`, and no account `daan.verbeek` on the instance diff --git a/openspec/changes/underscore-properties-and-config-paths/proposal.md b/openspec/changes/underscore-properties-and-config-paths/proposal.md new file mode 100644 index 0000000000..ec46536a4a --- /dev/null +++ b/openspec/changes/underscore-properties-and-config-paths/proposal.md @@ -0,0 +1,22 @@ +--- +kind: code +--- +# Proposal: underscore-properties-and-config-paths + +## Summary + +The Zuiddrecht rehearsal (R2, 15 October 2026) logged two OpenRegister faults that each cost real data, not only log lines. + +1. **A property whose name starts with an underscore broke every read of its row.** shillinq declares `_meta` on `RetentionRule`, `BbvAccountMapping` and `InnovatieboxTariff`. Its magic-table column is `_meta`, and the readers took every `_` column for object metadata: `MagicStatisticsHandler` called `ObjectEntity::setMeta()`, which throws, and dropped the whole row (16,038 log errors in one install, those objects unreadable through that path); `MagicSearchHandler` and `MagicMapper::rowToObjectEntity()` silently dropped the property; the bulk writer never wrote it. +2. **A dot-notation `objectDescriptionField` refused the schema.** opencatalogi's `publiccode` declares `description.nl.longDescription | description.en.longDescription`. The reader walks such paths (`MetadataHydrationHandler::getValueFromPath()`, and `Schema` documents the fields as dot-notation paths), but the save-time check compared the whole path with the top-level keys, so the schema was rejected on every import (`PARTIAL IMPORT`). + +## What changes + +- `MagicMetadataColumn` decides whether a `_` column is metadata or a schema property: a property the schema declares is a property, unless ObjectEntity has an attribute of that name. Both row readers, `rowToObjectEntity()` and the bulk writer use it or the same rule. +- A metadata column ObjectEntity has no attribute for is skipped and logged at debug once per field, instead of failing the row. A snake_case metadata column (`_schema_version`) reaches its camelCase attribute. +- `SchemaMapper::validateConfigField()` accepts a dot-notation path whose first segment is a property, alone or in a fallback list. + +## Out of scope + +- Renaming shillinq's `_meta` properties. +- Validating the segments after the first against nested `properties`: the reader does not need them declared. diff --git a/openspec/changes/underscore-properties-and-config-paths/specs/magic-table-reads/spec.md b/openspec/changes/underscore-properties-and-config-paths/specs/magic-table-reads/spec.md new file mode 100644 index 0000000000..564ccfb801 --- /dev/null +++ b/openspec/changes/underscore-properties-and-config-paths/specs/magic-table-reads/spec.md @@ -0,0 +1,19 @@ +# magic-table-reads + +## ADDED Requirements + +### Requirement: A property whose name starts with an underscore is read as a property (REQ-MTR-001) + +A magic-table column whose name starts with `_` SHALL be read as the schema property it holds when the schema declares that property, unless ObjectEntity has an attribute of the same name, in which case it SHALL be read as that metadata. Every reader of a magic-table row SHALL apply this rule, and the bulk writer SHALL write such a property. A metadata column ObjectEntity has no attribute for SHALL be skipped without failing the row, and SHALL be logged at debug level at most once per field per process. + +#### Scenario: shillinq's `_meta` property survives a read +- **GIVEN** a schema with properties `code` and `_meta` (an object), and a stored row with `_meta` = `{"source":"seeded"}` +- **WHEN** the row is converted to an ObjectEntity +- **THEN** the entity SHALL carry `_meta` = `{"source":"seeded"}` in its object data, and no error SHALL be logged +- @e2e exclude {row conversion has no screen of its own; covered by MagicStatisticsHandlerTest} + +#### Scenario: an unknown metadata column does not fail the row +- **GIVEN** a row with a `_meta` column that is no property of the schema, and `_schema_version` = `1.2.0` +- **WHEN** the row is converted +- **THEN** the entity SHALL be returned with its uuid and `schemaVersion` `1.2.0`, and `_meta` SHALL be skipped +- @e2e exclude {row conversion has no screen of its own; covered by MagicStatisticsHandlerTest} diff --git a/openspec/changes/underscore-properties-and-config-paths/specs/schema-configuration-paths/spec.md b/openspec/changes/underscore-properties-and-config-paths/specs/schema-configuration-paths/spec.md new file mode 100644 index 0000000000..5cdbc1d5cd --- /dev/null +++ b/openspec/changes/underscore-properties-and-config-paths/specs/schema-configuration-paths/spec.md @@ -0,0 +1,19 @@ +# schema-configuration-paths + +## ADDED Requirements + +### Requirement: A name or description field may be a dot-notation path (REQ-SCP-001) + +Saving a schema SHALL accept an `objectNameField` or `objectDescriptionField` that is a dot-notation path whose first segment is a property of the schema, alone or as one entry of a `|` fallback list, because the metadata reader walks such paths. A plain name, or a path whose first segment is no property, SHALL still be refused. + +#### Scenario: opencatalogi's publiccode schema imports +- **GIVEN** a schema with an object property `description` and `objectDescriptionField` `description.nl.longDescription | description.en.longDescription` +- **WHEN** the schema is saved or imported +- **THEN** it SHALL be stored, and the import SHALL NOT report it as a rejected schema +- @e2e exclude {save-time validation; covered by SchemaMapperConfigurationPathTest} + +#### Scenario: a path into a property the schema does not have is refused +- **GIVEN** a schema without a property `summary` +- **WHEN** it is saved with `objectDescriptionField` `summary.nl.longDescription` +- **THEN** it SHALL be refused naming the field +- @e2e exclude {save-time validation; covered by SchemaMapperConfigurationPathTest} diff --git a/openspec/changes/underscore-properties-and-config-paths/tasks.md b/openspec/changes/underscore-properties-and-config-paths/tasks.md new file mode 100644 index 0000000000..278d8490cf --- /dev/null +++ b/openspec/changes/underscore-properties-and-config-paths/tasks.md @@ -0,0 +1,17 @@ +# Tasks: underscore-properties-and-config-paths + +## 1. Underscore properties + +- [x] 1.1 `MagicMetadataColumn::field()` and `::attribute()` +- [x] 1.2 `MagicStatisticsHandler::convertRowToObjectEntity()` reads a `_` property as a property and skips an unknown metadata column once, at debug +- [x] 1.3 `MagicSearchHandler::convertRowToObjectEntity()` and `MagicMapper::rowToObjectEntity()` read a `_` property +- [x] 1.4 `MagicBulkHandler::prepareObjectsForDynamicTable()` writes a `_` property the schema declares + +## 2. Configuration paths + +- [x] 2.1 `SchemaMapper::isPropertyPath()`, used for single names and fallback lists + +## 3. Tests + +- [x] 3.1 `MagicMetadataColumnTest`, `MagicStatisticsHandlerTest` (underscore property, unknown metadata column as control) +- [x] 3.2 `SchemaMapperConfigurationPathTest` (paths accepted, unknown first segment refused as control) diff --git a/tests/Unit/Db/MagicMapper/MagicMetadataColumnTest.php b/tests/Unit/Db/MagicMapper/MagicMetadataColumnTest.php new file mode 100644 index 0000000000..ed2a0dcf0b --- /dev/null +++ b/tests/Unit/Db/MagicMapper/MagicMetadataColumnTest.php @@ -0,0 +1,68 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @link https://www.OpenRegister.app + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Tests\Unit\Db\MagicMapper; + +use OCA\OpenRegister\Db\MagicMapper\MagicMetadataColumn; +use PHPUnit\Framework\TestCase; + +/** + * @covers \OCA\OpenRegister\Db\MagicMapper\MagicMetadataColumn + */ +class MagicMetadataColumnTest extends TestCase { + + /** + * @return void + */ + public function testAColumnWithoutThePrefixIsAProperty(): void { + $this->assertNull(MagicMetadataColumn::field(column: 'code', columnToPropertyMap: [])); + }//end testAColumnWithoutThePrefixIsAProperty() + + /** + * @return void + */ + public function testAPrefixedColumnTheSchemaMapsToAPropertyIsThatProperty(): void { + $this->assertNull(MagicMetadataColumn::field(column: '_meta', columnToPropertyMap: ['_meta' => '_meta'])); + }//end testAPrefixedColumnTheSchemaMapsToAPropertyIsThatProperty() + + /** + * Control: the same column without a property in the schema is metadata. + * + * @return void + */ + public function testAPrefixedColumnWithoutAPropertyIsMetadata(): void { + $this->assertSame('meta', MagicMetadataColumn::field(column: '_meta', columnToPropertyMap: [])); + }//end testAPrefixedColumnWithoutAPropertyIsMetadata() + + /** + * A property named like a metadata attribute does not take the metadata column. + * + * @return void + */ + public function testAMetadataAttributeWinsOverAPropertyOfTheSameName(): void { + $this->assertSame('uuid', MagicMetadataColumn::field(column: '_uuid', columnToPropertyMap: ['_uuid' => '_uuid'])); + }//end testAMetadataAttributeWinsOverAPropertyOfTheSameName() + + /** + * @return void + */ + public function testAttributeNamesFollowObjectEntity(): void { + $this->assertSame('schemaVersion', MagicMetadataColumn::attribute(field: 'schema_version')); + $this->assertSame('id', MagicMetadataColumn::attribute(field: 'id')); + $this->assertNull(MagicMetadataColumn::attribute(field: 'meta')); + }//end testAttributeNamesFollowObjectEntity() +}//end class diff --git a/tests/Unit/Db/MagicStatisticsHandlerTest.php b/tests/Unit/Db/MagicStatisticsHandlerTest.php index 872d6b9c6e..ca6311d678 100644 --- a/tests/Unit/Db/MagicStatisticsHandlerTest.php +++ b/tests/Unit/Db/MagicStatisticsHandlerTest.php @@ -218,4 +218,56 @@ public function testForgetMagicTableListClearsTheMemo(): void { 'forgetMagicTableList() MUST drop the memo so the next call re-reads the catalog' ); }//end testForgetMagicTableListClearsTheMemo() + + /** + * A schema property whose name starts with an underscore (shillinq's + * `_meta`) has a `_meta` column. It is read as the property, not handed to + * ObjectEntity as a `meta` attribute, which failed the whole row. + */ + public function testAnUnderscorePropertyIsReadAsAPropertyAndTheRowConverts(): void { + $schema = $this->makeSchema( + properties: ['code' => ['type' => 'string'], '_meta' => ['type' => 'object']] + ); + $row = [ + '_uuid' => 'test-uuid-meta', + '_id' => 7, + '_register' => 1, + '_schema' => 1, + 'code' => 'R-1', + '_meta' => '{"source":"seeded"}', + ]; + + $this->logger->expects($this->never())->method('error'); + $entity = $this->handler->convertRowToObjectEntity($row, $this->makeRegister(), $schema); + + $this->assertNotNull($entity); + $this->assertSame('test-uuid-meta', $entity->getUuid()); + $this->assertSame(['source' => 'seeded'], $entity->getObject()['_meta']); + $this->assertSame('R-1', $entity->getObject()['code']); + }//end testAnUnderscorePropertyIsReadAsAPropertyAndTheRowConverts() + + /** + * Control: a prefixed column that is neither a property nor an attribute + * of ObjectEntity is skipped and noted once, and the row still converts. + */ + public function testAnUnknownMetadataColumnIsSkippedAndTheRowConverts(): void { + $schema = $this->makeSchema(properties: ['code' => ['type' => 'string']]); + $row = [ + '_uuid' => 'test-uuid-unknown', + '_id' => 8, + '_register' => 1, + '_schema' => 1, + '_meta' => '{"source":"seeded"}', + '_schema_version' => '1.2.0', + 'code' => 'R-2', + ]; + + $this->logger->expects($this->never())->method('error'); + $entity = $this->handler->convertRowToObjectEntity($row, $this->makeRegister(), $schema); + + $this->assertNotNull($entity); + $this->assertSame('test-uuid-unknown', $entity->getUuid()); + $this->assertSame('1.2.0', $entity->getSchemaVersion(), 'a snake_case metadata column reaches its camelCase attribute'); + $this->assertArrayNotHasKey('_meta', $entity->getObject()); + }//end testAnUnknownMetadataColumnIsSkippedAndTheRowConverts() }//end class diff --git a/tests/Unit/Db/SchemaMapperConfigurationPathTest.php b/tests/Unit/Db/SchemaMapperConfigurationPathTest.php new file mode 100644 index 0000000000..3e969bb2a5 --- /dev/null +++ b/tests/Unit/Db/SchemaMapperConfigurationPathTest.php @@ -0,0 +1,130 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @link https://www.OpenRegister.app + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Tests\Unit\Db; + +use Exception; +use OCA\OpenRegister\Db\OrganisationMapper; +use OCA\OpenRegister\Db\Schema; +use OCA\OpenRegister\Db\SchemaMapper; +use OCA\OpenRegister\Service\Schemas\PropertyValidatorHandler; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\IAppConfig; +use OCP\IDBConnection; +use OCP\IGroupManager; +use OCP\IUserSession; +use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; +use ReflectionMethod; + +/** + * Save-time check of objectNameField and objectDescriptionField paths. + */ +class SchemaMapperConfigurationPathTest extends TestCase { + + private SchemaMapper $mapper; + + /** + * @return void + */ + protected function setUp(): void { + $this->mapper = new SchemaMapper( + $this->createMock(IDBConnection::class), + $this->createMock(IEventDispatcher::class), + $this->createMock(PropertyValidatorHandler::class), + $this->createMock(OrganisationMapper::class), + $this->createMock(IUserSession::class), + $this->createMock(IGroupManager::class), + $this->createMock(IAppConfig::class), + $this->createMock(LoggerInterface::class) + ); + }//end setUp() + + /** + * Run the private check against a schema with these properties and configuration. + * + * @param array $configuration The schema configuration. + * + * @return void + */ + private function validate(array $configuration): void { + $schema = new Schema(); + $schema->setProperties( + [ + 'name' => ['type' => 'string'], + 'description' => ['type' => 'object'], + ] + ); + $schema->setConfiguration($configuration); + + $method = new ReflectionMethod(SchemaMapper::class, 'validateConfigurationFields'); + $method->invoke($this->mapper, $schema); + }//end validate() + + /** + * The publiccode shape: fallbacks that are paths into an object property. + * + * @return void + */ + public function testAFallbackOfPathsIntoAnObjectPropertyIsAccepted(): void { + $this->validate(['objectNameField' => 'name', 'objectDescriptionField' => 'description.nl.longDescription | description.en.longDescription']); + + $this->addToAssertionCount(1); + }//end testAFallbackOfPathsIntoAnObjectPropertyIsAccepted() + + /** + * A single path into an object property is accepted. + * + * @return void + */ + public function testASinglePathIntoAnObjectPropertyIsAccepted(): void { + $this->validate(['objectDescriptionField' => 'description.en.longDescription']); + + $this->addToAssertionCount(1); + }//end testASinglePathIntoAnObjectPropertyIsAccepted() + + /** + * Control: a path whose first segment is no property is still refused. + * + * @return void + */ + public function testAPathIntoAPropertyTheSchemaDoesNotHaveIsRefused(): void { + $this->expectException(Exception::class); + $this->expectExceptionMessage("None of the fallback fields in objectDescriptionField"); + + $this->validate(['objectDescriptionField' => 'summary.nl.longDescription | summary.en.longDescription']); + }//end testAPathIntoAPropertyTheSchemaDoesNotHaveIsRefused() + + /** + * Control: a plain name that is no property is still refused. + * + * @return void + */ + public function testAPlainNameTheSchemaDoesNotHaveIsRefused(): void { + $this->expectException(Exception::class); + $this->expectExceptionMessage("('title') does not exist as a property"); + + $this->validate(['objectNameField' => 'title']); + }//end testAPlainNameTheSchemaDoesNotHaveIsRefused() +}//end class diff --git a/tests/Unit/Service/ExampleSet/ExampleSetInstallerTest.php b/tests/Unit/Service/ExampleSet/ExampleSetInstallerTest.php index 9a6b17b4fd..0dd27201c6 100644 --- a/tests/Unit/Service/ExampleSet/ExampleSetInstallerTest.php +++ b/tests/Unit/Service/ExampleSet/ExampleSetInstallerTest.php @@ -147,6 +147,7 @@ public function testAValueTheSchemaRefusesFailsTheCheckAndWritesNothing(): void $result = $this->installer->check(plan: $this->catalogue->gather(set: 'demo'), today: $this->today()); $this->assertSame(['brp person' => 1, 'cases case' => 2], $result['checked']); + $this->assertSame([], $result['checkedChanges']); $this->assertSame(['cases/lib/Settings/example-sets/demo.json: object "case-1" (cases case): The property start is not allowed'], $result['refusals']); $this->assertSame(0, $this->store->creates); $this->assertSame(3, $this->store->validations, 'the check validates every object, hard validation or not'); diff --git a/tests/Unit/Service/ExampleSet/ExampleSetLiveCheckTest.php b/tests/Unit/Service/ExampleSet/ExampleSetLiveCheckTest.php index cbb94e2755..59417de3bd 100644 --- a/tests/Unit/Service/ExampleSet/ExampleSetLiveCheckTest.php +++ b/tests/Unit/Service/ExampleSet/ExampleSetLiveCheckTest.php @@ -80,6 +80,30 @@ public function testAChangeOnACaseWithAJsonTextHistoryPassesTheCheckAndKeepsTheH $this->assertSame([], $this->store->ids(register: 'cases', schema: 'case')); } + /** + * The check counts objects per register and schema as the install declares + * them, and a change on the same schema apart (the zuiddrecht set read 12 + * dossiq cases checked against 10 declared). + */ + public function testTheCheckCountsObjectsAsTheInstallDeclaresThemAndChangesApart(): void { + $this->app(app: 'cases', fragment: [ + 'objects' => [ + ['key' => 'case-1', 'register' => 'cases', 'schema' => 'case', 'data' => ['title' => 'Parkeren']], + ['key' => 'result-1', 'register' => 'cases', 'schema' => 'result', 'data' => ['case' => '{{object:case-1.id}}']], + ], + 'changes' => [['key' => 'case-1-result', 'register' => 'cases', 'schema' => 'case', 'object' => 'case-1', 'set' => ['result' => '{{object:result-1.id}}']]], + ]); + $plan = $this->catalogue->gather(set: 'demo'); + + $checked = $this->installer->check(plan: $plan, today: $this->today()); + $report = $this->installer->install(plan: $plan, today: $this->today()); + + $this->assertSame(['cases case' => 1, 'cases result' => 1], $checked['checked']); + $this->assertSame(['cases case' => 1], $checked['checkedChanges']); + $this->assertSame($report['types']['cases case']['declared'], $checked['checked']['cases case'], 'check and install count the same objects'); + $this->assertSame(['cases/case-1-result' => 'created'], $report['changes']); + } + /** * A change that declares its own `jsonFields` appends to the list inside * the text, is proved against the decoded list, and is taken out again. From f28140d7e5d68bdbea3516245a06a03f2c1b17fb Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Fri, 16 Oct 2026 02:05:26 +0200 Subject: [PATCH 2/2] refactor(magic-tables): the column classifier is an instance, and the bulk writer asks it too check:strict's phpmd flagged static access, a long variable name and the bulk writer's method length; the write-side rule moves into MagicMetadataColumn::skippedOnWrite(). --- lib/Db/MagicMapper.php | 2 +- lib/Db/MagicMapper/MagicBulkHandler.php | 9 ++---- lib/Db/MagicMapper/MagicMetadataColumn.php | 26 +++++++++++++++-- lib/Db/MagicMapper/MagicSearchHandler.php | 10 ++++++- lib/Db/MagicMapper/MagicStatisticsHandler.php | 18 ++++++++---- .../MagicMapper/MagicMetadataColumnTest.php | 28 ++++++++++++++----- 6 files changed, 70 insertions(+), 23 deletions(-) diff --git a/lib/Db/MagicMapper.php b/lib/Db/MagicMapper.php index eb640e5eb4..2e802ec60a 100644 --- a/lib/Db/MagicMapper.php +++ b/lib/Db/MagicMapper.php @@ -9724,7 +9724,7 @@ private function rowToObjectEntity(array $row): ?ObjectEntity { $objectData = []; foreach ($row as $column => $value) { // Skip metadata columns; a property column with the prefix is read. - if (MagicMetadataColumn::field(column: $column, columnToPropertyMap: $columnToPropertyMap) !== null) { + if ((new MagicMetadataColumn())->field(column: $column, columnToPropertyMap: $columnToPropertyMap) !== null) { continue; } diff --git a/lib/Db/MagicMapper/MagicBulkHandler.php b/lib/Db/MagicMapper/MagicBulkHandler.php index 2222680f7d..4f92739035 100644 --- a/lib/Db/MagicMapper/MagicBulkHandler.php +++ b/lib/Db/MagicMapper/MagicBulkHandler.php @@ -145,7 +145,7 @@ private function getFullTableName(string $tableName): string { private function prepareObjectsForDynamicTable(array $objects, Register $register, Schema $schema): array { $prepared = []; $now = new DateTime(); - $schemaProperties = ($schema->getProperties() ?? []); + $columnRoles = new MagicMetadataColumn(); foreach ($objects as $object) { $preparedObject = []; @@ -214,11 +214,7 @@ private function prepareObjectsForDynamicTable(array $objects, Register $registe // Properties can be at top level OR in 'object' key (structured format). $propertySource = $object['object'] ?? $object; foreach ($propertySource as $propertyName => $value) { - // Skip metadata (already handled) and @self; a schema property - // whose name starts with an underscore is written. - if ($propertyName === '@self' - || (str_starts_with($propertyName, '_') === true && isset($schemaProperties[$propertyName]) === false) - ) { + if ($columnRoles->skippedOnWrite(key: $propertyName, schemaProperties: ($schema->getProperties() ?? [])) === true) { continue; } @@ -246,6 +242,7 @@ private function prepareObjectsForDynamicTable(array $objects, Register $registe return $prepared; }//end prepareObjectsForDynamicTable() + /** * Map the six display-metadata columns onto a prepared row. * diff --git a/lib/Db/MagicMapper/MagicMetadataColumn.php b/lib/Db/MagicMapper/MagicMetadataColumn.php index 834c102231..b66b7ba52b 100644 --- a/lib/Db/MagicMapper/MagicMetadataColumn.php +++ b/lib/Db/MagicMapper/MagicMetadataColumn.php @@ -56,19 +56,39 @@ final class MagicMetadataColumn { * * @spec openspec/changes/underscore-properties-and-config-paths/specs/magic-table-reads/spec.md#requirement-a-property-whose-name-starts-with-an-underscore-is-read-as-a-property-req-mtr-001 */ - public static function field(string $column, array $columnToPropertyMap): ?string { + public function field(string $column, array $columnToPropertyMap): ?string { if (str_starts_with($column, self::PREFIX) === false) { return null; } $field = substr($column, strlen(self::PREFIX)); - if (isset($columnToPropertyMap[$column]) === true && self::attribute(field: $field) === null) { + if (isset($columnToPropertyMap[$column]) === true && $this->attribute(field: $field) === null) { return null; } return $field; }//end field() + /** + * Whether a key of an object being written is metadata or `@self`, not a property column. + * + * A key starting with the prefix is metadata unless the schema declares it as a property. + * + * @param string $key The object key. + * @param array $schemaProperties The schema's properties. + * + * @return bool + * + * @spec openspec/changes/underscore-properties-and-config-paths/specs/magic-table-reads/spec.md#requirement-a-property-whose-name-starts-with-an-underscore-is-read-as-a-property-req-mtr-001 + */ + public function skippedOnWrite(string $key, array $schemaProperties): bool { + if ($key === '@self') { + return true; + } + + return str_starts_with($key, self::PREFIX) === true && isset($schemaProperties[$key]) === false; + }//end skippedOnWrite() + /** * The ObjectEntity attribute a metadata field sets, or null when it has none. * @@ -78,7 +98,7 @@ public static function field(string $column, array $columnToPropertyMap): ?strin * * @spec openspec/changes/underscore-properties-and-config-paths/specs/magic-table-reads/spec.md#requirement-a-property-whose-name-starts-with-an-underscore-is-read-as-a-property-req-mtr-001 */ - public static function attribute(string $field): ?string { + public function attribute(string $field): ?string { $attribute = lcfirst(str_replace('_', '', ucwords($field, '_'))); if ($attribute === '' || property_exists(ObjectEntity::class, $attribute) === false) { return null; diff --git a/lib/Db/MagicMapper/MagicSearchHandler.php b/lib/Db/MagicMapper/MagicSearchHandler.php index 531d45233d..92eb3619c9 100644 --- a/lib/Db/MagicMapper/MagicSearchHandler.php +++ b/lib/Db/MagicMapper/MagicSearchHandler.php @@ -204,6 +204,13 @@ class MagicSearchHandler { */ private SearchTermSqlCompiler $termCompiler; + /** + * Tells metadata columns from underscore-named property columns. + * + * @var MagicMetadataColumn + */ + private MagicMetadataColumn $columnRoles; + /** * Constructor for MagicSearchHandler * @@ -234,6 +241,7 @@ public function __construct( ) { $this->termParser = new SearchTermParser(); $this->termCompiler = new SearchTermSqlCompiler(); + $this->columnRoles = new MagicMetadataColumn(); }//end __construct() /** @@ -4092,7 +4100,7 @@ private function convertRowToObjectEntity( $columnToPropertyMap = $schemaMaps['columns']; foreach ($row as $column => $value) { - $metadataField = MagicMetadataColumn::field(column: $column, columnToPropertyMap: $columnToPropertyMap); + $metadataField = $this->columnRoles->field(column: $column, columnToPropertyMap: $columnToPropertyMap); if ($metadataField !== null) { $metadataData[$metadataField] = $value; continue; diff --git a/lib/Db/MagicMapper/MagicStatisticsHandler.php b/lib/Db/MagicMapper/MagicStatisticsHandler.php index 65f9401865..546c97ccb7 100644 --- a/lib/Db/MagicMapper/MagicStatisticsHandler.php +++ b/lib/Db/MagicMapper/MagicStatisticsHandler.php @@ -97,7 +97,14 @@ class MagicStatisticsHandler { * * @var array */ - private static array $skippedMetadataFields = []; + private static array $skippedFields = []; + + /** + * Tells metadata columns from underscore-named property columns. + * + * @var MagicMetadataColumn + */ + private MagicMetadataColumn $columnRoles; /** * Callable for counting objects in a register-schema table. @@ -126,6 +133,7 @@ public function __construct( private readonly DateTimeNormalizer $dateTimeNormalizer, private readonly SchemaTypeConverter $schemaTypeConverter, ) { + $this->columnRoles = new MagicMetadataColumn(); }//end __construct() /** @@ -619,7 +627,7 @@ public function convertRowToObjectEntity(array $row, Register $_register, Schema $objectData = []; foreach ($row as $columnName => $value) { - $metadataField = MagicMetadataColumn::field(column: $columnName, columnToPropertyMap: $columnToPropertyMap); + $metadataField = $this->columnRoles->field(column: $columnName, columnToPropertyMap: $columnToPropertyMap); if ($metadataField !== null) { // Handle datetime fields. @@ -723,7 +731,7 @@ public function convertRowToObjectEntity(array $row, Register $_register, Schema // Entity setters are magic (__call), so is_callable() is true for // any name; ask whether ObjectEntity has the attribute instead. - $attribute = MagicMetadataColumn::attribute(field: $field); + $attribute = $this->columnRoles->attribute(field: $field); if ($attribute === null) { $this->skipUnknownMetadataField(field: $field); continue; @@ -777,11 +785,11 @@ public function convertRowToObjectEntity(array $row, Register $_register, Schema * @spec openspec/changes/underscore-properties-and-config-paths/specs/magic-table-reads/spec.md#requirement-a-property-whose-name-starts-with-an-underscore-is-read-as-a-property-req-mtr-001 */ private function skipUnknownMetadataField(string $field): void { - if (isset(self::$skippedMetadataFields[$field]) === true) { + if (isset(self::$skippedFields[$field]) === true) { return; } - self::$skippedMetadataFields[$field] = true; + self::$skippedFields[$field] = true; $this->logger->debug( message: '[MagicStatisticsHandler] Skipped a metadata column ObjectEntity has no attribute for', context: ['file' => __FILE__, 'line' => __LINE__, 'field' => $field] diff --git a/tests/Unit/Db/MagicMapper/MagicMetadataColumnTest.php b/tests/Unit/Db/MagicMapper/MagicMetadataColumnTest.php index ed2a0dcf0b..c496c56496 100644 --- a/tests/Unit/Db/MagicMapper/MagicMetadataColumnTest.php +++ b/tests/Unit/Db/MagicMapper/MagicMetadataColumnTest.php @@ -29,14 +29,14 @@ class MagicMetadataColumnTest extends TestCase { * @return void */ public function testAColumnWithoutThePrefixIsAProperty(): void { - $this->assertNull(MagicMetadataColumn::field(column: 'code', columnToPropertyMap: [])); + $this->assertNull((new MagicMetadataColumn())->field(column: 'code', columnToPropertyMap: [])); }//end testAColumnWithoutThePrefixIsAProperty() /** * @return void */ public function testAPrefixedColumnTheSchemaMapsToAPropertyIsThatProperty(): void { - $this->assertNull(MagicMetadataColumn::field(column: '_meta', columnToPropertyMap: ['_meta' => '_meta'])); + $this->assertNull((new MagicMetadataColumn())->field(column: '_meta', columnToPropertyMap: ['_meta' => '_meta'])); }//end testAPrefixedColumnTheSchemaMapsToAPropertyIsThatProperty() /** @@ -45,7 +45,7 @@ public function testAPrefixedColumnTheSchemaMapsToAPropertyIsThatProperty(): voi * @return void */ public function testAPrefixedColumnWithoutAPropertyIsMetadata(): void { - $this->assertSame('meta', MagicMetadataColumn::field(column: '_meta', columnToPropertyMap: [])); + $this->assertSame('meta', (new MagicMetadataColumn())->field(column: '_meta', columnToPropertyMap: [])); }//end testAPrefixedColumnWithoutAPropertyIsMetadata() /** @@ -54,15 +54,29 @@ public function testAPrefixedColumnWithoutAPropertyIsMetadata(): void { * @return void */ public function testAMetadataAttributeWinsOverAPropertyOfTheSameName(): void { - $this->assertSame('uuid', MagicMetadataColumn::field(column: '_uuid', columnToPropertyMap: ['_uuid' => '_uuid'])); + $this->assertSame('uuid', (new MagicMetadataColumn())->field(column: '_uuid', columnToPropertyMap: ['_uuid' => '_uuid'])); }//end testAMetadataAttributeWinsOverAPropertyOfTheSameName() + /** + * A declared `_` property is written; undeclared `_` keys and `@self` are not. + * + * @return void + */ + public function testOnlyADeclaredUnderscorePropertyIsWritten(): void { + $roles = new MagicMetadataColumn(); + + $this->assertFalse($roles->skippedOnWrite(key: '_meta', schemaProperties: ['_meta' => ['type' => 'object']])); + $this->assertTrue($roles->skippedOnWrite(key: '_meta', schemaProperties: [])); + $this->assertTrue($roles->skippedOnWrite(key: '@self', schemaProperties: [])); + $this->assertFalse($roles->skippedOnWrite(key: 'code', schemaProperties: [])); + }//end testOnlyADeclaredUnderscorePropertyIsWritten() + /** * @return void */ public function testAttributeNamesFollowObjectEntity(): void { - $this->assertSame('schemaVersion', MagicMetadataColumn::attribute(field: 'schema_version')); - $this->assertSame('id', MagicMetadataColumn::attribute(field: 'id')); - $this->assertNull(MagicMetadataColumn::attribute(field: 'meta')); + $this->assertSame('schemaVersion', (new MagicMetadataColumn())->attribute(field: 'schema_version')); + $this->assertSame('id', (new MagicMetadataColumn())->attribute(field: 'id')); + $this->assertNull((new MagicMetadataColumn())->attribute(field: 'meta')); }//end testAttributeNamesFollowObjectEntity() }//end class