diff --git a/crates/core/src/error.rs b/crates/core/src/error.rs index 3aa5dcf..61fb257 100644 --- a/crates/core/src/error.rs +++ b/crates/core/src/error.rs @@ -1,6 +1,9 @@ -use alloc::string::{String, ToString}; +use alloc::{ + format, + string::{String, ToString}, +}; use core::error::Error; -use sqlite_nostd::{sqlite3, Connection, ResultCode}; +use sqlite_nostd::{context, sqlite3, Connection, Context, ResultCode}; #[derive(Debug)] pub struct SQLiteError(pub ResultCode, pub Option); @@ -11,6 +14,24 @@ impl core::fmt::Display for SQLiteError { } } +impl SQLiteError { + pub fn apply_to_ctx(self, description: &str, ctx: *mut context) { + let SQLiteError(code, message) = self; + + if message.is_some() { + ctx.result_error(&format!("{:} {:}", description, message.unwrap())); + } else { + let error = ctx.db_handle().errmsg().unwrap(); + if error == "not an error" { + ctx.result_error(&format!("{:}", description)); + } else { + ctx.result_error(&format!("{:} {:}", description, error)); + } + } + ctx.result_error_code(code); + } +} + impl Error for SQLiteError {} pub trait PSResult { diff --git a/crates/core/src/fix035.rs b/crates/core/src/fix035.rs deleted file mode 100644 index f90cb6c..0000000 --- a/crates/core/src/fix035.rs +++ /dev/null @@ -1,47 +0,0 @@ -use alloc::format; - -use crate::error::{PSResult, SQLiteError}; -use sqlite_nostd as sqlite; -use sqlite_nostd::{Connection, ResultCode}; - -use crate::ext::SafeManagedStmt; -use crate::util::quote_identifier; - -// Apply a data migration to fix any existing data affected by the issue -// fixed in v0.3.5. -// -// The issue was that the `ps_updated_rows` table was not being populated -// with remove operations in some cases. This causes the rows to be removed -// from ps_oplog, but not from the ps_data__tables, resulting in dangling rows. -// -// The fix here is to find these dangling rows, and add them to ps_updated_rows. -// The next time the sync_local operation is run, these rows will be removed. -pub fn apply_v035_fix(db: *mut sqlite::sqlite3) -> Result { - // language=SQLite - let statement = db - .prepare_v2("SELECT name, powersync_external_table_name(name) FROM sqlite_master WHERE type='table' AND name GLOB 'ps_data__*'") - .into_db_result(db)?; - - while statement.step()? == ResultCode::ROW { - let full_name = statement.column_text(0)?; - let short_name = statement.column_text(1)?; - let quoted = quote_identifier(full_name); - - // language=SQLite - let statement = db.prepare_v2(&format!( - " -INSERT OR IGNORE INTO ps_updated_rows(row_type, row_id) -SELECT ?1, id FROM {} - WHERE NOT EXISTS ( - SELECT 1 FROM ps_oplog - WHERE row_type = ?1 AND row_id = {}.id - );", - quoted, quoted - ))?; - statement.bind_text(1, short_name, sqlite::Destructor::STATIC)?; - - statement.exec()?; - } - - Ok(1) -} diff --git a/crates/core/src/fix_data.rs b/crates/core/src/fix_data.rs new file mode 100644 index 0000000..5b7973b --- /dev/null +++ b/crates/core/src/fix_data.rs @@ -0,0 +1,200 @@ +use core::ffi::c_int; + +use alloc::format; +use alloc::string::String; + +use crate::create_sqlite_optional_text_fn; +use crate::error::{PSResult, SQLiteError}; +use sqlite_nostd::{self as sqlite, ColumnType, Value}; +use sqlite_nostd::{Connection, Context, ResultCode}; + +use crate::ext::SafeManagedStmt; +use crate::util::quote_identifier; + +// Apply a data migration to fix any existing data affected by the issue +// fixed in v0.3.5. +// +// The issue was that the `ps_updated_rows` table was not being populated +// with remove operations in some cases. This causes the rows to be removed +// from ps_oplog, but not from the ps_data__tables, resulting in dangling rows. +// +// The fix here is to find these dangling rows, and add them to ps_updated_rows. +// The next time the sync_local operation is run, these rows will be removed. +pub fn apply_v035_fix(db: *mut sqlite::sqlite3) -> Result { + // language=SQLite + let statement = db + .prepare_v2("SELECT name, powersync_external_table_name(name) FROM sqlite_master WHERE type='table' AND name GLOB 'ps_data__*'") + .into_db_result(db)?; + + while statement.step()? == ResultCode::ROW { + let full_name = statement.column_text(0)?; + let short_name = statement.column_text(1)?; + let quoted = quote_identifier(full_name); + + // language=SQLite + let statement = db.prepare_v2(&format!( + " +INSERT OR IGNORE INTO ps_updated_rows(row_type, row_id) +SELECT ?1, id FROM {} + WHERE NOT EXISTS ( + SELECT 1 FROM ps_oplog + WHERE row_type = ?1 AND row_id = {}.id + );", + quoted, quoted + ))?; + statement.bind_text(1, short_name, sqlite::Destructor::STATIC)?; + + statement.exec()?; + } + + Ok(1) +} + +/// Older versions of the JavaScript SDK for PowerSync used to encode the subkey in oplog data +/// entries as JSON. +/// +/// It wasn't supposed to do that, since the keys are regular strings already. To make databases +/// created with those SDKs compatible with other SDKs or the sync client implemented in the core +/// extensions, a migration is necessary. Since this migration is only relevant for the JS SDK, it +/// is mostly implemented there. However, the helper function to remove the key encoding is +/// implemented here because user-defined functions are expensive on JavaScript. +fn remove_duplicate_key_encoding(key: &str) -> Option { + // Acceptable format: // + // Inacceptable format: //"" + // This is a bit of a tricky conversion because both type and id can contain slashes and quotes. + // However, the subkey is either a UUID value or a `/UUID` value - so we know it can't + // end in a quote unless the improper encoding was used. + if !key.ends_with('"') { + return None; + } + + // Since the subkey is JSON-encoded, find the start quote by going backwards. + let mut chars = key.char_indices(); + chars.next_back()?; // Skip the quote ending the string + + enum FoundStartingQuote { + HasQuote { index: usize }, + HasBackslachThenQuote { quote_index: usize }, + } + let mut state: Option = None; + let found_starting_quote = loop { + if let Some((i, char)) = chars.next_back() { + state = match state { + Some(FoundStartingQuote::HasQuote { index }) => { + if char == '\\' { + // We've seen a \" pattern, not the start of the string + Some(FoundStartingQuote::HasBackslachThenQuote { quote_index: index }) + } else { + break Some(index); + } + } + Some(FoundStartingQuote::HasBackslachThenQuote { quote_index }) => { + if char == '\\' { + // \\" pattern, the quote is unescaped + break Some(quote_index); + } else { + None + } + } + None => { + if char == '"' { + Some(FoundStartingQuote::HasQuote { index: i }) + } else { + None + } + } + } + } else { + break None; + } + }?; + + let before_json = &key[..found_starting_quote]; + let mut result: String = serde_json::from_str(&key[found_starting_quote..]).ok()?; + + result.insert_str(0, before_json); + Some(result) +} + +fn powersync_remove_duplicate_key_encoding_impl( + ctx: *mut sqlite::context, + args: &[*mut sqlite::value], +) -> Result, SQLiteError> { + let arg = args.get(0).ok_or(ResultCode::MISUSE)?; + + if arg.value_type() != ColumnType::Text { + return Err(ResultCode::MISMATCH.into()); + } + + return Ok(remove_duplicate_key_encoding(arg.text())); +} + +create_sqlite_optional_text_fn!( + powersync_remote_duplicate_key_encoding, + powersync_remove_duplicate_key_encoding_impl, + "powersync_remote_duplicate_key_encoding" +); + +pub fn register(db: *mut sqlite::sqlite3) -> Result<(), ResultCode> { + db.create_function_v2( + "powersync_remote_duplicate_key_encoding", + 1, + sqlite::UTF8 | sqlite::DETERMINISTIC, + None, + Some(powersync_remote_duplicate_key_encoding), + None, + None, + None, + )?; + Ok(()) +} + +#[cfg(test)] +mod test { + use core::assert_matches::assert_matches; + + use super::remove_duplicate_key_encoding; + + fn assert_unaffected(source: &str) { + assert_matches!(remove_duplicate_key_encoding(source), None); + } + + #[test] + fn does_not_change_unaffected_keys() { + assert_unaffected("object_type/object_id/subkey"); + assert_unaffected("object_type/object_id/null"); + + // Object type and ID could technically contain quotes and forward slashes + assert_unaffected(r#""object"/"type"/subkey"#); + assert_unaffected("object\"/type/object\"/id/subkey"); + + // Invalid key, but we shouldn't crash + assert_unaffected("\"key\""); + } + + #[test] + fn removes_quotes() { + assert_eq!( + remove_duplicate_key_encoding("foo/bar/\"baz\"").unwrap(), + "foo/bar/baz", + ); + + assert_eq!( + remove_duplicate_key_encoding(r#"foo/bar/"nested/subkey""#).unwrap(), + "foo/bar/nested/subkey" + ); + + assert_eq!( + remove_duplicate_key_encoding(r#"foo/bar/"escaped\"key""#).unwrap(), + "foo/bar/escaped\"key" + ); + assert_eq!( + remove_duplicate_key_encoding(r#"foo/bar/"escaped\\key""#).unwrap(), + "foo/bar/escaped\\key" + ); + assert_eq!( + remove_duplicate_key_encoding(r#"foo/bar/"/\\"subkey""#).unwrap(), + "foo/bar/\"/\\\\subkey" + ); + } +} diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index b191fd0..9d1c997 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -18,7 +18,7 @@ mod crud_vtab; mod diff; mod error; mod ext; -mod fix035; +mod fix_data; mod json_merge; mod kv; mod macros; @@ -57,6 +57,7 @@ fn init_extension(db: *mut sqlite::sqlite3) -> Result<(), ResultCode> { crate::views::register(db)?; crate::uuid::register(db)?; crate::diff::register(db)?; + crate::fix_data::register(db)?; crate::json_merge::register(db)?; crate::view_admin::register(db)?; crate::checkpoint::register(db)?; diff --git a/crates/core/src/macros.rs b/crates/core/src/macros.rs index aeb1f1a..459a5ab 100644 --- a/crates/core/src/macros.rs +++ b/crates/core/src/macros.rs @@ -11,18 +11,7 @@ macro_rules! create_sqlite_text_fn { let result = $fn_impl_name(ctx, args); if let Err(err) = result { - let SQLiteError(code, message) = SQLiteError::from(err); - if message.is_some() { - ctx.result_error(&format!("{:} {:}", $description, message.unwrap())); - } else { - let error = ctx.db_handle().errmsg().unwrap(); - if error == "not an error" { - ctx.result_error(&format!("{:}", $description)); - } else { - ctx.result_error(&format!("{:} {:}", $description, error)); - } - } - ctx.result_error_code(code); + SQLiteError::from(err).apply_to_ctx($description, ctx); } else if let Ok(r) = result { ctx.result_text_transient(&r); } @@ -43,18 +32,7 @@ macro_rules! create_sqlite_optional_text_fn { let result = $fn_impl_name(ctx, args); if let Err(err) = result { - let SQLiteError(code, message) = SQLiteError::from(err); - if message.is_some() { - ctx.result_error(&format!("{:} {:}", $description, message.unwrap())); - } else { - let error = ctx.db_handle().errmsg().unwrap(); - if error == "not an error" { - ctx.result_error(&format!("{:}", $description)); - } else { - ctx.result_error(&format!("{:} {:}", $description, error)); - } - } - ctx.result_error_code(code); + SQLiteError::from(err).apply_to_ctx($description, ctx); } else if let Ok(r) = result { if let Some(s) = r { ctx.result_text_transient(&s); diff --git a/crates/core/src/migrations.rs b/crates/core/src/migrations.rs index 7241957..d00fef2 100644 --- a/crates/core/src/migrations.rs +++ b/crates/core/src/migrations.rs @@ -10,7 +10,7 @@ use sqlite_nostd::{Connection, Context}; use crate::bucket_priority::BucketPriority; use crate::error::{PSResult, SQLiteError}; -use crate::fix035::apply_v035_fix; +use crate::fix_data::apply_v035_fix; pub const LATEST_VERSION: i32 = 9; diff --git a/dart/test/js_key_encoding_test.dart b/dart/test/js_key_encoding_test.dart new file mode 100644 index 0000000..cd83fd0 --- /dev/null +++ b/dart/test/js_key_encoding_test.dart @@ -0,0 +1,77 @@ +import 'dart:convert'; + +import 'package:file/local.dart'; +import 'package:sqlite3/common.dart'; +import 'package:sqlite3/sqlite3.dart'; +import 'package:sqlite3_test/sqlite3_test.dart'; +import 'package:test/test.dart'; + +import 'utils/native_test_utils.dart'; + +void main() { + final vfs = TestSqliteFileSystem(fs: const LocalFileSystem()); + late CommonDatabase db; + + setUpAll(() { + loadExtension(); + sqlite3.registerVirtualFileSystem(vfs, makeDefault: false); + }); + tearDownAll(() => sqlite3.unregisterVirtualFileSystem(vfs)); + + setUp(() async { + db = openTestDatabase(vfs) + ..select('select powersync_init();') + ..select('select powersync_replace_schema(?)', [json.encode(_schema)]); + }); + + tearDown(() { + db.dispose(); + }); + + test('can fix JS key encoding', () { + db.execute('insert into powersync_operations (op, data) VALUES (?, ?);', [ + 'save', + json.encode({ + 'buckets': [ + { + 'bucket': 'a', + 'data': [ + { + 'op_id': '1', + 'op': 'PUT', + 'object_type': 'items', + 'object_id': '1', + 'subkey': json.encode('subkey'), + 'checksum': 0, + 'data': {'col': 'a'}, + } + ], + } + ], + }) + ]); + + db.execute('INSERT INTO powersync_operations(op, data) VALUES (?, ?)', + ['sync_local', null]); + var [row] = db.select('select * from ps_oplog'); + expect(row['key'], 'items/1/"subkey"'); + + // Apply migration + db.execute( + 'UPDATE ps_oplog SET key = powersync_remote_duplicate_key_encoding(key);'); + + [row] = db.select('select * from ps_oplog'); + expect(row['key'], 'items/1/subkey'); + }); +} + +const _schema = { + 'tables': [ + { + 'name': 'items', + 'columns': [ + {'name': 'col', 'type': 'text'} + ], + } + ] +};