Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes DISCO-2637: Replace SuggestionQuery options with providers. [firefox-android: linabutler/DISCO-2637] [firefox-ios: linabutler/DISCO-2637] #5867

Merged
merged 1 commit into from Oct 13, 2023

Conversation

linabutler
Copy link
Contributor

@linabutler linabutler commented Oct 13, 2023

This commit removes the include_sponsored and include_non_sponsored options, and replaces them with a list of providers. This lets consumers request specific suggestion types: AMP and Wikipedia only on mobile; and AMP, Wikipedia, and active experiments on Desktop.

This is a backward-incompatible API change.

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 dilligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

@linabutler linabutler requested a review from a team October 13, 2023 04:23
@codecov-commenter
Copy link

codecov-commenter commented Oct 13, 2023

Codecov Report

Attention: 32 lines in your changes are missing coverage. Please review.

Comparison is base (fe621c9) 36.72% compared to head (c22e000) 36.71%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5867      +/-   ##
==========================================
- Coverage   36.72%   36.71%   -0.01%     
==========================================
  Files         348      348              
  Lines       33532    33541       +9     
==========================================
  Hits        12313    12313              
- Misses      21219    21228       +9     
Files Coverage Δ
components/suggest/src/suggestion.rs 0.00% <ø> (ø)
components/suggest/src/lib.rs 0.00% <0.00%> (ø)
components/suggest/src/store.rs 0.00% <0.00%> (ø)
components/suggest/src/db.rs 0.00% <0.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

linabutler added a commit to linabutler/firefox-android that referenced this pull request Oct 13, 2023
@linabutler linabutler changed the title Fixes DISCO-2637: Replace SuggestionQuery options with providers. Fixes DISCO-2637: Replace SuggestionQuery options with providers. [firefox-android: linabutler/DISCO-2637] Oct 13, 2023
:url
)
RETURNING id",
SuggestionProvider::Amo as u8
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The SuggestionProvider variants are all integers, so, as with providers_to_sql_list above, we can safely interpolate them directly into the query string, without worrying about SQL injection. This is a trick we use in Places, too! ✨

pub fn fetch_suggestions(&self, query: &SuggestionQuery) -> Result<Vec<Suggestion>> {
let (keyword_prefix, keyword_suffix) = split_keyword(&query.keyword);

let (mut statement, params) = if query.providers.contains(&SuggestionProvider::Pocket) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pocket keywords use a separate query, so, if we aren't matching Pocket suggestions, we can skip querying pocket_keywords entirely.

That also means we end up with a dynamically-sized list of params—two for Pocket; one for non-Pocket—so we have to use a Vec<(&str, &dyn ToSql)> to hold them.

@@ -258,7 +279,9 @@ impl<'a> SuggestDao<'a> {
}
}
}
)?.into_iter().flatten().collect())
)?.flat_map(Result::transpose).collect::<Result<_>>()?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This transformation filters out the Nones from our query_and_then(...) iterator, then accumulates the remaining suggestions into a vector. It leans on a few iterator helpers to do this:

  • transpose turns a Result<Option<Suggestion>> into an Option<Result<Suggestion>>.
  • An Option is an iterator—it yields zero elements if it's None, and one if it's Some—so we can use it with flat_map to unwrap optionals. This turns our iterator of Option<Result<Suggestion>> into an iterator of Result<Suggestion>.
  • collect::<Result<_>> accumulates the iterator of Result<Suggestion> into a Result<Vec<Suggestion>>.

linabutler added a commit to linabutler/firefox-ios that referenced this pull request Oct 13, 2023
This commit fixes the breaking change introduced in
mozilla/application-services#5867.
@linabutler linabutler changed the title Fixes DISCO-2637: Replace SuggestionQuery options with providers. [firefox-android: linabutler/DISCO-2637] Fixes DISCO-2637: Replace SuggestionQuery options with providers. [firefox-android: linabutler/DISCO-2637] [firefox-ios: linabutler/DISCO-2637] Oct 13, 2023
Copy link
Member

@ncloudioj ncloudioj left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

I wonder if we could use a "cheaper" construct for SuggestionQuery.providers. Creating a new vector (despite the providers mostly remaining the same) for each query seems a bit wasteful to me. Would a bitflag fit the bill? I am thinking of this:

use bitflags::bitflags;

bitflags! {
    pub struct Providers: u32 {
        const Amp = 1;
        const Wikipedia = 1 << 1;
        const Pocket = 1 << 2;
        const Amo = 1 << 3;
    }
}

pub struct SuggestionQuery {
    pub keyword: String,
    pub providers: Providers, 
}

// For mobile
let query = SuggestionQuery {
    keyword: "foo".to_own(),
    providers = Providers::Amp | Providers::Wikipedia,
};

if query.providers.contains(Providers::Pocket) {
} else ...

WDYT?

@linabutler
Copy link
Contributor Author

Thanks for the review!

I thought bitflags—or even a set—would be perfect here, too! Unfortunately, they're tricky to do with UniFFI. Neither UDL nor UniFFI's serialization format supports custom values for fieldless enums, so I don't think it would be possible today for Swift, Kotlin, or JS to pass a bitmask, and have Rust see the same value.

We could use a custom type to work around that, but then we couldn't use the enum syntax in UDL, because we can't declare a custom value for each variant.

@ncloudioj
Copy link
Member

Bummer. Yep, the FFI limitations! If we use an opaque u32, then we'd sacrifice the type safety 🤷‍♂️. let's leave it as is then.

This commit removes the `include_sponsored` and `include_non_sponsored`
options, and replaces them with a list of `providers`. This lets
consumers request specific suggestion types: AMP and Wikipedia only on
mobile; and AMP, Wikipedia, and active experiments on Desktop.

This is a breaking change.
@linabutler
Copy link
Contributor Author

PRs resolving the breaking changes for our consumers:

The process for landing breaking changes like this is roughly:

  • Land the Application Services PR first.
  • Wait for the automated a-s version bumps in Firefox for Android and iOS to fail. (I'll drop links to those failing PRs here when that happens, so that folks can follow the process along).
  • Fold in the Android and iOS PRs that fix the breakages into the version bump PRs, push, and land that as a squashed commit. This keeps the version bump, and the fix for the breakages, in a single commit.

@linabutler linabutler added this pull request to the merge queue Oct 13, 2023
Merged via the queue into main with commit 04f012e Oct 13, 2023
18 checks passed
@linabutler linabutler deleted the DISCO-2637-query-providers branch October 13, 2023 21:15
linabutler added a commit to mozilla-mobile/firefox-android that referenced this pull request Oct 14, 2023
dsmithpadilla pushed a commit to mozilla-mobile/firefox-ios that referenced this pull request Oct 15, 2023
mergify bot added a commit to mozilla-mobile/firefox-android that referenced this pull request Oct 16, 2023
* Update A-S to 120.20231014050328.

* No bug - Update uses of `SuggestionQuery` in Firefox Suggest.

This commit fixes the breaking change introduced in
mozilla/application-services#5867.

* Fixup build breaking from Nimbus pref-key addition

---------

Co-authored-by: MickeyMoz <sebastian@mozilla.com>
Co-authored-by: Ryan VanderMeulen <rvandermeulen@mozilla.com>
Co-authored-by: Lina Butler <lina@yakshaving.ninja>
Co-authored-by: James Hugman <james@hugman.tv>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
lmarceau pushed a commit to mozilla-mobile/firefox-ios that referenced this pull request Oct 17, 2023
…1014050328 (#16840)

* Auto update SPM with latest rust-component release 120.0.20231014050328

* Bump [v120] Update uses of `SuggestionQuery`. (#16818)

This commit fixes the breaking change introduced in
mozilla/application-services#5867.

* Fixup build breaking from Nimbus pref-key addition

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Lina Butler <lina@yakshaving.ninja>
Co-authored-by: James Hugman <james@hugman.tv>
pmarks-net pushed a commit to pmarks-net/firefox-android that referenced this pull request Oct 24, 2023
* Update A-S to 120.20231014050328.

* No bug - Update uses of `SuggestionQuery` in Firefox Suggest.

This commit fixes the breaking change introduced in
mozilla/application-services#5867.

* Fixup build breaking from Nimbus pref-key addition

---------

Co-authored-by: MickeyMoz <sebastian@mozilla.com>
Co-authored-by: Ryan VanderMeulen <rvandermeulen@mozilla.com>
Co-authored-by: Lina Butler <lina@yakshaving.ninja>
Co-authored-by: James Hugman <james@hugman.tv>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
akliuxingyuan pushed a commit to fork-maintainers/iceraven-browser that referenced this pull request Nov 22, 2023
* Update A-S to 120.20231014050328.

* No bug - Update uses of `SuggestionQuery` in Firefox Suggest.

This commit fixes the breaking change introduced in
mozilla/application-services#5867.

* Fixup build breaking from Nimbus pref-key addition

---------

Co-authored-by: MickeyMoz <sebastian@mozilla.com>
Co-authored-by: Ryan VanderMeulen <rvandermeulen@mozilla.com>
Co-authored-by: Lina Butler <lina@yakshaving.ninja>
Co-authored-by: James Hugman <james@hugman.tv>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
akliuxingyuan pushed a commit to akliuxingyuan/android-components that referenced this pull request Nov 22, 2023
* Update A-S to 120.20231014050328.

* No bug - Update uses of `SuggestionQuery` in Firefox Suggest.

This commit fixes the breaking change introduced in
mozilla/application-services#5867.

* Fixup build breaking from Nimbus pref-key addition

---------

Co-authored-by: MickeyMoz <sebastian@mozilla.com>
Co-authored-by: Ryan VanderMeulen <rvandermeulen@mozilla.com>
Co-authored-by: Lina Butler <lina@yakshaving.ninja>
Co-authored-by: James Hugman <james@hugman.tv>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
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.

None yet

3 participants