From e9134789b1c29cc85aa59db557f1e09a026c6586 Mon Sep 17 00:00:00 2001 From: killecaptron <203002577+killecaptron@users.noreply.github.com> Date: Mon, 7 Sep 2026 17:11:27 +0200 Subject: [PATCH 1/2] Do not duplicate attachments on merge when only their external URL changed An attachment's name has to be unique per part and attachment type (see the UniqueEntity constraint on PartAttachment), so two attachments can never legitimately coexist once they share both of them - no matter what their content is. The attachment merger however also compared the external path, so a provider which hands out a fresh signed or tracking URL for the very same file on every request (e.g. TrustedParts) made the merger try to add a second, colliding attachment on every update of the part, which then failed to persist. Treat name + attachment type as the identity of an attachment and, if the other side carries an updated external URL, refresh it, so a stale or expired link does not linger just because the URL happened to differ from the previous import. Co-Authored-By: Claude Opus 5 --- .../Mergers/EntityMergerHelperTrait.php | 22 ++++++++----- .../EntityMergers/Mergers/PartMergerTest.php | 32 +++++++++++++++++++ 2 files changed, 46 insertions(+), 8 deletions(-) diff --git a/src/Services/EntityMergers/Mergers/EntityMergerHelperTrait.php b/src/Services/EntityMergers/Mergers/EntityMergerHelperTrait.php index ac0c0f28d..13f98d051 100644 --- a/src/Services/EntityMergers/Mergers/EntityMergerHelperTrait.php +++ b/src/Services/EntityMergers/Mergers/EntityMergerHelperTrait.php @@ -246,15 +246,21 @@ protected function mergeCollections(object $target, object $other, string $field protected function mergeAttachments(AttachmentContainingDBElement $target, AttachmentContainingDBElement $other): object { return $this->mergeCollections($target, $other, 'attachments', function (Attachment $t, Attachment $o) { - if ($t->getName() === $o->getName() && $t->getAttachmentType() === $o->getAttachmentType()) { - //An external source is authoritative. Ignore generated internal paths. - if ($t->hasExternal() || $o->hasExternal()) { - return $t->getExternalPath() === $o->getExternalPath(); - } - //Only for local attachments, compare the internal path. - return $t->getInternalPath() === $o->getInternalPath(); + if ($t->getName() !== $o->getName() || $t->getAttachmentType() !== $o->getAttachmentType()) { + return false; + } + + //An attachment's name must be unique per part and attachment type (see the UniqueEntity constraint on + //PartAttachment), so two attachments can never legitimately coexist once they share both - regardless of + //their content. Treat them as the same attachment rather than trying to add a second, colliding one. + //If the external source provides an updated URL (some providers issue a fresh signed/tracking URL for + //the very same file on every request, e.g. TrustedParts), refresh it, so a stale or expired link does + //not linger just because the URL happened to differ from the previous import. + if ($o->hasExternal() && $t->getExternalPath() !== $o->getExternalPath()) { + $t->setURL($o->getExternalPath()); } - return false; + + return true; }); } diff --git a/tests/Services/EntityMergers/Mergers/PartMergerTest.php b/tests/Services/EntityMergers/Mergers/PartMergerTest.php index 84551019d..c75b7132a 100644 --- a/tests/Services/EntityMergers/Mergers/PartMergerTest.php +++ b/tests/Services/EntityMergers/Mergers/PartMergerTest.php @@ -351,6 +351,38 @@ public function testMergeOfAttachmentsWithExternalPath(): void ); } + public function testMergeOfAttachmentWithChangedExternalUrlIsNotDuplicated(): void + { + //PartAttachment enforces that name + attachment type must be unique per part (see the UniqueEntity + //constraint), so two attachments can never coexist once they share both, no matter their content. Some + //providers (e.g. TrustedParts) issue a fresh signed/tracking URL for the very same file on every request, + //so comparing the external path in addition to name+type would make the merger try to add a second, + //colliding attachment on every refresh, which fails to persist. + $attachmentType = new AttachmentType(); + + $existingAttachment = (new PartAttachment()) + ->setName('datasheet') + ->setAttachmentType($attachmentType) + ->setExternalPath('https://trustedparts.com/productredirect?id=old-token'); + + $part1 = (new Part())->addAttachment($existingAttachment); + + $refreshedAttachment = (new PartAttachment()) + ->setName('datasheet') + ->setAttachmentType($attachmentType) + ->setExternalPath('https://trustedparts.com/productredirect?id=new-token'); + + $part2 = (new Part())->addAttachment($refreshedAttachment); + + $merged = $this->merger->merge($part1, $part2); + + //The two attachments must be merged into one, not kept side by side + $this->assertCount(1, $merged->getAttachments()); + $this->assertSame($existingAttachment, $merged->getAttachments()->get(0)); + //The stale URL must be refreshed to the new one + $this->assertSame('https://trustedparts.com/productredirect?id=new-token', $merged->getAttachments()->get(0)->getExternalPath()); + } + public function testSupports() { $this->assertFalse($this->merger->supports(new \stdClass(), new \stdClass())); From 23c6c7a4d607f702e2bede076a0e77f87e5f289b Mon Sep 17 00:00:00 2001 From: killecaptron <203002577+killecaptron@users.noreply.github.com> Date: Mon, 7 Sep 2026 17:11:27 +0200 Subject: [PATCH 2/2] Do not pre-select "download to server" for known non-downloadable files Some info providers only hand out URLs which a browser can follow, but a server can not: TrustedParts.com for example routes every datasheet through a tracking redirect on its own domain, which answers anything but a real browser with a 403 (and its terms of use forbid downloading from the site itself anyway). Combined with the app.attachments.download_by_default setting, every part created from such a provider produced one failed-download error per file - all noise, since none of them was ever going to work. FileDTO can now express that a file is not downloadable, and the part form uses that information to leave the "download to server" checkbox unchecked for those attachments. The flag is passed from the DTO through a form option instead of being stored on the Attachment entity, as it is a property of the provider's URL, not of the attachment itself. Co-Authored-By: Claude Opus 5 --- src/Form/AttachmentFormType.php | 16 +++- src/Form/Part/PartBaseType.php | 3 + .../InfoProviderSystem/DTOs/FileDTO.php | 5 ++ .../InfoProviderSystem/DTOs/PartDetailDTO.php | 18 ++++ .../Providers/TrustedPartsProvider.php | 7 +- .../InfoProviderSystem/DTOs/FileDTOTest.php | 12 +++ .../DTOs/PartDetailDTOTest.php | 84 +++++++++++++++++++ .../Providers/TrustedPartsProviderTest.php | 2 + 8 files changed, 143 insertions(+), 4 deletions(-) create mode 100644 tests/Services/InfoProviderSystem/DTOs/PartDetailDTOTest.php diff --git a/src/Form/AttachmentFormType.php b/src/Form/AttachmentFormType.php index 29b8e7129..88843af2a 100644 --- a/src/Form/AttachmentFormType.php +++ b/src/Form/AttachmentFormType.php @@ -174,7 +174,9 @@ static function (FormEvent $event): void { //If the attachment should be downloaded by default (and is download allowed at all), register a listener, // which sets the downloadURL checkbox to true for new attachments if ($this->settings->downloadByDefault && $this->settings->allowDownloads) { - $builder->addEventListener(FormEvents::POST_SET_DATA, function (FormEvent $event): void { + $non_downloadable_urls = $options['non_downloadable_urls']; + + $builder->addEventListener(FormEvents::POST_SET_DATA, function (FormEvent $event) use ($non_downloadable_urls): void { $form = $event->getForm(); $attachment = $form->getData(); @@ -182,8 +184,10 @@ static function (FormEvent $event): void { return; } - //If the attachment was not created yet, set the downloadURL checkbox to true - if ($attachment === null || $attachment->getId() === null) { + //If the attachment was not created yet and is actually downloadable, set the downloadURL checkbox to true + if (($attachment === null || $attachment->getId() === null) + && ($attachment === null + || !in_array($attachment->getExternalPath(), $non_downloadable_urls, true))) { $checkbox = $form->get('downloadURL'); //Ensure that the checkbox is not disabled if ($checkbox->isDisabled()) { @@ -202,7 +206,13 @@ public function configureOptions(OptionsResolver $resolver): void 'data_class' => Attachment::class, 'max_file_size' => $this->settings->maxFileSize, 'allow_builtins' => true, + //The external URLs which a local copy can never be downloaded from (e.g. tracking redirects of an info + //provider, which reject non-browser requests). Attachments with such an URL are not pre-selected for + //download, so the user is not shown a download error on every save. See FileDTO::$downloadable. + 'non_downloadable_urls' => [], ]); + + $resolver->setAllowedTypes('non_downloadable_urls', 'string[]'); } public function finishView(FormView $view, FormInterface $form, array $options): void diff --git a/src/Form/Part/PartBaseType.php b/src/Form/Part/PartBaseType.php index afef8fdbc..a185b1122 100644 --- a/src/Form/Part/PartBaseType.php +++ b/src/Form/Part/PartBaseType.php @@ -265,6 +265,9 @@ public function buildForm(FormBuilderInterface $builder, array $options): void 'label' => false, 'entry_options' => [ 'data_class' => PartAttachment::class, + //Some provider files are only reachable from a browser and can never be downloaded by the server, + //so they must not be pre-selected for download (see FileDTO::$downloadable) + 'non_downloadable_urls' => $dto?->getNonDownloadableFileUrls() ?? [], ], 'by_reference' => false, ]); diff --git a/src/Services/InfoProviderSystem/DTOs/FileDTO.php b/src/Services/InfoProviderSystem/DTOs/FileDTO.php index 84eed0c95..2871adadc 100644 --- a/src/Services/InfoProviderSystem/DTOs/FileDTO.php +++ b/src/Services/InfoProviderSystem/DTOs/FileDTO.php @@ -38,10 +38,15 @@ /** * @param string $url The URL where to get this file * @param string|null $name Optionally the name of this file + * @param bool $downloadable Whether a local copy of this file can be downloaded from the URL. Set this to + * false for URLs which are known to never be downloadable by a server (e.g. tracking redirects which reject + * non-browser requests), so the file is not pre-selected for automatic download and the user is not shown a + * download error on every save. */ public function __construct( string $url, public ?string $name = null, + public bool $downloadable = true, ) { //Find all occurrences of non URL safe characters and replace them with their URL encoded version. //We only want to replace characters which can not have a valid meaning in a URL (what would break the URL). diff --git a/src/Services/InfoProviderSystem/DTOs/PartDetailDTO.php b/src/Services/InfoProviderSystem/DTOs/PartDetailDTO.php index 99bf07ee7..68408860a 100644 --- a/src/Services/InfoProviderSystem/DTOs/PartDetailDTO.php +++ b/src/Services/InfoProviderSystem/DTOs/PartDetailDTO.php @@ -102,4 +102,22 @@ public function __construct( gtin: $gtin ); } + + /** + * Returns the URLs of all files of this part which are known to not be downloadable by the server + * (see FileDTO::$downloadable), so the part form can avoid pre-selecting them for download. + * @return string[] + */ + public function getNonDownloadableFileUrls(): array + { + $urls = []; + + foreach ([...($this->datasheets ?? []), ...($this->images ?? [])] as $file) { + if ($file instanceof FileDTO && !$file->downloadable) { + $urls[] = $file->url; + } + } + + return array_values(array_unique($urls)); + } } diff --git a/src/Services/InfoProviderSystem/Providers/TrustedPartsProvider.php b/src/Services/InfoProviderSystem/Providers/TrustedPartsProvider.php index 43bc7f0c7..39ec2065e 100644 --- a/src/Services/InfoProviderSystem/Providers/TrustedPartsProvider.php +++ b/src/Services/InfoProviderSystem/Providers/TrustedPartsProvider.php @@ -315,8 +315,13 @@ private function partResultToDTO(array $part): PartDetailDTO if (strcasecmp((string) ($link['Type'] ?? ''), 'Datasheet') === 0) { //Every distributor links its own copy of the datasheet, so we name them after the //distributor. The URL is used as key to filter out duplicates. + //These are tracking redirects on trustedparts.com itself, not a link to the actual file: + //they only resolve for an actual browser and 403 for anything else (and the API terms of use + //prohibit scraping/downloading from the TrustedParts site anyway), so a local copy can never + //be downloaded. $datasheets[$url] = new FileDTO($url, - $distributor_name === '' ? 'Datasheet' : 'Datasheet ('.$distributor_name.')'); + $distributor_name === '' ? 'Datasheet' : 'Datasheet ('.$distributor_name.')', + downloadable: false); } elseif ($product_url === null) { //The link to the offer is either of type "Buy" (orderable) or "View" $product_url = $url; diff --git a/tests/Services/InfoProviderSystem/DTOs/FileDTOTest.php b/tests/Services/InfoProviderSystem/DTOs/FileDTOTest.php index fe563fb18..55b6e55e0 100644 --- a/tests/Services/InfoProviderSystem/DTOs/FileDTOTest.php +++ b/tests/Services/InfoProviderSystem/DTOs/FileDTOTest.php @@ -47,4 +47,16 @@ public function testURLEscaping(string $expected, string $input): void $fileDTO = new FileDTO( $input); self::assertSame($expected, $fileDTO->url); } + + public function testDownloadableDefaultsToTrue(): void + { + $fileDTO = new FileDTO('https://example.com/datasheet.pdf'); + self::assertTrue($fileDTO->downloadable); + } + + public function testDownloadableCanBeDisabled(): void + { + $fileDTO = new FileDTO('https://example.com/redirect?id=1234', downloadable: false); + self::assertFalse($fileDTO->downloadable); + } } diff --git a/tests/Services/InfoProviderSystem/DTOs/PartDetailDTOTest.php b/tests/Services/InfoProviderSystem/DTOs/PartDetailDTOTest.php new file mode 100644 index 000000000..03dfbf08d --- /dev/null +++ b/tests/Services/InfoProviderSystem/DTOs/PartDetailDTOTest.php @@ -0,0 +1,84 @@ +. + */ +namespace App\Tests\Services\InfoProviderSystem\DTOs; + +use App\Services\InfoProviderSystem\DTOs\FileDTO; +use App\Services\InfoProviderSystem\DTOs\PartDetailDTO; +use PHPUnit\Framework\TestCase; + +final class PartDetailDTOTest extends TestCase +{ + private static function dtoWithFiles(?array $datasheets, ?array $images): PartDetailDTO + { + return new PartDetailDTO( + provider_key: 'test', + provider_id: '1234', + name: 'Test part', + description: 'A part', + datasheets: $datasheets, + images: $images, + ); + } + + public function testNoFilesGivesEmptyList(): void + { + $this->assertSame([], self::dtoWithFiles(null, null)->getNonDownloadableFileUrls()); + } + + public function testOnlyDownloadableFilesGivesEmptyList(): void + { + $dto = self::dtoWithFiles( + [new FileDTO('https://example.com/datasheet.pdf')], + [new FileDTO('https://example.com/image.png')] + ); + + $this->assertSame([], $dto->getNonDownloadableFileUrls()); + } + + public function testNonDownloadableFilesAreCollectedFromDatasheetsAndImages(): void + { + $dto = self::dtoWithFiles( + [ + new FileDTO('https://example.com/datasheet.pdf'), + new FileDTO('https://example.com/redirect?id=1', downloadable: false), + ], + [new FileDTO('https://example.com/redirect?id=2', downloadable: false)] + ); + + $this->assertSame([ + 'https://example.com/redirect?id=1', + 'https://example.com/redirect?id=2', + ], $dto->getNonDownloadableFileUrls()); + } + + public function testDuplicateUrlsAreReturnedOnce(): void + { + //The same file can be listed as datasheet and image, but the form only needs the URL once + $dto = self::dtoWithFiles( + [new FileDTO('https://example.com/redirect?id=1', downloadable: false)], + [new FileDTO('https://example.com/redirect?id=1', downloadable: false)] + ); + + $this->assertSame(['https://example.com/redirect?id=1'], $dto->getNonDownloadableFileUrls()); + } +} diff --git a/tests/Services/InfoProviderSystem/Providers/TrustedPartsProviderTest.php b/tests/Services/InfoProviderSystem/Providers/TrustedPartsProviderTest.php index 5126c24a5..718a7654e 100644 --- a/tests/Services/InfoProviderSystem/Providers/TrustedPartsProviderTest.php +++ b/tests/Services/InfoProviderSystem/Providers/TrustedPartsProviderTest.php @@ -183,6 +183,8 @@ public function testSearchByKeywordReturnsMappedResults(): void $this->assertCount(1, $result->datasheets); $this->assertSame('https://www.trustedparts.com/productredirect?id=datasheet', $result->datasheets[0]->url); $this->assertSame('Datasheet (DigiKey)', $result->datasheets[0]->name); + //The datasheet links are tracking redirects on trustedparts.com, which always reject non-browser requests + $this->assertFalse($result->datasheets[0]->downloadable); $this->assertSame(1, $httpClient->getRequestsCount()); }