-
Notifications
You must be signed in to change notification settings - Fork 5
fix: resolve cross-entity slug references during configuration import #727
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
7bc5102
5b818ba
25ffd3f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -22,6 +22,7 @@ | |
| use OCA\OpenConnector\Service\ConfigurationHandlers\JobHandler; | ||
| use OCA\OpenConnector\Service\ConfigurationHandlers\SourceHandler; | ||
| use OCA\OpenConnector\Service\ConfigurationHandlers\RuleHandler; | ||
| use OCP\AppFramework\Db\Entity; | ||
|
|
||
| /** | ||
| * Class ConfigurationService | ||
|
|
@@ -218,6 +219,93 @@ private function resetMappings(): void | |
| $this->mappings['schema']['slugToId'] = $this->schemaMapper->getSlugToIdMap(); | ||
| } | ||
|
|
||
| /** | ||
| * Register a freshly imported entity in the in-memory slug/ID maps. | ||
| * | ||
| * This keeps references created earlier in the same import run available | ||
| * to later entities without requiring a second import pass. | ||
| * | ||
| * @param string $entityType Mapping bucket name | ||
| * @param Entity $entity Imported entity | ||
| * | ||
| * @return void | ||
| */ | ||
| private function registerImportedEntityMapping(string $entityType, Entity $entity): void | ||
| { | ||
| if (isset($this->mappings[$entityType]) === false) { | ||
| return; | ||
| } | ||
|
|
||
| if (method_exists($entity, 'getSlug') === false) { | ||
| return; | ||
| } | ||
|
|
||
| $id = $entity->getId(); | ||
| $slug = $entity->getSlug(); | ||
|
|
||
| if ($id === null || $slug === null || $slug === '') { | ||
| return; | ||
| } | ||
|
|
||
| $id = (string) $id; | ||
|
|
||
| $this->mappings[$entityType]['idToSlug'][$id] = $slug; | ||
| $this->mappings[$entityType]['slugToId'][$slug] = $id; | ||
| } | ||
|
|
||
| /** | ||
| * Ensure imported component payloads always carry their component-key slug. | ||
| * | ||
| * Some exports reference related entities by component key even when the nested | ||
| * payload is missing or inconsistent about its own slug field. Normalizing the | ||
| * slug up front keeps first-pass imports referentially stable. | ||
| * | ||
| * @param array $data Component payload | ||
| * @param string $slug Component key | ||
| * | ||
| * @return array | ||
| */ | ||
| private function withComponentSlug(array $data, string $slug): array | ||
| { | ||
| if (($data['slug'] ?? null) === null || $data['slug'] === '') { | ||
| $data['slug'] = $slug; | ||
| } | ||
|
|
||
| return $data; | ||
| } | ||
|
|
||
| /** | ||
| * Re-run import handlers for entity types whose dependencies are imported later | ||
| * in the ordered pass, so those references are resolved with the complete mapping set. | ||
| * | ||
| * Rules (step 3) and endpoints (step 4) are imported before synchronizations (step 5), | ||
| * so any references they hold to synchronizations cannot be resolved in the first pass. | ||
| * Synchronizations and jobs are imported last and have no such forward dependencies. | ||
| * | ||
| * Each handler is idempotent (upsert by slug), so re-running it is safe. | ||
| * | ||
| * @param array<string,mixed> $components Original imported components | ||
| * @param array<string,array<string,Entity>> $result Imported entity result map | ||
| * | ||
| * @return void | ||
| */ | ||
| private function reconcileImportedReferences(array $components, array &$result): void | ||
| { | ||
| if (isset($components['rules']) && is_array($components['rules'])) { | ||
| foreach ($components['rules'] as $ruleSlug => $ruleData) { | ||
| $ruleData = $this->withComponentSlug($ruleData, $ruleSlug); | ||
| $result['rules'][$ruleSlug] = $this->handlers['rule']->import($ruleData, $this->mappings); | ||
| } | ||
| } | ||
|
|
||
| if (isset($components['endpoints']) && is_array($components['endpoints'])) { | ||
| foreach ($components['endpoints'] as $endpointSlug => $endpointData) { | ||
| $endpointData = $this->withComponentSlug($endpointData, $endpointSlug); | ||
| $result['endpoints'][$endpointSlug] = $this->handlers['endpoint']->import($endpointData, $this->mappings); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Get all entities associated with a specific configuration ID, indexed by their slug. | ||
| * | ||
|
|
@@ -753,51 +841,65 @@ public function importConfiguration(array $oas): array | |
| // 1. Import sources first (no dependencies). | ||
| if (isset($components['sources'])) { | ||
| foreach ($components['sources'] as $sourceSlug => $sourceData) { | ||
| $sourceData = $this->withComponentSlug($sourceData, $sourceSlug); | ||
| $source = $this->handlers['source']->import($sourceData, $this->mappings); | ||
| $result['sources'][$sourceSlug] = $source; | ||
| $this->registerImportedEntityMapping('source', $source); | ||
| } | ||
| } | ||
|
|
||
| // 2. Import mappings (depends on sources). | ||
| if (isset($components['mappings'])) { | ||
| foreach ($components['mappings'] as $mappingSlug => $mappingData) { | ||
| $mappingData = $this->withComponentSlug($mappingData, $mappingSlug); | ||
| $mapping = $this->handlers['mapping']->import($mappingData, $this->mappings); | ||
| $result['mappings'][$mappingSlug] = $mapping; | ||
| $this->registerImportedEntityMapping('mapping', $mapping); | ||
| } | ||
| } | ||
|
|
||
| // 3. Import rules (depends on sources). | ||
| if (isset($components['rules'])) { | ||
| foreach ($components['rules'] as $ruleSlug => $ruleData) { | ||
| $ruleData = $this->withComponentSlug($ruleData, $ruleSlug); | ||
| $rule = $this->handlers['rule']->import($ruleData, $this->mappings); | ||
| $result['rules'][$ruleSlug] = $rule; | ||
| $this->registerImportedEntityMapping('rule', $rule); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Concern —
Same issue applies to the Either remove both dead calls, or — if rule/job slugs are intended to be resolvable in the future — add the corresponding buckets to |
||
| } | ||
| } | ||
|
|
||
| // 4. Import endpoints (depends on sources and mappings). | ||
| if (isset($components['endpoints'])) { | ||
| foreach ($components['endpoints'] as $endpointSlug => $endpointData) { | ||
| $endpointData = $this->withComponentSlug($endpointData, $endpointSlug); | ||
| $endpoint = $this->handlers['endpoint']->import($endpointData, $this->mappings); | ||
| $result['endpoints'][$endpointSlug] = $endpoint; | ||
| $this->registerImportedEntityMapping('endpoint', $endpoint); | ||
| } | ||
| } | ||
|
|
||
| // 5. Import synchronizations (depends on sources, mappings, and endpoints). | ||
| if (isset($components['synchronizations'])) { | ||
| foreach ($components['synchronizations'] as $syncSlug => $syncData) { | ||
| $syncData = $this->withComponentSlug($syncData, $syncSlug); | ||
| $synchronization = $this->handlers['synchronization']->import($syncData, $this->mappings); | ||
| $result['synchronizations'][$syncSlug] = $synchronization; | ||
| $this->registerImportedEntityMapping('synchronization', $synchronization); | ||
| } | ||
| } | ||
|
|
||
| // 6. Import jobs (depends on synchronizations, endpoints, and sources). | ||
| if (isset($components['jobs'])) { | ||
| foreach ($components['jobs'] as $jobSlug => $jobData) { | ||
| $jobData = $this->withComponentSlug($jobData, $jobSlug); | ||
| $job = $this->handlers['job']->import($jobData, $this->mappings); | ||
| $result['jobs'][$jobSlug] = $job; | ||
| $this->registerImportedEntityMapping('job', $job); | ||
| } | ||
| } | ||
|
|
||
| $this->reconcileImportedReferences($components, $result); | ||
|
|
||
| return $result; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Concern —
reconcileImportedReferencesdoesn't re-register updated endpoint mappings after the second-pass importAfter re-importing endpoints here,
registerImportedEntityMappingis not called on the updated entity. If the upsert produces a freshly-assigned ID (e.g. the first-pass import failed to fully resolve the entity and the second pass creates it),$this->mappings['endpoint']will be stale.No current downstream step reads from the endpoint mapping after the reconcile pass runs, so there is no observable bug today. However, if
reconcileImportedReferencesis extended to include additional entity types that reference endpoints, the stale mapping will cause hard-to-trace failures.Consider calling
$this->registerImportedEntityMapping('endpoint', $result['endpoints'][$endpointSlug])after this line.