refactor(ads-client): type ad URLs as URLs instead of strings - #7614
Merged
Merged
Conversation
Almaju
force-pushed
the
ads-client-newtypes
branch
from
September 22, 2026 15:20
d5cda93 to
969652f
Compare
`url` and `image_url` on `AdImage`, `AdSpoc` and `AdTile`, and on their `MozAds*` counterparts, were `String`. They come straight out of the MARS response body, so nothing stopped a malformed value reaching the caller. They are now `Url` on the wire types and `AdsClientUrl` on the uniffi records, which parses them once where they enter the component. Both types were already in use next door: `AdCallbacks` and `MozAdsCallbacks` have typed their three URL fields this way from the start. The one behaviour change is that a malformed URL now fails deserialization, so the request errors instead of handing the caller a string that is not a URL. Covered by a new test. No binding change. `AdsClientUrl` is a uniffi `custom_type!` that lowers to `String` and is already part of the generated bindings through `MozAdsCallbacks`, so the fields still cross the FFI as strings. Test fixtures use the existing `url!` macro, which validates at compile time, so there are no new unwraps. The two contract-test assertions that checked these fields were not blank now check `has_host()`, since a parsed `Url` cannot be empty. `fmt`, `clippy --all-targets -D warnings` and 106 unit tests are green.
Almaju
force-pushed
the
ads-client-newtypes
branch
from
September 22, 2026 15:34
969652f to
6e67be9
Compare
Member
|
I think this makes sense and fits with the callback urls already. Good to have consistency for things that must be valid to be usable for clients. |
Keep url!() macro for typed URL fields in AdTile test fixtures (from the ads-client URL typing refactor), while adopting the "tile" format string from main. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0174ZP6xEv5z8MkdQv1a2387
thesuzerain
approved these changes
Sep 24, 2026
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
4 participants
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.
urlandimage_urlon the image, spoc and tile ad types wereString. They come straight out of the MARS response, so nothing stopped a malformed value reaching the caller.They're now
Urlon the wire types andAdsClientUrlon the uniffi records.AdCallbacksandMozAdsCallbacksalready type their URL fields that way, so this applies the same choice to the ones that were missed.One behaviour change: a malformed URL now fails deserialization instead of being passed through. There's a test for it.
No binding change.
AdsClientUrllowers toStringand is already in the bindings viaMozAdsCallbacks.Pull Request checklist
fmtandclippy --all-targets -- -D warningsclean, 106 unit tests pass.### Ads-Clientin v158.0, since the failure behaviour changes.