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

feat: introduce updated at field in top level toriii query #2807

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions crates/torii/core/src/model.rs
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,7 @@
}

/// Creates a query that fetches all models and their nested data.
#[allow(clippy::too_many_arguments)]
pub fn build_sql_query(
schemas: &Vec<Ty>,
table_name: &str,
Expand All @@ -124,6 +125,7 @@
order_by: Option<&str>,
limit: Option<u32>,
offset: Option<u32>,
internal_updated_at: u64,
) -> Result<(String, String), Error> {
fn collect_columns(table_prefix: &str, path: &str, ty: &Ty, selections: &mut Vec<String>) {
match ty {
Expand Down Expand Up @@ -172,6 +174,7 @@
selections.push(format!("{}.id", table_name));
selections.push(format!("{}.keys", table_name));

let mut internal_updated_at_clause = Vec::with_capacity(schemas.len());
// Process each model schema
for model in schemas {
let model_table = model.name();
Expand All @@ -180,6 +183,11 @@
[{model_table}].{entity_relation_column}",
));

if internal_updated_at > 0 {
internal_updated_at_clause
.push(format!("[{model_table}].internal_updated_at > {}", internal_updated_at));

Check warning on line 188 in crates/torii/core/src/model.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/core/src/model.rs#L187-L188

Added lines #L187 - L188 were not covered by tests
}

// Collect columns with table prefix
collect_columns(&model_table, "", model, &mut selections);
}
Expand All @@ -197,6 +205,18 @@
count_query += &format!(" WHERE {}", where_clause);
}

if !internal_updated_at_clause.is_empty() {
if where_clause.is_none() {
query += " WHERE ";
count_query += " WHERE ";
} else {
query += " AND ";
count_query += " AND ";
}
query += &format!(" {}", internal_updated_at_clause.join(" AND "));
count_query += &format!(" {}", internal_updated_at_clause.join(" AND "));

Check warning on line 217 in crates/torii/core/src/model.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/core/src/model.rs#L209-L217

Added lines #L209 - L217 were not covered by tests
}

// Use custom order by if provided, otherwise default to event_id DESC
if let Some(order_clause) = order_by {
query += &format!(" ORDER BY {}", order_clause);
Expand Down Expand Up @@ -494,6 +514,7 @@
None,
None,
None,
0,
)
.unwrap();

Expand Down
1 change: 1 addition & 0 deletions crates/torii/grpc/proto/types.proto
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,7 @@ message Query {
bool dont_include_hashed_keys = 4;
repeated OrderBy order_by = 5;
repeated string entity_models = 6;
uint64 internal_updated_at = 7;
}

message EventQuery {
Expand Down
21 changes: 20 additions & 1 deletion crates/torii/grpc/src/server/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -229,6 +229,7 @@
dont_include_hashed_keys: bool,
order_by: Option<&str>,
entity_models: Vec<String>,
internal_updated_at: u64,

Check warning on line 232 in crates/torii/grpc/src/server/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/grpc/src/server/mod.rs#L232

Added line #L232 was not covered by tests
) -> Result<(Vec<proto::types::Entity>, u32), Error> {
self.query_by_hashed_keys(
table,
Expand All @@ -240,6 +241,7 @@
dont_include_hashed_keys,
order_by,
entity_models,
internal_updated_at,

Check warning on line 244 in crates/torii/grpc/src/server/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/grpc/src/server/mod.rs#L244

Added line #L244 was not covered by tests
)
.await
}
Expand All @@ -258,6 +260,7 @@
row_events.iter().map(map_row_to_event).collect()
}

#[allow(clippy::too_many_arguments)]
async fn fetch_entities(
&self,
table: &str,
Expand All @@ -266,6 +269,7 @@
dont_include_hashed_keys: bool,
order_by: Option<&str>,
entity_models: Vec<String>,
internal_updated_at: u64,
) -> Result<Vec<proto::types::Entity>, Error> {
let entity_models =
entity_models.iter().map(|tag| compute_selector_from_tag(tag)).collect::<Vec<Felt>>();
Expand Down Expand Up @@ -354,9 +358,11 @@
order_by,
None,
None,
internal_updated_at,
)?;

let rows = sqlx::query(&entity_query).bind(models_str).fetch_all(&mut *tx).await?;
let query = sqlx::query(&entity_query).bind(models_str);
let rows = query.fetch_all(&mut *tx).await?;
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ohayo, sensei! Missing binding for internal_updated_at in the SQL query.

While internal_updated_at is used within the build_sql_query function to build the SQL query, it appears that in lines 363-364, internal_updated_at is not bound to the query. This may lead to incorrect query results.

Please bind internal_updated_at to the query to ensure proper functionality.

Apply this diff to fix the issue:

-                let query = sqlx::query(&entity_query).bind(models_str);
+                let query = sqlx::query(&entity_query).bind(models_str).bind(internal_updated_at);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
internal_updated_at,
)?;
let rows = sqlx::query(&entity_query).bind(models_str).fetch_all(&mut *tx).await?;
let query = sqlx::query(&entity_query).bind(models_str);
let rows = query.fetch_all(&mut *tx).await?;
internal_updated_at,
)?;
let query = sqlx::query(&entity_query).bind(models_str).bind(internal_updated_at);
let rows = query.fetch_all(&mut *tx).await?;

let schemas = Arc::new(schemas);

let group_entities: Result<Vec<_>, Error> = rows
Expand Down Expand Up @@ -430,6 +436,7 @@
dont_include_hashed_keys: bool,
order_by: Option<&str>,
entity_models: Vec<String>,
internal_updated_at: u64,

Check warning on line 439 in crates/torii/grpc/src/server/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/grpc/src/server/mod.rs#L439

Added line #L439 was not covered by tests
) -> Result<(Vec<proto::types::Entity>, u32), Error> {
// TODO: use prepared statement for where clause
let filter_ids = match hashed_keys {
Expand Down Expand Up @@ -510,6 +517,7 @@
dont_include_hashed_keys,
order_by,
entity_models,
internal_updated_at,

Check warning on line 520 in crates/torii/grpc/src/server/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/grpc/src/server/mod.rs#L520

Added line #L520 was not covered by tests
)
.await?;
Ok((entities, total_count))
Expand All @@ -527,6 +535,7 @@
dont_include_hashed_keys: bool,
order_by: Option<&str>,
entity_models: Vec<String>,
internal_updated_at: u64,
) -> Result<(Vec<proto::types::Entity>, u32), Error> {
let keys_pattern = build_keys_pattern(keys_clause)?;

Expand Down Expand Up @@ -655,6 +664,7 @@
dont_include_hashed_keys,
order_by,
entity_models,
internal_updated_at,
)
.await?;
Ok((entities, total_count))
Expand Down Expand Up @@ -699,6 +709,7 @@
dont_include_hashed_keys: bool,
order_by: Option<&str>,
entity_models: Vec<String>,
internal_updated_at: u64,

Check warning on line 712 in crates/torii/grpc/src/server/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/grpc/src/server/mod.rs#L712

Added line #L712 was not covered by tests
) -> Result<(Vec<proto::types::Entity>, u32), Error> {
let entity_models =
entity_models.iter().map(|model| compute_selector_from_tag(model)).collect::<Vec<_>>();
Expand Down Expand Up @@ -765,6 +776,7 @@
order_by,
limit,
offset,
internal_updated_at,

Check warning on line 779 in crates/torii/grpc/src/server/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/grpc/src/server/mod.rs#L779

Added line #L779 was not covered by tests
)?;

let total_count = sqlx::query_scalar(&count_query)
Expand Down Expand Up @@ -798,6 +810,7 @@
dont_include_hashed_keys: bool,
order_by: Option<&str>,
entity_models: Vec<String>,
internal_updated_at: u64,

Check warning on line 813 in crates/torii/grpc/src/server/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/grpc/src/server/mod.rs#L813

Added line #L813 was not covered by tests
) -> Result<(Vec<proto::types::Entity>, u32), Error> {
let (where_clause, having_clause, join_clause, bind_values) =
build_composite_clause(table, model_relation_table, &composite)?;
Expand Down Expand Up @@ -868,6 +881,7 @@
dont_include_hashed_keys,
order_by,
entity_models,
internal_updated_at,

Check warning on line 884 in crates/torii/grpc/src/server/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/grpc/src/server/mod.rs#L884

Added line #L884 was not covered by tests
)
.await?;
Ok((entities, total_count))
Expand Down Expand Up @@ -1036,6 +1050,7 @@
query.dont_include_hashed_keys,
order_by,
query.entity_models,
query.internal_updated_at,

Check warning on line 1053 in crates/torii/grpc/src/server/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/grpc/src/server/mod.rs#L1053

Added line #L1053 was not covered by tests
)
.await?
}
Expand All @@ -1059,6 +1074,7 @@
query.dont_include_hashed_keys,
order_by,
query.entity_models,
query.internal_updated_at,

Check warning on line 1077 in crates/torii/grpc/src/server/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/grpc/src/server/mod.rs#L1077

Added line #L1077 was not covered by tests
)
.await?
}
Expand All @@ -1073,6 +1089,7 @@
query.dont_include_hashed_keys,
order_by,
query.entity_models,
query.internal_updated_at,

Check warning on line 1092 in crates/torii/grpc/src/server/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/grpc/src/server/mod.rs#L1092

Added line #L1092 was not covered by tests
)
.await?
}
Expand All @@ -1087,6 +1104,7 @@
query.dont_include_hashed_keys,
order_by,
query.entity_models,
query.internal_updated_at,

Check warning on line 1107 in crates/torii/grpc/src/server/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/grpc/src/server/mod.rs#L1107

Added line #L1107 was not covered by tests
)
.await?
}
Expand All @@ -1101,6 +1119,7 @@
query.dont_include_hashed_keys,
order_by,
query.entity_models,
query.internal_updated_at,

Check warning on line 1122 in crates/torii/grpc/src/server/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/grpc/src/server/mod.rs#L1122

Added line #L1122 was not covered by tests
)
.await?
}
Expand Down
1 change: 1 addition & 0 deletions crates/torii/grpc/src/server/tests/entities_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ async fn test_entities_queries(sequencer: &RunnerCtx) {
false,
None,
vec![],
0,
)
.await
.unwrap()
Expand Down
2 changes: 2 additions & 0 deletions crates/torii/grpc/src/types/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@
pub dont_include_hashed_keys: bool,
pub order_by: Vec<OrderBy>,
pub entity_models: Vec<String>,
pub internal_updated_at: u64,
}

#[derive(Debug, Serialize, Deserialize, PartialEq, Hash, Eq, Clone)]
Expand Down Expand Up @@ -271,6 +272,7 @@
dont_include_hashed_keys: value.dont_include_hashed_keys,
order_by: value.order_by.into_iter().map(|o| o.into()).collect(),
entity_models: value.entity_models,
internal_updated_at: value.internal_updated_at,

Check warning on line 275 in crates/torii/grpc/src/types/mod.rs

View check run for this annotation

Codecov / codecov/patch

crates/torii/grpc/src/types/mod.rs#L275

Added line #L275 was not covered by tests
}
}
}
Expand Down
Loading