From 7f96b236509c6d676802784c6f5be760ec842fbe Mon Sep 17 00:00:00 2001 From: Ben Dean-Kawamura Date: Thu, 23 May 2024 11:06:36 -0400 Subject: [PATCH] Bug 1898509 - more details for SQL errors The goal here is to track down some foreign key failures that we're seeing, but don't have any context on where they're happening. - Record a breadcrumb when we start to ingest different suggestion types. - Record what we were doing when we saw an SQL error. This should change the message from `FOREIGN KEY constraint failed` to something like `FOREIGN KEY constraint failed (context: mdn insert)`, which I think will help greatly. I wanted to also record which field the foreign key error was happening on, but AFAICT this is not possible with SQLite. I added the `extend` crate to help with some of this code. It's not really needed, but I think it's worth the dependency. We've been using it for `uniffi-bindgen-gecko-js` so it's already vetted. --- Cargo.lock | 13 + DEPENDENCIES.md | 29 ++ components/suggest/Cargo.toml | 2 + components/suggest/src/db.rs | 267 ++++++++++-------- components/suggest/src/error.rs | 30 +- components/suggest/src/store.rs | 5 +- megazords/full/DEPENDENCIES.md | 29 ++ .../full/android/dependency-licenses.xml | 4 + megazords/ios-rust/DEPENDENCIES.md | 29 ++ 9 files changed, 281 insertions(+), 127 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index f2f1b9b790..1ed8956965 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1428,6 +1428,17 @@ dependencies = [ "once_cell", ] +[[package]] +name = "extend" +version = "1.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "311a6d2f1f9d60bff73d2c78a0af97ed27f79672f15c238192a5bbb64db56d00" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.32", +] + [[package]] name = "fallible-iterator" version = "0.3.0" @@ -4081,8 +4092,10 @@ dependencies = [ "env_logger", "error-support", "expect-test", + "extend", "hex", "interrupt-support", + "log", "once_cell", "parking_lot", "rc_crypto", diff --git a/DEPENDENCIES.md b/DEPENDENCIES.md index 508a2edb4a..dbb8e8165d 100644 --- a/DEPENDENCIES.md +++ b/DEPENDENCIES.md @@ -12,6 +12,7 @@ the details of which are reproduced below. * [MIT License: bytes](#mit-license-bytes) * [MIT License: cargo_metadata](#mit-license-cargo_metadata) * [MIT License: caseless](#mit-license-caseless) +* [MIT License: extend](#mit-license-extend) * [MIT License: generic-array](#mit-license-generic-array) * [MIT License: goblin](#mit-license-goblin) * [MIT License: h2](#mit-license-h2) @@ -992,6 +993,34 @@ LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. +``` +------------- +## MIT License: extend + +The following text applies to code linked from these dependencies: +[extend](https://github.com/davidpdrsn/extend) + +``` +MIT License Copyright (c) 2020 David Pedersen + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is furnished +to do so, subject to the following conditions: + +The above copyright notice and this permission notice (including the next +paragraph) shall be included in all copies or substantial portions of the +Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS +FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS +OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, +WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF +OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + ``` ------------- ## MIT License: generic-array diff --git a/components/suggest/Cargo.toml b/components/suggest/Cargo.toml index e2ae50fe7a..a92056b623 100644 --- a/components/suggest/Cargo.toml +++ b/components/suggest/Cargo.toml @@ -10,7 +10,9 @@ exclude = ["/android", "/ios"] [dependencies] anyhow = "1.0" chrono = "0.4" +extend = "1.1" interrupt-support = { path = "../support/interrupt" } +log = "0.4" once_cell = "1.5" parking_lot = ">=0.11,<=0.12" remote_settings = { path = "../remote_settings" } diff --git a/components/suggest/src/db.rs b/components/suggest/src/db.rs index 4e12a87969..cf8c42eb60 100644 --- a/components/suggest/src/db.rs +++ b/components/suggest/src/db.rs @@ -16,6 +16,7 @@ use sql_support::{open_database::open_database_with_flags, ConnExt}; use crate::{ config::{SuggestGlobalConfig, SuggestProviderConfig}, + error::RusqliteResultExt, keyword::full_keyword, pocket::{split_keyword, KeywordConfidence}, provider::SuggestionProvider, @@ -679,6 +680,8 @@ impl<'a> SuggestDao<'a> { suggestions: &[DownloadedAmoSuggestion], ) -> Result<()> { let mut suggestion_insert = SuggestionInsertStatement::new(self.conn)?; + let mut amo_insert = AmoInsertStatement::new(self.conn)?; + let mut prefix_keyword_insert = PrefixKeywordInsertStatement::new(self.conn)?; for suggestion in suggestions { self.scope.err_if_interrupted()?; let suggestion_id = suggestion_insert.execute( @@ -688,53 +691,15 @@ impl<'a> SuggestDao<'a> { suggestion.score, SuggestionProvider::Amo, )?; - self.conn.execute( - "INSERT INTO amo_custom_details( - suggestion_id, - description, - guid, - icon_url, - rating, - number_of_ratings - ) - VALUES( - :suggestion_id, - :description, - :guid, - :icon_url, - :rating, - :number_of_ratings - )", - named_params! { - ":suggestion_id": suggestion_id, - ":description": suggestion.description, - ":guid": suggestion.guid, - ":icon_url": suggestion.icon_url, - ":rating": suggestion.rating, - ":number_of_ratings": suggestion.number_of_ratings - }, - )?; + amo_insert.execute(suggestion_id, suggestion)?; for (index, keyword) in suggestion.keywords.iter().enumerate() { let (keyword_prefix, keyword_suffix) = split_keyword(keyword); - self.conn.execute( - "INSERT INTO prefix_keywords( - keyword_prefix, - keyword_suffix, - suggestion_id, - rank - ) - VALUES( - :keyword_prefix, - :keyword_suffix, - :suggestion_id, - :rank - )", - named_params! { - ":keyword_prefix": keyword_prefix, - ":keyword_suffix": keyword_suffix, - ":rank": index, - ":suggestion_id": suggestion_id, - }, + prefix_keyword_insert.execute( + suggestion_id, + None, + keyword_prefix, + keyword_suffix, + index, )?; } } @@ -843,6 +808,7 @@ impl<'a> SuggestDao<'a> { suggestions: &[DownloadedPocketSuggestion], ) -> Result<()> { let mut suggestion_insert = SuggestionInsertStatement::new(self.conn)?; + let mut prefix_keyword_insert = PrefixKeywordInsertStatement::new(self.conn)?; for suggestion in suggestions { self.scope.err_if_interrupted()?; let suggestion_id = suggestion_insert.execute( @@ -866,28 +832,12 @@ impl<'a> SuggestDao<'a> { ) { let (keyword_prefix, keyword_suffix) = split_keyword(keyword); - self.conn.execute( - "INSERT INTO prefix_keywords( - keyword_prefix, - keyword_suffix, - confidence, - rank, - suggestion_id - ) - VALUES( - :keyword_prefix, - :keyword_suffix, - :confidence, - :rank, - :suggestion_id - )", - named_params! { - ":keyword_prefix": keyword_prefix, - ":keyword_suffix": keyword_suffix, - ":confidence": confidence, - ":rank": rank, - ":suggestion_id": suggestion_id, - }, + prefix_keyword_insert.execute( + suggestion_id, + Some(confidence as u8), + keyword_prefix, + keyword_suffix, + rank, )?; } } @@ -902,6 +852,8 @@ impl<'a> SuggestDao<'a> { suggestions: &[DownloadedMdnSuggestion], ) -> Result<()> { let mut suggestion_insert = SuggestionInsertStatement::new(self.conn)?; + let mut mdn_insert = MdnInsertStatement::new(self.conn)?; + let mut prefix_keyword_insert = PrefixKeywordInsertStatement::new(self.conn)?; for suggestion in suggestions { self.scope.err_if_interrupted()?; let suggestion_id = suggestion_insert.execute( @@ -911,41 +863,15 @@ impl<'a> SuggestDao<'a> { suggestion.score, SuggestionProvider::Mdn, )?; - self.conn.execute_cached( - "INSERT INTO mdn_custom_details( - suggestion_id, - description - ) - VALUES( - :suggestion_id, - :description - )", - named_params! { - ":suggestion_id": suggestion_id, - ":description": suggestion.description, - }, - )?; + mdn_insert.execute(suggestion_id, suggestion)?; for (index, keyword) in suggestion.keywords.iter().enumerate() { let (keyword_prefix, keyword_suffix) = split_keyword(keyword); - self.conn.execute_cached( - "INSERT INTO prefix_keywords( - keyword_prefix, - keyword_suffix, - suggestion_id, - rank - ) - VALUES( - :keyword_prefix, - :keyword_suffix, - :suggestion_id, - :rank - )", - named_params! { - ":keyword_prefix": keyword_prefix, - ":keyword_suffix": keyword_suffix, - ":rank": index, - ":suggestion_id": suggestion_id, - }, + prefix_keyword_insert.execute( + suggestion_id, + None, + keyword_prefix, + keyword_suffix, + index, )?; } } @@ -959,6 +885,7 @@ impl<'a> SuggestDao<'a> { data: &DownloadedWeatherData, ) -> Result<()> { let mut suggestion_insert = SuggestionInsertStatement::new(self.conn)?; + let mut keyword_insert = KeywordInsertStatement::new(self.conn)?; self.scope.err_if_interrupted()?; let suggestion_id = suggestion_insert.execute( record_id, @@ -968,15 +895,7 @@ impl<'a> SuggestDao<'a> { SuggestionProvider::Weather, )?; for (index, keyword) in data.weather.keywords.iter().enumerate() { - self.conn.execute( - "INSERT INTO keywords(keyword, suggestion_id, rank) - VALUES(:keyword, :suggestion_id, :rank)", - named_params! { - ":keyword": keyword, - ":suggestion_id": suggestion_id, - ":rank": index, - }, - )?; + keyword_insert.execute(suggestion_id, keyword, None, index)?; } self.put_provider_config( SuggestionProvider::Weather, @@ -1220,10 +1139,12 @@ impl<'conn> SuggestionInsertStatement<'conn> { score: f64, provider: SuggestionProvider, ) -> Result { - Ok(self.0.query_row( - (record_id.as_str(), title, url, score, provider as u8), - |row| row.get(0), - )?) + self.0 + .query_row( + (record_id.as_str(), title, url, score, provider as u8), + |row| row.get(0), + ) + .with_context("suggestion insert") } } @@ -1247,15 +1168,17 @@ impl<'conn> AmpInsertStatement<'conn> { } fn execute(&mut self, suggestion_id: i64, amp: &DownloadedAmpSuggestion) -> Result<()> { - self.0.execute(( - suggestion_id, - &.advertiser, - amp.block_id, - &.iab_category, - &.impression_url, - &.click_url, - &.icon_id, - ))?; + self.0 + .execute(( + suggestion_id, + &.advertiser, + amp.block_id, + &.iab_category, + &.impression_url, + &.click_url, + &.icon_id, + )) + .with_context("amp insert")?; Ok(()) } } @@ -1279,7 +1202,64 @@ impl<'conn> WikipediaInsertStatement<'conn> { suggestion_id: i64, wikipedia: &DownloadedWikipediaSuggestion, ) -> Result<()> { - self.0.execute((suggestion_id, &wikipedia.icon_id))?; + self.0 + .execute((suggestion_id, &wikipedia.icon_id)) + .with_context("wikipedia insert")?; + Ok(()) + } +} + +struct AmoInsertStatement<'conn>(rusqlite::Statement<'conn>); + +impl<'conn> AmoInsertStatement<'conn> { + fn new(conn: &'conn Connection) -> Result { + Ok(Self(conn.prepare( + "INSERT INTO amo_custom_details( + suggestion_id, + description, + guid, + icon_url, + rating, + number_of_ratings + ) + VALUES(?, ?, ?, ?, ?, ?) + ", + )?)) + } + + fn execute(&mut self, suggestion_id: i64, amo: &DownloadedAmoSuggestion) -> Result<()> { + self.0 + .execute(( + suggestion_id, + &amo.description, + &amo.guid, + &amo.icon_url, + &amo.rating, + amo.number_of_ratings, + )) + .with_context("amo insert")?; + Ok(()) + } +} + +struct MdnInsertStatement<'conn>(rusqlite::Statement<'conn>); + +impl<'conn> MdnInsertStatement<'conn> { + fn new(conn: &'conn Connection) -> Result { + Ok(Self(conn.prepare( + "INSERT INTO mdn_custom_details( + suggestion_id, + description + ) + VALUES(?, ?) + ", + )?)) + } + + fn execute(&mut self, suggestion_id: i64, mdn: &DownloadedMdnSuggestion) -> Result<()> { + self.0 + .execute((suggestion_id, &mdn.description)) + .with_context("mdn insert")?; Ok(()) } } @@ -1308,7 +1288,46 @@ impl<'conn> KeywordInsertStatement<'conn> { rank: usize, ) -> Result<()> { self.0 - .execute((suggestion_id, keyword, full_keyword_id, rank))?; + .execute((suggestion_id, keyword, full_keyword_id, rank)) + .with_context("keyword insert")?; + Ok(()) + } +} + +struct PrefixKeywordInsertStatement<'conn>(rusqlite::Statement<'conn>); + +impl<'conn> PrefixKeywordInsertStatement<'conn> { + fn new(conn: &'conn Connection) -> Result { + Ok(Self(conn.prepare( + "INSERT INTO prefix_keywords( + suggestion_id, + confidence, + keyword_prefix, + keyword_suffix, + rank + ) + VALUES(?, ?, ?, ?, ?) + ", + )?)) + } + + fn execute( + &mut self, + suggestion_id: i64, + confidence: Option, + keyword_prefix: &str, + keyword_suffix: &str, + rank: usize, + ) -> Result<()> { + self.0 + .execute(( + suggestion_id, + confidence.unwrap_or(0), + keyword_prefix, + keyword_suffix, + rank, + )) + .with_context("prefix keyword insert")?; Ok(()) } } diff --git a/components/suggest/src/error.rs b/components/suggest/src/error.rs index eb5744dc4a..f99ff3658b 100644 --- a/components/suggest/src/error.rs +++ b/components/suggest/src/error.rs @@ -14,8 +14,11 @@ pub enum Error { #[error("Error opening database: {0}")] OpenDatabase(#[from] sql_support::open_database::Error), - #[error("Error executing SQL: {0}")] - Sql(#[from] rusqlite::Error), + #[error("Error executing SQL: {inner} (context: {context})")] + Sql { + inner: rusqlite::Error, + context: String, + }, #[error("JSON error: {0}")] Json(#[from] serde_json::Error), @@ -33,6 +36,29 @@ pub enum Error { SuggestStoreBuilder(String), } +impl Error { + fn sql(e: rusqlite::Error, context: impl Into) -> Self { + Self::Sql { + inner: e, + context: context.into(), + } + } +} + +impl From for Error { + fn from(e: rusqlite::Error) -> Self { + Self::sql(e, "") + } +} + +#[extend::ext(name=RusqliteResultExt)] +pub impl Result { + // Convert an rusqlite::Error to our error type, with a context value + fn with_context(self, context: &str) -> Result { + self.map_err(|e| Error::sql(e, context)) + } +} + /// The error type for all Suggest component operations. These errors are /// exposed to your application, which should handle them as needed. #[derive(Debug, thiserror::Error)] diff --git a/components/suggest/src/store.rs b/components/suggest/src/store.rs index 2fc1b0d98a..c13f47e9e8 100644 --- a/components/suggest/src/store.rs +++ b/components/suggest/src/store.rs @@ -9,7 +9,7 @@ use std::{ sync::Arc, }; -use error_support::handle_error; +use error_support::{breadcrumb, handle_error}; use once_cell::sync::OnceCell; use parking_lot::Mutex; use remote_settings::{self, RemoteSettingsConfig, RemoteSettingsServer}; @@ -336,6 +336,7 @@ where S: Client, { pub fn ingest(&self, constraints: SuggestIngestionConstraints) -> Result<()> { + breadcrumb!("Ingestion starting"); let writer = &self.dbs()?.writer; if constraints.empty_only && !writer.read(|dao| dao.suggestions_table_empty())? { return Ok(()); @@ -355,10 +356,12 @@ where // Handle ingestion inside single write scope let mut write_scope = writer.write_scope()?; for ingest_record_type in ingest_record_types { + breadcrumb!("Ingesting {ingest_record_type}"); write_scope .write(|dao| self.ingest_records_by_type(ingest_record_type, dao, &constraints))?; write_scope.err_if_interrupted()?; } + breadcrumb!("Ingestion complete"); Ok(()) } diff --git a/megazords/full/DEPENDENCIES.md b/megazords/full/DEPENDENCIES.md index 8077f6b964..e46a90212b 100644 --- a/megazords/full/DEPENDENCIES.md +++ b/megazords/full/DEPENDENCIES.md @@ -11,6 +11,7 @@ the details of which are reproduced below. * [MIT License: bytes](#mit-license-bytes) * [MIT License: cargo_metadata](#mit-license-cargo_metadata) * [MIT License: caseless](#mit-license-caseless) +* [MIT License: extend](#mit-license-extend) * [MIT License: generic-array](#mit-license-generic-array) * [MIT License: goblin](#mit-license-goblin) * [MIT License: libsqlite3-sys, rusqlite](#mit-license-libsqlite3-sys-rusqlite) @@ -902,6 +903,34 @@ LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. +``` +------------- +## MIT License: extend + +The following text applies to code linked from these dependencies: +[extend](https://github.com/davidpdrsn/extend) + +``` +MIT License Copyright (c) 2020 David Pedersen + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is furnished +to do so, subject to the following conditions: + +The above copyright notice and this permission notice (including the next +paragraph) shall be included in all copies or substantial portions of the +Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS +FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS +OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, +WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF +OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + ``` ------------- ## MIT License: generic-array diff --git a/megazords/full/android/dependency-licenses.xml b/megazords/full/android/dependency-licenses.xml index c8cc4a03f8..70365b2646 100644 --- a/megazords/full/android/dependency-licenses.xml +++ b/megazords/full/android/dependency-licenses.xml @@ -536,6 +536,10 @@ the details of which are reproduced below. MIT License: caseless https://github.com/SimonSapin/rust-caseless/blob/master/LICENSE + + MIT License: extend + https://github.com/davidpdrsn/extend/blob/master/LICENSE + MIT License: generic-array https://github.com/fizyk20/generic-array/blob/master/LICENSE diff --git a/megazords/ios-rust/DEPENDENCIES.md b/megazords/ios-rust/DEPENDENCIES.md index b9f7a22906..052fc59139 100644 --- a/megazords/ios-rust/DEPENDENCIES.md +++ b/megazords/ios-rust/DEPENDENCIES.md @@ -12,6 +12,7 @@ the details of which are reproduced below. * [MIT License: bytes](#mit-license-bytes) * [MIT License: cargo_metadata](#mit-license-cargo_metadata) * [MIT License: caseless](#mit-license-caseless) +* [MIT License: extend](#mit-license-extend) * [MIT License: generic-array](#mit-license-generic-array) * [MIT License: goblin](#mit-license-goblin) * [MIT License: h2](#mit-license-h2) @@ -971,6 +972,34 @@ LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. +``` +------------- +## MIT License: extend + +The following text applies to code linked from these dependencies: +[extend](https://github.com/davidpdrsn/extend) + +``` +MIT License Copyright (c) 2020 David Pedersen + +Permission is hereby granted, free of charge, to any person obtaining a copy +of this software and associated documentation files (the "Software"), to deal +in the Software without restriction, including without limitation the rights +to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +copies of the Software, and to permit persons to whom the Software is furnished +to do so, subject to the following conditions: + +The above copyright notice and this permission notice (including the next +paragraph) shall be included in all copies or substantial portions of the +Software. + +THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, FITNESS +FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS +OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, +WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF +OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + ``` ------------- ## MIT License: generic-array