Fix duplicated attachments and unnecessary download attempts for tracker links

src/Form/AttachmentFormType.php

@@ -174,16 +174,20 @@ 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();
 
                 if (!$attachment instanceof Attachment && $attachment !== null) {
                     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

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,
         ]);

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

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).

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));
+    }
 }

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;

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

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);
+    }
 }

tests/Services/InfoProviderSystem/DTOs/PartDetailDTOTest.php

@@ -0,0 +1,84 @@
+<?php
+
+declare(strict_types=1);
+
+/*
+ * This file is part of Part-DB (https://github.com/Part-DB/Part-DB-symfony).
+ *
+ *  Copyright (C) 2019 - 2024 Jan Böhmer (https://github.com/jbtronics)
+ *
+ *  This program is free software: you can redistribute it and/or modify
+ *  it under the terms of the GNU Affero General Public License as published
+ *  by the Free Software Foundation, either version 3 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU Affero General Public License for more details.
+ *
+ *  You should have received a copy of the GNU Affero General Public License
+ *  along with this program.  If not, see <https://www.gnu.org/licenses/>.
+ */
+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());
+    }
+}

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