Skip to content

[AC-154] creates temporary ads client structure AdsStore - #7552

Draft
thesuzerain wants to merge 7 commits into
mainfrom
AC-154-Creates-temporary-ads-client-structure
Draft

[AC-154] creates temporary ads client structure AdsStore#7552
thesuzerain wants to merge 7 commits into
mainfrom
AC-154-Creates-temporary-ads-client-structure

Conversation

@thesuzerain

@thesuzerain thesuzerain commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

As discussed in this PR, this splits out the temporary ads store out of the PR to reduce its complexity.

This is a temporary structure to be used for the first vertical slice of the fire-and-forget refactoring for ads-client. It will be modified in a later PR to use sqlite (durable) instead.

It is currently unused.

I don't think this needs a changelog entry, but the greater fire-and-forget mechanism will include one.

Pull Request checklist

  • Breaking changes: This PR follows our breaking change policy
    • This PR follows the breaking change policy:
      • This PR has no breaking API changes, or
      • There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
  • Quality: This PR builds and tests run cleanly
    • Note:
      • For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
      • If this pull request includes a breaking change, consider cutting a new release after merging.
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
    • Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
  • Dependencies: This PR follows our dependency management guidelines
    • Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

@thesuzerain

Copy link
Copy Markdown
Collaborator Author

Regarding the comment from the other PR @Almaju :

Do we need the StorageType associated type / three separate maps? It looks like this could just be placement_id -> Vec uniformly. image/tile would simply be a Vec of length 0 or 1, and the unwrap-to-single-ad already happens at the call site (take_first() in client.rs). That removes the trait indirection and the three near-identical impls.

My concern with this is that to store them as Vec<Ad>, because the different ad types seemingly have different fields (eg: spoc call to MARS does not return a alt_text field, but the billboard/image one does), we can't easily create an Ad struct that encompasses all of them. we would need to either:

  • store it as a boxed trait type anyway
  • store it as an enum (Ad), which has variants, and add extra logic to convert + check for each variant
  • store it as a raw string and parse on load from the adsstore
  • create a new Ad struct that stores all of them and all possible fields (as Option<...>s or default values), and can be 'downcast' (using From/Into) into each Ad type (but with the possibility of missing fields if you downcast to the 'wrong' one

Let me know if I've misunderstood your idea.

@thesuzerain
thesuzerain requested a review from Almaju August 18, 2026 16:42
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.

1 participant