Skip to content

feat: Adds secondary sqlite db to ads-client - #7567

Draft
thesuzerain wants to merge 11 commits into
mainfrom
ads-client-adds-secondary-sqlite-db
Draft

feat: Adds secondary sqlite db to ads-client#7567
thesuzerain wants to merge 11 commits into
mainfrom
ads-client-adds-secondary-sqlite-db

Conversation

@thesuzerain

@thesuzerain thesuzerain commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

This is a draft PR, and not yet ready for review.

This adds an 'adstore' equivalent to the to be used for storing ads in the new stateful ads-client design. It does not provide any FFI or interfaces or hooks or anything, simply the sqlite queries and internal logic, mimicking a lot of the logic for the http_cache. This uses a separate sqlite db to the http_cache one, though they are functionally quite similar. The tests are all similar/the same, except for removed ones (such as TTL tests, see below).

In particular, this does not include:

  • In-memory database fallbac: Waiting on confirmation for this, but this currently simply stores the AdsStore as None if the proper db path is not provided n setp. The in-memory db can be added in another PR after as a default.
  • TTL: Because the TTL strategy will look different for this than the http_cache (we will have a background worker thread updating + clearing) it's not included in this PR
  • Expanded telemetry (eg: http_cache has record_http_cache_outcome for cache hits/misses)

I did, however, keep:

  • bytesize (as I imagine we will be keeping the max_size part)
  • clock + stored_at (as this will almost certainly be relevant to TTL later)

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.

pub placement_id: PlacementId,
pub ad_type: StorableAdType,
pub ad_body: Vec<u8>,
}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This struct isn't strictly needed at this moment, but it makes the test generation a bit cleaner, and I could see it being useful for the next PR in the process (which will likely utilize conversions from AdImage, etc, to a more generic structure- this one)

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