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/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/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/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())); 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()); }