Repository navigation
AdsClient - refactors request-level sendable options to be client-level - #7743
thesuzerain wants to merge 17 commits into
Conversation
…into Ads-client-refactors-sendable-options
| store_config: Option<MozAdsStoreConfig>, | ||
| telemetry: Option<Arc<dyn MozAdsTelemetry>>, | ||
| user_agent: Option<NonEmptyString>, | ||
| request_options: Option<MozAdsRequestConfigOptions>, |
There was a problem hiding this comment.
I think you need a uniffi default here or that might be a breaking change 🤔
There was a problem hiding this comment.
I don't think so, unless I'm missing something or this is on the wrong line. This is a field in MozAdsClientBuilderInner which doesn't cross the uniffi barrier, you explicitly call (eg):
builder().request_options(options).build()
LMK if I have misunderstood!
| let mars_options = options.map(|opt| MARSRequestOptions { | ||
| flags: AdRequestFlags::from(&opt), | ||
| cache_policy: opt.cache_policy.map(CachePolicy::from).unwrap_or_default(), | ||
| ohttp: opt.ohttp, | ||
| blocks: opt.blocks, | ||
| }); |
There was a problem hiding this comment.
You repeat this code a couple of times. We could have a From impl and then do options.map(Into::into) I think.
There was a problem hiding this comment.
My reasoning for doing this was I had mixed feelings about using From<> when some of the fields were getting 'lost' (specifically relevance, which would not carry into the new struct). I liked being able to see it explicitly here, but I'm not tied to it, so happy to change this.
There was a problem hiding this comment.
To clarify, this obviously wouldn't affect the code written already. the separate relevance extraction already happens. I meant more in a 'lets not let this field be forgettable in future refactors' kind of change.
But on reflection, I'm not sure how much of a difference it makes to have the Into
| new_settings: MozAdsRequestConfigOptions, | ||
| ) -> AdsClientApiResult<()> { | ||
| self.inner | ||
| .set_request_options(MARSRequestOptions::from(new_settings)); |
There was a problem hiding this comment.
nit but you don't need the ::from call here if you do fn set_request_options(opts: impl Into<MARSRequestOptions>)
Almaju
left a comment
Almaju
left a comment
There was a problem hiding this comment.
I think there’s a subtle issue with how request options fall back to the configured ones. unwrap_or replaces the whole struct, so if the consumer passes Some(options) with just one field set, all the other fields fall back to their defaults, not to the configured settings.
This is made worse on the FFI side. Because MozAdsRequestConfigOptions uses #[uniffi(default)], a call like { ohttp: true } arrives as { ohttp: true, flags: {}, blocks: [], cache_policy: None }, and the configured flags get erased. I don’t think a consumer would expect that setting one field resets the others.
The root cause is that we can’t distinguish “not provided” from “explicitly empty/false” (e.g. ohttp: bool defaulting to false). What do you think about making the fields optional and merging field by field?
pub struct MozAdsRequestConfigOptions {
#[uniffi(default)]
pub blocks: Option<Vec<String>>,
#[uniffi(default)]
pub cache_policy: Option<MozAdsCachePolicy>,
#[uniffi(default)]
pub flags: Option<HashMap<String, bool>>,
#[uniffi(default)]
pub ohttp: Option<bool>,
}Then each field falls back to the configured value when it’s None.
Also nit: unwrap_or(self.request_options.read().clone()) takes the lock and clones even when options are provided; unwrap_or_else(|| ...) would avoid that.
|
This was something I thought a bit about- I leaned towards allowing it and simply forcing each setting to be set (as you'd have easy access to to all of them when you set it on a surface side, and you'd be able to easily set it back to defaults) but you are correct with uniffi defaults it might be misused. |
…into Ads-client-refactors-sendable-options
Pull Request checklist
[ci full]to the PR title.