Add SurrealDB as an option for app manager #95
Conversation
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @countradooku, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly expands the backend options for application management by integrating SurrealDB. It provides a robust and cached solution for storing and retrieving application configurations, offering more flexibility for deployment environments and database preferences.
Highlights
- New App Manager Driver: Introduced SurrealDB as a new option for the application manager, allowing application configurations to be stored and managed in a SurrealDB instance.
- SurrealDB Integration: Implemented a dedicated
SurrealDbAppManagermodule, providing full CRUD operations for application data, including connection handling, authentication, schema management (indexes), and an in-memory cache for improved performance. - Configuration and Documentation Updates: Updated the
.env.examplefile with new SurrealDB configuration variables and theREADME.mdto reflect the added support for SurrealDB in the App Manager. - Dependency Management: Added the
surrealdbcrate as a new dependency inCargo.tomlto enable SurrealDB connectivity and operations.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request introduces SurrealDB as a new database option for the application manager. The implementation is well-structured and follows the patterns of other database managers. I've identified a couple of critical issues in the new surrealdb_app_manager.rs file related to incorrect record ID creation and a flawed user signature verification logic. I've also included a suggestion to optimize the new surrealdb dependency in Cargo.toml. Overall, this is a great addition that will be ready with a few fixes.
| impl From<&App> for AppRecord { | ||
| fn from(app: &App) -> Self { | ||
| Self { | ||
| id: RecordId::from_table_key(&app.id, &app.id), | ||
| key: app.key.clone(), | ||
| secret: app.secret.clone(), | ||
| max_connections: app.max_connections, | ||
| enable_client_messages: app.enable_client_messages, | ||
| enabled: app.enabled, | ||
| max_backend_events_per_second: app.max_backend_events_per_second, | ||
| max_client_events_per_second: app.max_client_events_per_second, | ||
| max_read_requests_per_second: app.max_read_requests_per_second, | ||
| max_presence_members_per_channel: app.max_presence_members_per_channel, | ||
| max_presence_member_size_in_kb: app.max_presence_member_size_in_kb, | ||
| max_channel_name_length: app.max_channel_name_length, | ||
| max_event_channels_at_once: app.max_event_channels_at_once, | ||
| max_event_name_length: app.max_event_name_length, | ||
| max_event_payload_in_kb: app.max_event_payload_in_kb, | ||
| max_event_batch_size: app.max_event_batch_size, | ||
| enable_user_authentication: app.enable_user_authentication, | ||
| enable_watchlist_events: app.enable_watchlist_events, | ||
| allowed_origins: app.allowed_origins.clone(), | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
This From implementation incorrectly constructs the RecordId because it uses app.id as the table name. This is a critical bug that will cause data to be written to incorrect tables or fail entirely. The From trait doesn't have access to the correct table name from the manager's configuration.
A better approach is to change this into a static method on AppRecord that accepts the table name as an argument.
After applying this suggestion, you will also need to update the calls in create_app and update_app from AppRecord::from(&app) to AppRecord::from_app(&app, &self.config.table_name).
impl AppRecord {
fn from_app(app: &App, table_name: &str) -> Self {
Self {
id: RecordId::new(table_name, &app.id),
key: app.key.clone(),
secret: app.secret.clone(),
max_connections: app.max_connections,
enable_client_messages: app.enable_client_messages,
enabled: app.enabled,
max_backend_events_per_second: app.max_backend_events_per_second,
max_client_events_per_second: app.max_client_events_per_second,
max_read_requests_per_second: app.max_read_requests_per_second,
max_presence_members_per_channel: app.max_presence_members_per_channel,
max_presence_member_size_in_kb: app.max_presence_member_size_in_kb,
max_channel_name_length: app.max_channel_name_length,
max_event_channels_at_once: app.max_event_channels_at_once,
max_event_name_length: app.max_event_name_length,
max_event_payload_in_kb: app.max_event_payload_in_kb,
max_event_batch_size: app.max_event_batch_size,
enable_user_authentication: app.enable_user_authentication,
enable_watchlist_events: app.enable_watchlist_events,
allowed_origins: app.allowed_origins.clone(),
}
}
}| self.app_cache.remove(app_id).await; | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| /// Verify a user authentication signature for the given app and socket | ||
| pub async fn verify_user_signature( | ||
| &self, | ||
| app_id: &str, | ||
| socket_id: &SocketId, | ||
| signature: &str, | ||
| ) -> Result<bool> { | ||
| let app = self | ||
| .find_by_id(app_id) | ||
| .await? | ||
| .ok_or_else(|| Error::InvalidAppKey)?; | ||
|
|
||
| // Create string to sign: socket_id | ||
| let string_to_sign = format!("{socket_id}::user::{signature}"); | ||
|
|
There was a problem hiding this comment.
The user signature verification logic is incorrect. The string to be signed should not contain the signature itself. According to the Pusher protocol, for user authentication, the string to sign is socket_id::user::user_data. This method is missing the user_data parameter.
The current implementation format!("{socket_id}::user::{signature}") will always fail verification. This is a critical security flaw.
pub async fn verify_user_signature(
&self,
app_id: &str,
socket_id: &SocketId,
user_data: &str,
signature: &str,
) -> Result<bool> {
let app = self
.find_by_id(app_id)
.await?
.ok_or_else(|| Error::InvalidAppKey)?;
// Create string to sign: socket_id::user::user_data
let string_to_sign = format!("{socket_id}::user::{user_data}");
// Generate token
let token = Token::new(app.key.clone(), app.secret.clone());
// Verify
Ok(token.verify(&string_to_sign, signature))
}| rustls = { version = "0.23.29", features = ["ring"] } | ||
| tower = "0.5.2" | ||
| mimalloc = { version = "0.1.48", features = ["v3"] } | ||
| surrealdb = { version = "2.3.7", features = ["kv-mem", "kv-rocksdb", "protocol-ws", "protocol-http"] } |
There was a problem hiding this comment.
The surrealdb dependency includes kv-mem and kv-rocksdb features, which are for embedded database usage. Since the implementation connects to a remote SurrealDB instance via WebSockets, these features are unnecessary and can increase compile times and binary size. It's best to only include the features that are actually used.
| surrealdb = { version = "2.3.7", features = ["kv-mem", "kv-rocksdb", "protocol-ws", "protocol-http"] } | |
| surrealdb = { version = "2.3.7", features = ["protocol-ws", "protocol-http"] } |
There was a problem hiding this comment.
Code Review
This pull request introduces SurrealDB as a new backend option for the application manager. The changes are comprehensive, including a new SurrealDB app manager implementation, updates to configuration options and the factory, a database schema file, and documentation updates. The implementation is solid, but I've identified a few critical and high-severity issues, particularly concerning an incorrect user signature verification method, a bug in data model conversion, and missing persistence for webhooks. I've also included some medium-severity suggestions to improve schema consistency and code clarity. Addressing these points will ensure the new feature is robust and correct.
| pub async fn verify_user_signature( | ||
| &self, | ||
| app_id: &str, | ||
| socket_id: &SocketId, | ||
| signature: &str, | ||
| ) -> Result<bool> { | ||
| let app = self | ||
| .find_by_id(app_id) | ||
| .await? | ||
| .ok_or_else(|| Error::InvalidAppKey)?; | ||
|
|
||
| // Create string to sign: socket_id | ||
| let string_to_sign = format!("{socket_id}::user::{signature}"); | ||
|
|
||
| // Generate token | ||
| let token = Token::new(app.key.clone(), app.secret.clone()); | ||
|
|
||
| // Verify | ||
| Ok(token.verify(&string_to_sign, &signature)) | ||
| } |
There was a problem hiding this comment.
This user signature verification logic is critically flawed. The string being signed (string_to_sign) incorrectly includes the signature itself. According to the Pusher protocol and your README.md, the string to sign for user authentication should be socket_id::user::user_data_json_string.
This function is missing the user_data parameter, and its current implementation will cause all user authentication attempts to fail. The function signature and implementation need to be corrected to properly validate user tokens.
| #[derive(Debug, Clone, Serialize, Deserialize)] | ||
| struct AppRecord { | ||
| id: RecordId, | ||
| key: String, | ||
| secret: String, | ||
| max_connections: u32, | ||
| enable_client_messages: bool, | ||
| enabled: bool, | ||
| max_backend_events_per_second: Option<u32>, | ||
| max_client_events_per_second: u32, | ||
| max_read_requests_per_second: Option<u32>, | ||
| max_presence_members_per_channel: Option<u32>, | ||
| max_presence_member_size_in_kb: Option<u32>, | ||
| max_channel_name_length: Option<u32>, | ||
| max_event_channels_at_once: Option<u32>, | ||
| max_event_name_length: Option<u32>, | ||
| max_event_payload_in_kb: Option<u32>, | ||
| max_event_batch_size: Option<u32>, | ||
| enable_user_authentication: Option<bool>, | ||
| enable_watchlist_events: Option<bool>, | ||
| allowed_origins: Option<Vec<String>>, | ||
| } |
There was a problem hiding this comment.
The AppRecord struct is missing the webhooks field, which is present in the main App struct. This means that webhook configurations will not be persisted or retrieved when using the SurrealDB app manager, leading to a loss of functionality compared to other app managers. To fix this, you should add the webhooks field to AppRecord and update the into_app and From<&App> implementations accordingly.
| impl From<&App> for AppRecord { | ||
| fn from(app: &App) -> Self { | ||
| Self { | ||
| id: RecordId::from_table_key(&app.id, &app.id), | ||
| key: app.key.clone(), | ||
| secret: app.secret.clone(), | ||
| max_connections: app.max_connections, | ||
| enable_client_messages: app.enable_client_messages, | ||
| enabled: app.enabled, | ||
| max_backend_events_per_second: app.max_backend_events_per_second, | ||
| max_client_events_per_second: app.max_client_events_per_second, | ||
| max_read_requests_per_second: app.max_read_requests_per_second, | ||
| max_presence_members_per_channel: app.max_presence_members_per_channel, | ||
| max_presence_member_size_in_kb: app.max_presence_member_size_in_kb, | ||
| max_channel_name_length: app.max_channel_name_length, | ||
| max_event_channels_at_once: app.max_event_channels_at_once, | ||
| max_event_name_length: app.max_event_name_length, | ||
| max_event_payload_in_kb: app.max_event_payload_in_kb, | ||
| max_event_batch_size: app.max_event_batch_size, | ||
| enable_user_authentication: app.enable_user_authentication, | ||
| enable_watchlist_events: app.enable_watchlist_events, | ||
| allowed_origins: app.allowed_origins.clone(), | ||
| } | ||
| } |
There was a problem hiding this comment.
The From<&App> for AppRecord implementation incorrectly constructs the RecordId. The line id: RecordId::from_table_key(&app.id, &app.id) uses the app's ID as the table name, which is incorrect. While SurrealDB might ignore the id field in the content when an ID is provided to the create method, this is misleading and error-prone.
A better approach is to recognize that the id field is primarily for deserialization. You can make it defaultable and not set it during creation.
I suggest adding Default to the derive macro for AppRecord and #[serde(default)] to the id field, then using Default::default() in this From implementation.
impl From<&App> for AppRecord {
fn from(app: &App) -> Self {
Self {
id: Default::default(), // This will be ignored by SurrealDB during creation but is needed for Default trait.
key: app.key.clone(),
secret: app.secret.clone(),
max_connections: app.max_connections,
enable_client_messages: app.enable_client_messages,
enabled: app.enabled,
max_backend_events_per_second: app.max_backend_events_per_second,
max_client_events_per_second: app.max_client_events_per_second,
max_read_requests_per_second: app.max_read_requests_per_second,
max_presence_members_per_channel: app.max_presence_members_per_channel,
max_presence_member_size_in_kb: app.max_presence_member_size_in_kb,
max_channel_name_length: app.max_channel_name_length,
max_event_channels_at_once: app.max_event_channels_at_once,
max_event_name_length: app.max_event_name_length,
max_event_payload_in_kb: app.max_event_payload_in_kb,
max_event_batch_size: app.max_event_batch_size,
enable_user_authentication: app.enable_user_authentication,
enable_watchlist_events: app.enable_watchlist_events,
allowed_origins: app.allowed_origins.clone(),
}
}
}| async fn ensure_schema(&self) -> Result<()> { | ||
| info!("Setting up SurrealDB schema for applications"); | ||
|
|
||
| // Create unique index on the key field for fast lookups | ||
| let index_query = format!( | ||
| "DEFINE INDEX unique_key ON TABLE {} COLUMNS key UNIQUE", | ||
| self.config.table_name | ||
| ); | ||
|
|
||
| self.db | ||
| .query(index_query) | ||
| .await | ||
| .map_err(|e| Error::Internal(format!("Failed to create unique key index: {e}")))?; | ||
|
|
||
| // Create index on enabled field for filtering | ||
| let enabled_index_query = format!( | ||
| "DEFINE INDEX enabled_idx ON TABLE {} COLUMNS enabled", | ||
| self.config.table_name | ||
| ); | ||
|
|
||
| self.db | ||
| .query(enabled_index_query) | ||
| .await | ||
| .map_err(|e| Error::Internal(format!("Failed to create enabled index: {e}")))?; | ||
|
|
||
| info!("SurrealDB schema setup completed"); | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
The ensure_schema function is inconsistent with the sql/init-srql.surrealql script. It's missing the creation of an index on the created_at field (created_at_idx). While the SQL script can be run manually, the application should ensure all necessary indexes exist for robust operation and to prevent performance issues with time-based queries.
| DEFINE TABLE applications SCHEMAFULL; | ||
|
|
||
| -- Define all fields with their types and constraints | ||
| DEFINE FIELD id ON TABLE applications TYPE string; |
There was a problem hiding this comment.
The definition DEFINE FIELD id ON TABLE applications TYPE string; is redundant. SurrealDB records inherently have a unique ID (e.g., applications:some_id), which your application code correctly uses via the RecordId type. Defining id as a separate field in the schema is unnecessary and can be confusing. It's best to remove this line and rely on the built-in record ID.
| -- DEFINE ACCESS applications ON DATABASE TYPE RECORD | ||
| -- SIGNUP (CREATE user SET email = $email, pass = crypto::argon2::generate($pass)) | ||
| -- SIGNIN (SELECT * FROM user WHERE email = $email AND crypto::argon2::compare(pass, $pass)) | ||
| -- SELECT, UPDATE, DELETE WHERE user = $auth.id; No newline at end of file |
There was a problem hiding this comment.
The file ends without a final newline character. It's a common convention and good practice to end text files with a newline.
-- SELECT, UPDATE, DELETE WHERE user = $auth.id;
|
@MarcEspiard can you take a look? I am too tired :( |
Description
Type of Change
Testing
Checklist
Build Artifacts
Additional Notes