refactor: Plumb HTTP resolver through internals #1513
Merged
Conversation
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
ok-nick
added
the
check-release
scouten-adobe
approved these changes
Oct 28, 2025
Collaborator
scouten-adobe
left a comment
scouten-adobe
left a comment
There was a problem hiding this comment.
A couple things to think about, but overall very happy to see this! 👍
| #[cfg(not(target_arch = "wasm32"))] | ||
| pub fn from_stream(format: &str, stream: impl Read + Seek + Send) -> Result<Reader> { | ||
| let settings = crate::settings::get_settings().unwrap_or_default(); | ||
| let http_resolver = if _sync { |
Collaborator
There was a problem hiding this comment.
Hmmm. I'm seeing this pattern a lot. Worth adding http_resolver and async_http_resolver accessors to settings?
Not a blocker for this PR; just a thought to explore.
Contributor
Author
There was a problem hiding this comment.
I considered that approach in my ReaderBuilder PR (#1465), although I wasn't a fan for a few reasons:
- Settings is likely to be deserialized from something, we'd either have to mark the resolver(s) as optional or default them
- If we default them, we may be initializing an HTTP client the user will override anyways
- If it's optional, we'd have settings represent an intermediate state when it should actually be a final state (with the resolver initialized)
- We'd either have to add a generic or a boxed trait object to the settings struct
- The generic would be odd when deserializing from serde
- The boxed trait object works, but can run us into problems with Send/Sync and WASM again (and other object safety issues). Ideally we wouldn't have to heap allocate and can design an API that allows for generics (in this case separate from settings)
Collaborator
There was a problem hiding this comment.
I'm thinking we add a C2paContext that can contain both, and it is the context that we pass everywhere.
Contributor
There was a problem hiding this comment.
That makes sense, I have a feeling the context could extend in the future to hold more such objects/properties too.
scouten-adobe
changed the title
…s into ok-nick/pipe-http-resolvers
gpeacock
reviewed
Nov 4, 2025
| // sign and write our store to to the output image file | ||
| if _sync { | ||
| store.save_to_stream(&format, source, dest, signer, &settings) | ||
| store.save_to_stream(&format, source, dest, signer, &http_resolver, &settings) |
Collaborator
There was a problem hiding this comment.
we could create the resolvers here and and simplify the code a bit.
Contributor
There was a problem hiding this comment.
Relating to this comment, I wonder if the http_resolver and settings here could go into a context. Since they are both needed for the execution here, and both tweak the execution.
gpeacock
approved these changes
Nov 5, 2025
| #[cfg(not(target_arch = "wasm32"))] | ||
| pub fn from_stream(format: &str, stream: impl Read + Seek + Send) -> Result<Reader> { | ||
| let settings = crate::settings::get_settings().unwrap_or_default(); | ||
| let http_resolver = if _sync { |
Collaborator
There was a problem hiding this comment.
I'm thinking we add a C2paContext that can contain both, and it is the context that we pass everywhere.
ok-nick
added a commit
that referenced
this pull request
Nov 6, 2025
* feat: sync/async http resolvers API * fix: impl wasi sync/async http resolvers * fix: clean up http dependencies * fix: clean up resolver error types and unwraps * fix: use async generic resolver for cawg * fix: wasi tests and compilation * refactor: remove curl as sync http resolver * fix: simultaneous features, lints, and ci * fix: finish wasi and wasm impl * ci: fix wasm features * fix: wasm issues again * fix: wasm issues again * test: add generic http resolver tests * chore: clean up unnecessary changes * fix: do not expose http resolver in `Ingredient::from_stream_async` * docs: document new feature flags and WASM/WASI notes * fix: default to reqwest_blocking if ureq is also specified * fix: simplify feature flags for generic resolvers * ci: fix benchmarking to use proper http featuers * ci: define env before executing command for benchmarking * style: alphabetize http crate * ci: fix rust native crypto jobs * ci: revert rust_native_crypto changes for now * docs: simplify doc references * test: individual networking lib tests * build: remove wasi dep from fetch_remote_manifests feature * docs: add new features to usage.md * fix: use concrete resolver type instead of trait object * refactor: modularize http resolvers * fix: resolver imports * fix: wasm tests * fix: reqwest_blocking import issues * docs: remove comment stating http dependency can be optional * docs: clarify why we use certain http features in ci * docs: fix grammar * ci: add http features to release readiness * ci: run benchmarks with minimal featuers * fix: dependencies and test caused by merge * fix: allow timestamp assertion to be fetched on WASM * fix: support timestamping on WASM/WASI * fix: remove forbidden HOST header for WASI networking * docs: remove review note * fix: use write_all for WASI to bypass 4096 byte limit * fix: pipe http resolvers through timestamping * fix: default to ureq instead of reqwest_blocking until async tests are fixed * fix: WASM http resolver for timestamps * ci: use all features instead of excluding reqwest_blocking since we default to ureq * feat: add `default_http` feature for simplification * fix: add default_http as a feature of ffi and make test images * feat: add networking feature flag for c2patool * Capitalize HTTP consistently * Capitalize HTTP consistently * fix: add back http crate and fix mut variable * refactor: Plumb HTTP resolver through internals (#1513) * fix: pass http resolver to archive code, fix clippy lint, allow more timestamp fuctions for wasm * style: fix formatting to nightly --------- Co-authored-by: Eric Scouten <[email protected]>
ribhavh
pushed a commit
to ribhavh/c2pa-rs
that referenced
this pull request
Nov 11, 2025
* feat: sync/async http resolvers API * fix: impl wasi sync/async http resolvers * fix: clean up http dependencies * fix: clean up resolver error types and unwraps * fix: use async generic resolver for cawg * fix: wasi tests and compilation * refactor: remove curl as sync http resolver * fix: simultaneous features, lints, and ci * fix: finish wasi and wasm impl * ci: fix wasm features * fix: wasm issues again * fix: wasm issues again * test: add generic http resolver tests * chore: clean up unnecessary changes * fix: do not expose http resolver in `Ingredient::from_stream_async` * docs: document new feature flags and WASM/WASI notes * fix: default to reqwest_blocking if ureq is also specified * fix: simplify feature flags for generic resolvers * ci: fix benchmarking to use proper http featuers * ci: define env before executing command for benchmarking * style: alphabetize http crate * ci: fix rust native crypto jobs * ci: revert rust_native_crypto changes for now * docs: simplify doc references * test: individual networking lib tests * build: remove wasi dep from fetch_remote_manifests feature * docs: add new features to usage.md * fix: use concrete resolver type instead of trait object * refactor: modularize http resolvers * fix: resolver imports * fix: wasm tests * fix: reqwest_blocking import issues * docs: remove comment stating http dependency can be optional * docs: clarify why we use certain http features in ci * docs: fix grammar * ci: add http features to release readiness * ci: run benchmarks with minimal featuers * fix: dependencies and test caused by merge * fix: allow timestamp assertion to be fetched on WASM * fix: support timestamping on WASM/WASI * fix: remove forbidden HOST header for WASI networking * docs: remove review note * fix: use write_all for WASI to bypass 4096 byte limit * fix: pipe http resolvers through timestamping * fix: default to ureq instead of reqwest_blocking until async tests are fixed * fix: WASM http resolver for timestamps * ci: use all features instead of excluding reqwest_blocking since we default to ureq * feat: add `default_http` feature for simplification * fix: add default_http as a feature of ffi and make test images * feat: add networking feature flag for c2patool * Capitalize HTTP consistently * Capitalize HTTP consistently * fix: add back http crate and fix mut variable * refactor: Plumb HTTP resolver through internals (contentauth#1513) * fix: pass http resolver to archive code, fix clippy lint, allow more timestamp fuctions for wasm * style: fix formatting to nightly --------- Co-authored-by: Eric Scouten <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This change builds off #1355 and #1444 to plumb http resolvers throughout all the internals. It also fixes a few issues with the settings plumbing and usage of sync APIs in async APIs.
The last change is removing the
async-recursiondependency in favor of pinning the return ofStore::ingredient_checks_async. The dependency had conflicts with usage of the http resolver traits thus leading to this change.