Skip to content

Allow duplicate assets on the manifest#428

Merged
JackLewis-digirati merged 14 commits intodevelopfrom
fix/duplicateAssetsOnManifest
Jul 24, 2025
Merged

Allow duplicate assets on the manifest#428
JackLewis-digirati merged 14 commits intodevelopfrom
fix/duplicateAssetsOnManifest

Conversation

@JackLewis-digirati
Copy link
Copy Markdown
Collaborator

@JackLewis-digirati JackLewis-digirati commented Jul 10, 2025

Resolves #392

This PR makes it so that the DLCS coordinator is modified so that assets with the same asset id are only ingested into the DLCS once

@JackLewis-digirati JackLewis-digirati force-pushed the fix/duplicateAssetsOnManifest branch from 1f52b44 to c597c15 Compare July 21, 2025 15:49
@JackLewis-digirati JackLewis-digirati force-pushed the fix/duplicateAssetsOnManifest branch from 7e5cfb7 to bd01718 Compare July 22, 2025 09:06
@JackLewis-digirati JackLewis-digirati marked this pull request as ready for review July 22, 2025 13:00
Copy link
Copy Markdown
Member

@donaldgray donaldgray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When running locally fetching asset details is failing, happening on POST/PUT and GET

"asset": {
   "@id": "https://dlcs.host/customers/7/spaces/144/images/dup_test2",
   "error": "Unable to retrieve asset details"
}

The /allImages endpoint rejects duplicates with a 400 response.

Thought, may or may not be viable - is this duplicate check the type of thing we could do in the validator? Before it gets further into the code? Or is it okay to do further dowm?

Comment thread src/IIIFPresentation/Services/Manifests/ManifestMerger.cs Outdated
Comment thread src/IIIFPresentation/Services.Tests/Manifests/Helpers/ManifestMergerTests.cs Outdated
Comment thread src/IIIFPresentation/API/Features/Manifest/DlcsManifestCoordinator.cs Outdated
@JackLewis-digirati
Copy link
Copy Markdown
Collaborator Author

I could swap it to do the duplicate check in the validator, but it does mean interrogating the JObject within the validator as well as in code, which I think would mean duplicating the assetId retrieval, so I felt it would be more work than if we do it at the point we already know the assetId

Copy link
Copy Markdown
Member

@donaldgray donaldgray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Single comment on how de-dup works. Also - should description be changed as there are now no changes to ManifestMerger?

Comment thread src/IIIFPresentation/DLCS/API/DlcsApiClient.cs Outdated
@JackLewis-digirati JackLewis-digirati merged commit b015b01 into develop Jul 24, 2025
4 checks passed
@JackLewis-digirati JackLewis-digirati deleted the fix/duplicateAssetsOnManifest branch July 24, 2025 12:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Duplicate assets on Manifest

2 participants