-
Notifications
You must be signed in to change notification settings - Fork 25
feat: prevent multiple connections to the same system #93
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
base: feature/migration-logging-refactor
Are you sure you want to change the base?
Changes from 7 commits
66392b8
5da14f2
51cdc18
24efbc8
9b60272
afec881
6771f5c
5632aa8
3fa5eb8
49fe4d0
464e581
607e473
e8c497a
18aa263
a125a36
e8bdf93
f8b1f07
e80970b
8aef8e6
dff1431
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 |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| <?php declare(strict_types=1); | ||
| /* | ||
| * (c) shopware AG <[email protected]> | ||
| * For the full copyright and license information, please view the LICENSE | ||
| * file that was distributed with this source code. | ||
| */ | ||
|
|
||
| namespace SwagMigrationAssistant\Core\Migration; | ||
|
|
||
| use Doctrine\DBAL\Connection; | ||
| use Shopware\Core\Framework\Log\Package; | ||
| use Shopware\Core\Framework\Migration\MigrationStep; | ||
|
|
||
| /** | ||
| * @internal | ||
| */ | ||
| #[Package('fundamentals@after-sales')] | ||
| class Migration1764145444AddFingerprintToConnectionTable extends MigrationStep | ||
| { | ||
| public const TABLE = 'swag_migration_connection'; | ||
|
|
||
| public const COLUMN = 'source_system_fingerprint'; | ||
|
|
||
| public function getCreationTimestamp(): int | ||
| { | ||
| return 1764145444; | ||
| } | ||
|
|
||
| public function update(Connection $connection): void | ||
| { | ||
| $this->addColumn( | ||
| connection: $connection, | ||
| table: self::TABLE, | ||
| column: self::COLUMN, | ||
| type: 'VARCHAR(255)', | ||
| ); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| <?php declare(strict_types=1); | ||
| /* | ||
| * (c) shopware AG <[email protected]> | ||
| * For the full copyright and license information, please view the LICENSE | ||
| * file that was distributed with this source code. | ||
| */ | ||
|
|
||
| namespace SwagMigrationAssistant\Migration\Connection; | ||
|
|
||
| use Shopware\Core\Framework\Context; | ||
| use Shopware\Core\Framework\DataAbstractionLayer\EntityRepository; | ||
| use Shopware\Core\Framework\DataAbstractionLayer\Search\Criteria; | ||
| use Shopware\Core\Framework\DataAbstractionLayer\Search\Filter\EqualsFilter; | ||
| use Shopware\Core\Framework\DataAbstractionLayer\Search\Filter\MultiFilter; | ||
| use Shopware\Core\Framework\DataAbstractionLayer\Search\Filter\NotFilter; | ||
| use Shopware\Core\Framework\Log\Package; | ||
| use Shopware\Core\Framework\Util\Hasher; | ||
| use SwagMigrationAssistant\Profile\Shopware\Gateway\Api\ShopwareApiGateway; | ||
| use SwagMigrationAssistant\Profile\Shopware\Gateway\Local\ShopwareLocalGateway; | ||
|
|
||
| #[Package('fundamentals@after-sales')] | ||
| class ConnectionFingerprintService | ||
| { | ||
| public const FIELD_KEY_ENDPOINT = 'endpoint'; | ||
|
|
||
| public const FIELD_KEY_HOST = 'dbHost'; | ||
|
|
||
| public const FIELD_KEY_PORT = 'dbPort'; | ||
|
|
||
| public const FIELD_KEY_NAME = 'dbName'; | ||
|
|
||
| public const FIELD_KEY_PROFILE = 'profile'; | ||
|
|
||
| public const DEFAULT_DB_PORT = '3306'; | ||
|
|
||
| /** | ||
| * @param EntityRepository<SwagMigrationConnectionCollection> $connectionRepo | ||
| * | ||
| * @internal | ||
| */ | ||
| public function __construct( | ||
| private readonly EntityRepository $connectionRepo, | ||
| ) { | ||
| } | ||
|
|
||
| /** | ||
| * @param array<string, mixed>|null $credentialFields | ||
| */ | ||
| public function generateFingerprint(?array $credentialFields, string $gatewayName, string $profileName): ?string | ||
| { | ||
| if (empty($credentialFields)) { | ||
| return null; | ||
| } | ||
|
|
||
| $data = $this->extractFingerprintData($credentialFields, $gatewayName); | ||
|
|
||
| if (empty($data)) { | ||
| return null; | ||
| } | ||
|
|
||
| $data[self::FIELD_KEY_PROFILE] = $profileName; | ||
| ksort($data); | ||
|
|
||
| return Hasher::hash($data); | ||
| } | ||
|
|
||
| public function hasDuplicateConnection(string $fingerprint, Context $context, ?string $excludeConnectionId): bool | ||
| { | ||
| $criteria = new Criteria(); | ||
| $criteria->addFilter(new EqualsFilter('sourceSystemFingerprint', $fingerprint)); | ||
|
|
||
| if (isset($excludeConnectionId)) { | ||
| $criteria->addFilter(new NotFilter(MultiFilter::CONNECTION_AND, [ | ||
| new EqualsFilter('id', $excludeConnectionId), | ||
| ])); | ||
| } | ||
|
|
||
| return $this->connectionRepo->searchIds($criteria, $context)->getTotal() > 0; | ||
| } | ||
|
|
||
| /** | ||
| * @param array<string, mixed> $credentialFields | ||
| * | ||
| * @return array<string, string>|null | ||
| */ | ||
| private function extractFingerprintData(array $credentialFields, string $gatewayName): ?array | ||
| { | ||
| if ($gatewayName === ShopwareApiGateway::GATEWAY_NAME) { | ||
| return $this->extractApiFingerprintData($credentialFields, $gatewayName); | ||
| } | ||
|
|
||
| if ($gatewayName === ShopwareLocalGateway::GATEWAY_NAME) { | ||
| return $this->extractLocalFingerprintData($credentialFields); | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
|
|
||
| /** | ||
| * @param array<string, mixed> $credentialFields | ||
| * | ||
| * @return array<string, string>|null | ||
| */ | ||
| private function extractApiFingerprintData(array $credentialFields, string $gatewayName): ?array | ||
| { | ||
| if (!isset($credentialFields[self::FIELD_KEY_ENDPOINT])) { | ||
| return null; | ||
| } | ||
|
|
||
| $normalizedEndpoint = $this->normalizeEndpoint( | ||
| (string) $credentialFields[self::FIELD_KEY_ENDPOINT] | ||
| ); | ||
|
|
||
| return [ | ||
| 'type' => $gatewayName, | ||
| self::FIELD_KEY_ENDPOINT => $normalizedEndpoint, | ||
| ]; | ||
| } | ||
|
|
||
| /** | ||
| * @param array<string, mixed> $credentialFields | ||
| * | ||
| * @return array<string, string>|null | ||
| */ | ||
| private function extractLocalFingerprintData(array $credentialFields): ?array | ||
| { | ||
| if (!isset($credentialFields[self::FIELD_KEY_HOST], $credentialFields[self::FIELD_KEY_NAME])) { | ||
| return null; | ||
| } | ||
|
|
||
| return [ | ||
| 'type' => ShopwareLocalGateway::GATEWAY_NAME, | ||
| self::FIELD_KEY_HOST => \strtolower(\trim((string) $credentialFields[self::FIELD_KEY_HOST])), | ||
| self::FIELD_KEY_PORT => (string) ($credentialFields[self::FIELD_KEY_PORT] ?? self::DEFAULT_DB_PORT), | ||
| self::FIELD_KEY_NAME => \trim((string) $credentialFields[self::FIELD_KEY_NAME]), | ||
| ]; | ||
| } | ||
|
|
||
| private function normalizeEndpoint(string $endpoint): string | ||
| { | ||
| // lowercase & trim | ||
| $endpoint = strtolower(trim($endpoint)); | ||
|
|
||
| // remove ending slash | ||
| $endpoint = rtrim($endpoint, '/'); | ||
|
|
||
| // remove protocol (http or https does not matter for fingerprint) | ||
| $endpoint = (string) preg_replace('#^https?://#', '', $endpoint); | ||
|
|
||
| // remove www. prefix | ||
| return (string) preg_replace('#^www\.#', '', $endpoint); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,7 @@ | |
| use Shopware\Storefront\Theme\ThemeDefinition; | ||
| use Shopware\Storefront\Theme\ThemeService; | ||
| use SwagMigrationAssistant\Exception\MigrationException; | ||
| use SwagMigrationAssistant\Migration\Connection\ConnectionFingerprintService; | ||
| use SwagMigrationAssistant\Migration\Connection\SwagMigrationConnectionCollection; | ||
| use SwagMigrationAssistant\Migration\Connection\SwagMigrationConnectionEntity; | ||
| use SwagMigrationAssistant\Migration\DataSelection\DataSelectionCollection; | ||
|
|
@@ -74,6 +75,7 @@ public function __construct( | |
| private readonly MigrationContextFactoryInterface $migrationContextFactory, | ||
| private readonly PremappingServiceInterface $premappingService, | ||
| private readonly RunTransitionServiceInterface $runTransitionService, | ||
| private readonly ConnectionFingerprintService $connectionFingerprintService, | ||
| ) { | ||
| } | ||
|
|
||
|
|
@@ -134,19 +136,46 @@ public function getRunStatus(Context $context): MigrationState | |
| } | ||
|
|
||
| /** | ||
| * @param array<int, string>|null $credentialFields | ||
| * @param array<string, mixed>|null $credentialFields | ||
| */ | ||
| public function updateConnectionCredentials(Context $context, string $connectionUuid, ?array $credentialFields): void | ||
| { | ||
| if ($this->isMigrationRunning($context)) { | ||
| throw MigrationException::migrationIsAlreadyRunning(); | ||
| } | ||
|
|
||
| $context->scope(MigrationContext::SOURCE_CONTEXT, function (Context $context) use ($connectionUuid, $credentialFields): void { | ||
| $connection = $this->connectionRepo->search(new Criteria([$connectionUuid]), $context)->first(); | ||
larskemper marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
|
|
||
| if ($connection === null) { | ||
| throw MigrationException::noConnectionFound(); | ||
| } | ||
|
|
||
| $fingerprint = $this->connectionFingerprintService->generateFingerprint( | ||
| $credentialFields, | ||
| $connection->getGatewayName(), | ||
| $connection->getProfileName(), | ||
| ); | ||
|
|
||
| if ($fingerprint === null) { | ||
| throw MigrationException::invalidConnectionCredentials(); | ||
| } | ||
|
|
||
| $hasDuplicates = $this->connectionFingerprintService->hasDuplicateConnection( | ||
| $fingerprint, | ||
| $context, | ||
| $connectionUuid, | ||
| ); | ||
|
|
||
| if ($hasDuplicates) { | ||
| throw MigrationException::duplicateSourceConnection(); | ||
|
Member
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. Doesn't doing all of this in I tested this locally (SW6 → SW6) and creating a duplicate connection currently behaves like this:
Member
Author
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. That's a general migration design issue we didn't refactor until now. I suspect it was done this way because the wizard is url based: each step lives on its "own page", so state isn't shared and nothing gets stored in vuex. That said, this shouldn't prevent existing duplicate connections from receiving a fingerprint, the service only checks for existing fingerprints and exits early when none are present. So the first connection should get a fingerprint and is valid, the second not... but in this case it seems different, I will look into that, probably need to add a filter for null in the service 👍
Member
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. What I don't get is what this PR achieves then, because the only difference to me as a user of this is that I see the warning modal, but nothing "prevent[s] multiple connections to the same system" - what am I missing here? 😅
Member
Author
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. i get what you mean now, it validates newly created connections: "prevent creating multiple connections to the same system". Existing connections do not have a fingerprint, that's true, so their are not validated
Member
Author
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. found the issue: we update the connection first and then did a connection check, so creds where written before checking. I refactored it and now we first check the connection with given creds and then update the entity & generate the fingerprint
Member
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. Better 👍 but I still think there's something strange about it 🤔
Kapture.2025-12-10.at.18.16.43.mp4
Feel free to move this to follow-up issues due to how all of this works, but I think this feature does not make much sense with the behavior described in 2.
Member
Author
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. yeah, definitely strange but at least "not connected" 😅. Let's create a follow up bug ticket for that 👍 |
||
| } | ||
|
|
||
| $context->scope(MigrationContext::SOURCE_CONTEXT, function (Context $context) use ($connectionUuid, $credentialFields, $fingerprint): void { | ||
| $this->connectionRepo->update([ | ||
| [ | ||
| 'id' => $connectionUuid, | ||
| 'credentialFields' => $credentialFields, | ||
| 'sourceSystemFingerprint' => $fingerprint, | ||
| ], | ||
| ], $context); | ||
| }); | ||
|
|
||



Uh oh!
There was an error while loading. Please reload this page.