fix: add missing remote url #1598
Conversation
Add unit test to make sure external manifests work with BMFF files.
…x/missing_remote_url
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1598 +/- ##
==========================================
+ Coverage 77.97% 78.00% +0.02%
==========================================
Files 165 165
Lines 41433 41441 +8
==========================================
+ Hits 32308 32324 +16
+ Misses 9125 9117 -8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CodSpeed Performance ReportMerging #1598 will degrade performances by 12.06%Comparing Summary
Benchmarks breakdown
Footnotes
|
|
|
||
| let mut builder = Builder::from_json(manifest_json)?; | ||
| builder.set_intent(BuilderIntent::Edit); | ||
| builder.definition.claim_version = Some(2); // use v1 for this test |
| // fetch remote manifest if available | ||
| if let Some(ext_ref) = | ||
| crate::utils::xmp_inmemory_utils::XmpInfo::from_source(&mut stream, format).provenance | ||
| { | ||
| store.set_remote_url(&ext_ref); | ||
| } |
There was a problem hiding this comment.
The docs for Reader::remote_url is defined as:
Returns the remote url of the manifest if this Reader obtained the manifest remotely.
Here we are changing that definition but also the behavior that exists in other parts of the code.
There was a problem hiding this comment.
The problem is that no remote_url is returned when using this path and Tania reported an error. What do you want here?
There was a problem hiding this comment.
The current behavior is to ignore remote_url if the manifest is found as embedded first. Here we are always setting the remote_url. It's an unusual situation though because we aren't actually looking for an embedded manifest anyways.
If we are going to make this change here, we should probably make that change in the other constructors and change the reader docs to state that it's always returned regardless of how the manifest was obtained.
There was a problem hiding this comment.
(For context, on the error, if it's the one I am thinking of( #1532): The reported issue is that in certain cases you can "keep" and outdated remote URL manifest. Even if our SDK does not look at it, it still could be found. For instance, even if our SDK does the right thing, you could still "find" URLs by using exiftool. More details in #1532).
Changes in this pull request
Fix missing remote_url when loading via from_manifest_data_and_stream
Checklist
TO DOitems (or similar) have been entered as GitHub issues and the link to that issue has been included in a comment.