Fix duplicated attachments and unnecessary download attempts for tracker links - #1531
Open
killecaptron wants to merge 2 commits into
Open
Fix duplicated attachments and unnecessary download attempts for tracker links#1531killecaptron wants to merge 2 commits into
killecaptron wants to merge 2 commits into
Conversation
…anged 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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1531 +/- ##
============================================
- Coverage 62.39% 62.38% -0.02%
- Complexity 9879 9885 +6
============================================
Files 736 736
Lines 31779 31792 +13
============================================
+ Hits 19829 19833 +4
- Misses 11950 11959 +9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two issues which both surface when a part is updated from an info provider that serves its
files through tracking redirects, as TrustedParts does for every datasheet.
First, attachments were duplicated on every update. The merger only considered two attachments
identical if their external path matched as well, so a provider which issues a fresh signed or
tracking URL for the same file on every request produced a second attachment with the same name
and attachment type - which cannot be persisted at all, because PartAttachment enforces that
name and type are unique per part, so the update failed. Name and attachment type now decide,
which is exactly what that constraint already says, and a changed external URL refreshes the
existing attachment instead of adding a colliding one, so a stale or expired link does not
linger either.
Second, "download to server" was pre-selected for files which can never be downloaded. Those
tracking redirects only resolve for an actual browser and reject anything else (and the
TrustedParts terms of use prohibit downloading from their site anyway), so with
app.attachments.download_by_default enabled, every save ended in a download error. FileDTO
gained a downloadable flag which TrustedPartsProvider sets to false for its datasheet links,
and it is passed to AttachmentFormType as a form option, so all that changes is the default
state of a checkbox - nothing about it is persisted.
One thing worth pointing out for the review: the merge change applies to merging two parts as
well, not only to provider updates. Two attachments which share name and attachment type are
now merged into one, and the target's URL is refreshed from the other. Given the unique
constraint the previous behaviour could not have produced a valid result there either, but it
is a deliberate change rather than a side effect.