Skip to content

Commit b257451

Browse files
simolus3simolus3
committed
Avoid nested JSON elements in ps_crud
1 parent 89f4790 commit b257451

File tree

6 files changed

+113
-37
lines changed

6 files changed

+113
-37
lines changed

‎crates/core/src/schema/mod.rs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,8 @@ use serde::Deserialize;
66
use sqlite::ResultCode;
77
use sqlite_nostd as sqlite;
88
pub use table_info::{
9-
DiffIncludeOld, PendingStatement, PendingStatementValue, RawTable, Table, TableInfoFlags,
9+
Column, DiffIncludeOld, PendingStatement, PendingStatementValue, RawTable, Table,
10+
TableInfoFlags,
1011
};
1112

1213
#[derive(Deserialize, Default)]

‎crates/core/src/schema/table_info.rs‎

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,21 @@ impl Table {
4545
}
4646
}
4747

48-
pub fn column_names(&self) -> impl Iterator<Item = &str> {
49-
self.columns.iter().map(|c| c.name.as_str())
48+
pub fn filtered_columns<'a>(
49+
&'a self,
50+
names: impl Iterator<Item = &'a str>,
51+
) -> impl Iterator<Item = &'a Column> {
52+
// First, sort all columns by name for faster lookups by name.
53+
let mut sorted_by_name: Vec<&Column> = self.columns.iter().collect();
54+
sorted_by_name.sort_by_key(|c| &*c.name);
55+
56+
names.filter_map(move |name| {
57+
let index = sorted_by_name
58+
.binary_search_by_key(&name, |c| c.name.as_str())
59+
.ok()?;
60+
61+
Some(sorted_by_name[index])
62+
})
5063
}
5164
}
5265

‎crates/core/src/util.rs‎

Lines changed: 26 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
extern crate alloc;
22

3+
use core::fmt::{Display, Write};
4+
35
use alloc::format;
4-
use alloc::string::String;
6+
use alloc::string::{String, ToString};
57

68
#[cfg(not(feature = "getrandom"))]
79
use crate::sqlite;
@@ -13,7 +15,7 @@ use uuid::Uuid;
1315
use uuid::Builder;
1416

1517
pub fn quote_string(s: &str) -> String {
16-
format!("'{:}'", s.replace("'", "''"))
18+
return QuotedString(s).to_string();
1719
}
1820

1921
pub fn quote_json_path(s: &str) -> String {
@@ -32,6 +34,28 @@ pub fn quote_internal_name(name: &str, local_only: bool) -> String {
3234
}
3335
}
3436

37+
/// A string that [Display]s as a SQLite string literal.
38+
pub struct QuotedString<'a>(pub &'a str);
39+
40+
impl<'a> Display for QuotedString<'a> {
41+
fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
42+
const SINGLE_QUOTE: char = '\'';
43+
const ESCAPE_SEQUENCE: &'static str = "''";
44+
45+
f.write_char(SINGLE_QUOTE)?;
46+
47+
for (i, group) in self.0.split(SINGLE_QUOTE).enumerate() {
48+
if i != 0 {
49+
f.write_str(ESCAPE_SEQUENCE)?;
50+
}
51+
52+
f.write_str(group)?;
53+
}
54+
55+
f.write_char(SINGLE_QUOTE)
56+
}
57+
}
58+
3559
pub fn quote_identifier_prefixed(prefix: &str, name: &str) -> String {
3660
return format!("\"{:}{:}\"", prefix, name.replace("\"", "\"\""));
3761
}

‎crates/core/src/views.rs‎

Lines changed: 38 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ use sqlite_nostd::{self as sqlite};
1212

1313
use crate::create_sqlite_text_fn;
1414
use crate::error::PowerSyncError;
15-
use crate::schema::{DiffIncludeOld, Table};
15+
use crate::schema::{Column, DiffIncludeOld, Table};
1616
use crate::util::*;
1717

1818
fn powersync_view_sql_impl(
@@ -88,11 +88,12 @@ fn powersync_trigger_delete_sql_impl(
8888
match &table_info.diff_include_old {
8989
Some(include_old) => {
9090
let mut json = match include_old {
91-
DiffIncludeOld::OnlyForColumns { columns } => {
92-
json_object_fragment("OLD", &mut columns.iter().map(|c| c.as_str()))
93-
}
91+
DiffIncludeOld::OnlyForColumns { columns } => json_object_fragment(
92+
"OLD",
93+
&mut table_info.filtered_columns(columns.iter().map(|c| c.as_str())),
94+
),
9495
DiffIncludeOld::ForAllColumns => {
95-
json_object_fragment("OLD", &mut table_info.column_names())
96+
json_object_fragment("OLD", &mut table_info.columns.iter())
9697
}
9798
}?;
9899

@@ -174,7 +175,7 @@ fn powersync_trigger_insert_sql_impl(
174175
let trigger_name = quote_identifier_prefixed("ps_view_insert_", view_name);
175176
let type_string = quote_string(name);
176177

177-
let json_fragment = json_object_fragment("NEW", &mut table_info.column_names())?;
178+
let json_fragment = json_object_fragment("NEW", &mut table_info.columns.iter())?;
178179

179180
let (metadata_key, metadata_value) = if table_info.flags.include_metadata() {
180181
(",metadata", ",NEW._metadata")
@@ -247,15 +248,15 @@ fn powersync_trigger_update_sql_impl(
247248
let trigger_name = quote_identifier_prefixed("ps_view_update_", view_name);
248249
let type_string = quote_string(name);
249250

250-
let json_fragment_new = json_object_fragment("NEW", &mut table_info.column_names())?;
251-
let json_fragment_old = json_object_fragment("OLD", &mut table_info.column_names())?;
251+
let json_fragment_new = json_object_fragment("NEW", &mut table_info.columns.iter())?;
252+
let json_fragment_old = json_object_fragment("OLD", &mut table_info.columns.iter())?;
252253

253254
let mut old_values_fragment = match &table_info.diff_include_old {
254255
None => None,
255256
Some(DiffIncludeOld::ForAllColumns) => Some(json_fragment_old.clone()),
256257
Some(DiffIncludeOld::OnlyForColumns { columns }) => Some(json_object_fragment(
257258
"OLD",
258-
&mut columns.iter().map(|c| c.as_str()),
259+
&mut table_info.filtered_columns(columns.iter().map(|c| c.as_str())),
259260
)?),
260261
};
261262

@@ -266,9 +267,12 @@ fn powersync_trigger_update_sql_impl(
266267
let filtered_new_fragment = match &table_info.diff_include_old {
267268
// When include_old_only_when_changed is combined with a column filter, make sure we
268269
// only include the powersync_diff of columns matched by the filter.
269-
Some(DiffIncludeOld::OnlyForColumns { columns }) => Cow::Owned(
270-
json_object_fragment("NEW", &mut columns.iter().map(|c| c.as_str()))?,
271-
),
270+
Some(DiffIncludeOld::OnlyForColumns { columns }) => {
271+
Cow::Owned(json_object_fragment(
272+
"NEW",
273+
&mut table_info.filtered_columns(columns.iter().map(|c| c.as_str())),
274+
)?)
275+
}
272276
_ => Cow::Borrowed(json_fragment_new.as_str()),
273277
};
274278

@@ -401,21 +405,35 @@ pub fn register(db: *mut sqlite::sqlite3) -> Result<(), ResultCode> {
401405
/// Example output with prefix "NEW": "json_object('id', NEW.id, 'name', NEW.name, 'age', NEW.age)".
402406
fn json_object_fragment<'a>(
403407
prefix: &str,
404-
name_results: &mut dyn Iterator<Item = &'a str>,
408+
columns: &mut dyn Iterator<Item = &'a Column>,
405409
) -> Result<String, PowerSyncError> {
406410
// floor(SQLITE_MAX_FUNCTION_ARG / 2).
407411
// To keep databases portable, we use the default limit of 100 args for this,
408412
// and don't try to query the limit dynamically.
409413
const MAX_ARG_COUNT: usize = 50;
410414

411415
let mut column_names_quoted: Vec<String> = alloc::vec![];
412-
while let Some(name) = name_results.next() {
413-
let quoted: String = format!(
414-
"{:}, {:}.{:}",
415-
quote_string(name),
416-
prefix,
417-
quote_identifier(name)
418-
);
416+
while let Some(column) = columns.next() {
417+
let name = &*column.name;
418+
let quoted = match &*column.type_name {
419+
// We really want the individual columns here to appear as they show up in the database.
420+
// For text columns however, it's possible that e.g. NEW.column was created by a JSON
421+
// function, meaning that it has a JSON subtype active - causing the json_object() call
422+
// we're about to emit to include it as a subobject instead of a string.
423+
"TEXT" | "text" => format!(
424+
"{:}, concat({:}.{:})",
425+
QuotedString(name),
426+
prefix,
427+
quote_identifier(name)
428+
),
429+
_ => format!(
430+
"{:}, {:}.{:}",
431+
QuotedString(name),
432+
prefix,
433+
quote_identifier(name)
434+
),
435+
};
436+
419437
column_names_quoted.push(quoted);
420438
}
421439

‎dart/test/crud_test.dart‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -661,5 +661,25 @@ void main() {
661661
// The update which didn't change any rows should not be recorded.
662662
expect(db.select('SELECT * FROM ps_crud'), hasLength(1));
663663
});
664+
665+
test('json values are included as text', () {
666+
db
667+
..execute('select powersync_replace_schema(?)', [
668+
json.encode({
669+
'tables': [
670+
{
671+
'name': 'items',
672+
'columns': [
673+
{'name': 'col', 'type': 'text'}
674+
],
675+
}
676+
]
677+
})
678+
])
679+
..execute('INSERT INTO items (id, col) VALUES (uuid(), json_object())');
680+
681+
final [update] = db.select('SELECT data FROM ps_crud');
682+
expect(json.decode(update['data']), containsPair('data', {'col': '{}'}));
683+
});
664684
});
665685
}

‎dart/test/utils/migration_fixtures.dart‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -536,8 +536,8 @@ END
536536
THEN RAISE (FAIL, 'id should be text')
537537
END;
538538
INSERT INTO "ps_data__lists"
539-
SELECT NEW.id, json_object('description', NEW."description");
540-
INSERT INTO powersync_crud_(data) VALUES(json_object('op', 'PUT', 'type', 'lists', 'id', NEW.id, 'data', json(powersync_diff('{}', json_object('description', NEW."description")))));
539+
SELECT NEW.id, json_object('description', concat(NEW."description"));
540+
INSERT INTO powersync_crud_(data) VALUES(json_object('op', 'PUT', 'type', 'lists', 'id', NEW.id, 'data', json(powersync_diff('{}', json_object('description', concat(NEW."description"))))));
541541
INSERT INTO ps_oplog(bucket, op_id, op, row_type, row_id, hash, superseded)
542542
SELECT '$local',
543543
1,
@@ -557,9 +557,9 @@ BEGIN
557557
THEN RAISE (FAIL, 'Cannot update id')
558558
END;
559559
UPDATE "ps_data__lists"
560-
SET data = json_object('description', NEW."description")
560+
SET data = json_object('description', concat(NEW."description"))
561561
WHERE id = NEW.id;
562-
INSERT INTO powersync_crud_(data) VALUES(json_object('op', 'PATCH', 'type', 'lists', 'id', NEW.id, 'data', json(powersync_diff(json_object('description', OLD."description"), json_object('description', NEW."description")))));
562+
INSERT INTO powersync_crud_(data) VALUES(json_object('op', 'PATCH', 'type', 'lists', 'id', NEW.id, 'data', json(powersync_diff(json_object('description', concat(OLD."description")), json_object('description', concat(NEW."description"))))));
563563
INSERT INTO ps_oplog(bucket, op_id, op, row_type, row_id, hash, superseded)
564564
SELECT '$local',
565565
1,
@@ -598,8 +598,8 @@ END
598598
WHEN (typeof(NEW.id) != 'text')
599599
THEN RAISE (FAIL, 'id should be text')
600600
END;
601-
INSERT INTO "ps_data__lists" SELECT NEW.id, json_object('description', NEW."description");
602-
INSERT INTO powersync_crud(op,id,type,data) VALUES ('PUT',NEW.id,'lists',json(powersync_diff('{}', json_object('description', NEW."description"))));
601+
INSERT INTO "ps_data__lists" SELECT NEW.id, json_object('description', concat(NEW."description"));
602+
INSERT INTO powersync_crud(op,id,type,data) VALUES ('PUT',NEW.id,'lists',json(powersync_diff('{}', json_object('description', concat(NEW."description")))));
603603
END
604604
;CREATE TRIGGER "ps_view_update_lists"
605605
INSTEAD OF UPDATE ON "lists"
@@ -610,9 +610,9 @@ BEGIN
610610
THEN RAISE (FAIL, 'Cannot update id')
611611
END;
612612
UPDATE "ps_data__lists"
613-
SET data = json_object('description', NEW."description")
613+
SET data = json_object('description', concat(NEW."description"))
614614
WHERE id = NEW.id;
615-
INSERT INTO powersync_crud(op,type,id,data,options) VALUES ('PATCH','lists',NEW.id,json(powersync_diff(json_object('description', OLD."description"), json_object('description', NEW."description"))),0);
615+
INSERT INTO powersync_crud(op,type,id,data,options) VALUES ('PATCH','lists',NEW.id,json(powersync_diff(json_object('description', concat(OLD."description")), json_object('description', concat(NEW."description")))),0);
616616
END
617617
''';
618618

@@ -636,8 +636,8 @@ END
636636
WHEN (typeof(NEW.id) != 'text')
637637
THEN RAISE (FAIL, 'id should be text')
638638
END;
639-
INSERT INTO "ps_data__lists" SELECT NEW.id, json_object('description', NEW."description");
640-
INSERT INTO powersync_crud(op,id,type,data) VALUES ('PUT',NEW.id,'lists',json(powersync_diff('{}', json_object('description', NEW."description"))));
639+
INSERT INTO "ps_data__lists" SELECT NEW.id, json_object('description', concat(NEW."description"));
640+
INSERT INTO powersync_crud(op,id,type,data) VALUES ('PUT',NEW.id,'lists',json(powersync_diff('{}', json_object('description', concat(NEW."description")))));
641641
END
642642
;CREATE TRIGGER "ps_view_update_lists"
643643
INSTEAD OF UPDATE ON "lists"
@@ -648,8 +648,8 @@ BEGIN
648648
THEN RAISE (FAIL, 'Cannot update id')
649649
END;
650650
UPDATE "ps_data__lists"
651-
SET data = json_object('description', NEW."description")
651+
SET data = json_object('description', concat(NEW."description"))
652652
WHERE id = NEW.id;
653-
INSERT INTO powersync_crud(op,type,id,data,options) VALUES ('PATCH','lists',NEW.id,json(powersync_diff(json_object('description', OLD."description"), json_object('description', NEW."description"))),0);
653+
INSERT INTO powersync_crud(op,type,id,data,options) VALUES ('PATCH','lists',NEW.id,json(powersync_diff(json_object('description', concat(OLD."description")), json_object('description', concat(NEW."description")))),0);
654654
END
655655
''';

0 commit comments

Comments
 (0)