-
-
Notifications
You must be signed in to change notification settings - Fork 0
Feat/cloudflare d1 #92
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
Conversation
Signed-off-by: Illyrius <fitimq@live.nl>
…alDB types Signed-off-by: Illyrius <fitimq@live.nl>
Split GitHub OAuth callback logic into smaller functions for fetching user data, saving or updating users, extracting user ID, and setting session cookies. Update chrono dependency to enable serde support. Signed-off-by: Illyrius <fitimq@live.nl>
…ated logic Signed-off-by: Illyrius <fitimq@live.nl>
Signed-off-by: Illyrius <fitimq@live.nl>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR attempts to migrate the database layer from SurrealDB to Cloudflare D1. However, the implementation is incomplete and contains critical architectural incompatibilities that prevent it from functioning.
Changes:
- Replaced SurrealDB database client with a stub Cloudflare D1 implementation (incomplete)
- Removed IFC model data routes and associated functionality
- Changed model IDs from SurrealDB RecordId to UUID v7
- Refactored GitHub OAuth callback handler into smaller helper functions
- Updated configuration to use database binding name instead of connection URL
- Added worker and uuid dependencies, removed surrealdb dependencies
- Renamed GitHub workflow cache step ID from
cache_depstocache_dependencies
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 16 comments.
Show a summary per file
| File | Description |
|---|---|
| src/database.rs | Complete rewrite of database layer with new trait-based abstraction and D1-style implementation (incomplete with critical bugs) |
| src/config.rs | Updated database configuration fields from URL/username/password to binding name |
| src/models/user.rs | Changed ID type from RecordId to Uuid, added Default derive |
| src/models/card.rs | Changed ID type from RecordId to Uuid, added Default derive |
| src/routes/github.rs | Removed SurrealDB imports, refactored callback into helper functions, uses new database trait |
| src/routes/data.rs | Completely removed file containing IFC model CRUD endpoints |
| src/main.rs | Updated Database initialization to synchronous call, removed data route imports |
| src/lib.rs | Removed data module export |
| src/utils.rs | Updated error message to display database_binding_name instead of database_url |
| Cargo.toml | Added worker, uuid, chrono serde features; removed surrealdb dependencies |
| .github/workflows/release_build.yml | Renamed cache_dependencies step ID |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| pub struct Database { | ||
| pub client: Surreal<Client>, | ||
| binding_name: String, |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The binding_name field is stored in the Database struct but never used in any of the methods. In a Cloudflare Workers environment with D1, this binding name should be used to access the D1 database instance from the Workers runtime environment.
| let sql = format!("UPDATE {} SET data = ?, updated_at = ? WHERE id = ?", table); | ||
| let params = vec![ |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SQL query uses format! to directly interpolate the table name into the query string. While the params are properly parameterized, the table name itself could be vulnerable to SQL injection if the caller passes unsanitized input. Consider validating the table name against a whitelist or using a safer approach to construct the query.
| let where_clause: Vec<String> = conditions | ||
| .keys() | ||
| .map(|key| format!("json_extract(data, '$.{}') = ?", key)) | ||
| .collect(); | ||
| let sql = format!( | ||
| "SELECT data FROM {} WHERE {}", | ||
| table, | ||
| where_clause.join(" AND ") | ||
| ); |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SQL query uses format! to directly interpolate both the table name and the condition keys into the query string. The condition keys from the HashMap are inserted into the json_extract function without sanitization. This could be vulnerable to SQL injection if malicious keys are passed in the conditions HashMap. Consider validating both the table name and condition keys against whitelists.
| uuid = { version = "1.19", features = ["v7"] } | ||
| worker = "0.7.4" No newline at end of file |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The worker crate (version 0.7.4) has been added to Cargo.toml but is not used anywhere in the codebase. This dependency is essential for Cloudflare Workers and D1 integration, but the actual integration code is missing. The database implementation needs to use this crate to interact with D1 bindings.
| uuid = { version = "1.19", features = ["v7"] } | |
| worker = "0.7.4" | |
| uuid = { version = "1.19", features = ["v7"] } |
| #[launch] | ||
| async fn rocket() -> Rocket<Build> { | ||
| let config = Config::new(); | ||
| build() | ||
| .manage(config.clone()) | ||
| .manage(Database::new(&config).await) | ||
| .mount( | ||
| "/", | ||
| routes![ | ||
| github_login, | ||
| github_callback, | ||
| health, | ||
| data_upload, | ||
| data_get, | ||
| data_update, | ||
| data_delete, | ||
| ], | ||
| ) | ||
| .manage(Database::new(&config.database_binding_name)) | ||
| .mount("/", routes![github_login, github_callback, health,]) |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR attempts to migrate to Cloudflare D1 (which requires Cloudflare Workers runtime) while keeping Rocket as the web framework. Rocket is a standalone Rust web framework that runs on standard servers, not in the Cloudflare Workers runtime environment. These two are fundamentally incompatible. To use Cloudflare D1, the application needs to be rewritten as a Cloudflare Worker using the worker crate's HTTP routing instead of Rocket, or the database layer needs to use a different database that works with traditional server deployments.
| async fn create<T: Serialize + Send>(&self, table: &str, data: T) -> Result<String> { | ||
| let id = Uuid::now_v7().to_string(); | ||
| let data_value = serde_json::to_value(data)?; | ||
| let data_str = data_value.to_string(); | ||
| let now = chrono::Utc::now().to_rfc3339(); | ||
| let sql = format!( | ||
| "INSERT INTO {} (id, data, created_at) VALUES (?, ?, ?)", | ||
| table | ||
| ); | ||
| let params = vec![ | ||
| Value::String(id.clone()), | ||
| Value::String(data_str), | ||
| Value::String(now), | ||
| ]; |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The database stores entire objects as serialized JSON strings in a data column. While this works, it prevents efficient querying by object fields and loses type safety at the database level. For the D1 migration, consider using proper table schemas with individual columns for key fields, which would enable better indexing and querying capabilities. The current json_extract approach in the query method will be slower than native column queries.
| #[derive(Clone, Default, Debug, Serialize, Deserialize)] | ||
| #[serde(crate = "rocket::serde")] | ||
| pub struct User { | ||
| pub id: Option<RecordId>, | ||
| pub id: Uuid, |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The User struct now derives Default trait (line 8), but the id field is of type Uuid which doesn't have a sensible default value. The derived Default will use Uuid::from_bytes([0; 16]) which creates a nil UUID. This could lead to ID conflicts when the default is used. Consider removing the Default derive or implementing a custom Default that generates a new UUID.
| #[derive(Clone, Default, Debug, Serialize, Deserialize)] | ||
| #[serde(crate = "rocket::serde")] | ||
| pub struct Card { | ||
| pub id: Option<RecordId>, | ||
| pub id: Uuid, | ||
| pub thumbnail: Option<String>, | ||
| pub title: String, | ||
| pub author: User, |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Card struct now derives Default trait (line 8), which creates problematic defaults for multiple fields: id will be a nil UUID (potential conflicts), author will use User's default (which also has a nil UUID), and title, description, and platform will be empty strings. These defaults don't represent valid Card instances and could lead to data integrity issues. Consider removing the Default derive.
| let sql = format!( | ||
| "INSERT INTO {} (id, data, created_at) VALUES (?, ?, ?)", | ||
| table | ||
| ); |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SQL query uses format! to directly interpolate the table name into the query string. While the params are properly parameterized, the table name itself could be vulnerable to SQL injection if the caller passes unsanitized input. Consider validating the table name against a whitelist or using a safer approach to construct the query.
| let sql = format!("SELECT data FROM {}", table); | ||
| let result = self.execute_query(&sql, vec![]).await?; |
Copilot
AI
Feb 5, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The SQL query uses format! to directly interpolate the table name into the query string. While the params are properly parameterized, the table name itself could be vulnerable to SQL injection if the caller passes unsanitized input. Consider validating the table name against a whitelist or using a safer approach to construct the query.
Description
Please include a summary of the changes and the related issue. Explain the motivation behind these changes.
Checklist: