Create new tables and basic database impls #311
Merged
+1,538
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
Caution Review failedThe pull request is closed. WalkthroughAdds a SQL migration creating five new tables with AFTER UPDATE triggers; adds three Rust DB submodules (accounts_new, relays_new, users_new), sqlx::FromRow mappings with parsing/validation, new AccountNew type and nostr MLS wiring, a DatabaseError::InvalidTimestamp variant, and comprehensive unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Whitenoise as Whitenoise API
participant SQL as SQLite/sqlx
participant FromRow as sqlx::FromRow impl
participant Parser as Parsers (pubkey/json/url/timestamps)
App->>Whitenoise: load_account_new(pubkey)
Whitenoise->>SQL: SELECT ... FROM accounts_new WHERE pubkey = ?
SQL->>FromRow: row -> FromRow::from_row
FromRow->>Parser: parse pubkey, settings, timestamps
alt all parses succeed
Parser-->>FromRow: typed values
FromRow-->>Whitenoise: AccountNewRow
Whitenoise->>App: Ok(AccountNew) (with nostr_mls created)
else parse/convert failure
Parser-->>FromRow: error (e.g., ColumnDecode/InvalidTimestamp)
FromRow-->>Whitenoise: Err(sqlx::Error::ColumnDecode)
Whitenoise->>App: Err(...)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~35 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
src/whitenoise/database/accounts_new.rs (1)
1-69: Add unit tests for AccountNewRowThis module lacks unit tests, unlike
relays_new.rsandusers_new.rswhich have comprehensive test coverage. Consider adding tests for:
- Valid data mapping
- Invalid pubkey handling
- Invalid settings JSON handling
- Timestamp edge cases (0, future timestamps, invalid timestamps)
Would you like me to generate a comprehensive test suite similar to the ones in the other modules?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
db_migrations/0005_new_tables.sql(1 hunks)src/whitenoise/database/accounts_new.rs(1 hunks)src/whitenoise/database/mod.rs(2 hunks)src/whitenoise/database/relays_new.rs(1 hunks)src/whitenoise/database/users_new.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/whitenoise/database/accounts_new.rs (2)
src/whitenoise/database/users_new.rs (1)
from_row(27-73)src/whitenoise/database/relays_new.rs (1)
from_row(25-62)
src/whitenoise/database/relays_new.rs (3)
src/whitenoise/database/accounts_new.rs (1)
from_row(30-67)src/whitenoise/database/users_new.rs (1)
from_row(27-73)src/whitenoise/database/mod.rs (1)
new(43-87)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: check (ubuntu-latest, native)
🔇 Additional comments (3)
src/whitenoise/database/mod.rs (1)
13-16: LGTM! Well-structured module organization and error handlingThe new public modules and
InvalidTimestamperror variant are properly integrated. The error message includes the problematic timestamp value which aids debugging.Also applies to: 31-32
src/whitenoise/database/relays_new.rs (1)
1-383: Excellent implementation with comprehensive test coverage!The
RelayNewRowimplementation demonstrates:
- Proper error handling for URL parsing and timestamp conversions
- Consistent use of
DatabaseError::InvalidTimestamp- Comprehensive test suite covering edge cases
- Clear error messages with appropriate column indices
src/whitenoise/database/users_new.rs (1)
1-348: Well-implemented with thorough error handling and testing!The
UserRowimplementation shows:
- Proper error handling for pubkey parsing, JSON deserialization, and timestamp conversions
- Consistent error mapping with appropriate column indices
- Comprehensive test coverage including invalid data scenarios
- Good use of the
DatabaseError::InvalidTimestamppattern
Comment on lines
1
to
10
| -- New accounts table | ||
| CREATE TABLE accounts_new ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| pubkey TEXT NOT NULL, -- Hex encoded nostr public key | ||
| user_id INTEGER NOT NULL, -- Foreign key to the users table | ||
| settings JSONB NOT NULL, | ||
| last_synced_at DATETIME, | ||
| created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP | ||
| ); |
Contributor
There was a problem hiding this comment.
Add foreign key constraint and indexes for accounts_new table
The table has a comment indicating user_id is a foreign key, but no actual foreign key constraint is defined. Also missing indexes on frequently queried columns.
Consider adding:
-- Add foreign key constraint
FOREIGN KEY (user_id) REFERENCES users_new(id),
-- Add indexes
CREATE INDEX idx_accounts_new_pubkey ON accounts_new(pubkey);
CREATE INDEX idx_accounts_new_user_id ON accounts_new(user_id);
CREATE UNIQUE INDEX idx_accounts_new_pubkey_user_id ON accounts_new(pubkey, user_id);🤖 Prompt for AI Agents
In db_migrations/0005_new_tables.sql around lines 1 to 10, the accounts_new
table defines user_id in a comment but lacks an actual foreign key constraint
and has no indexes; update the CREATE TABLE statement to declare user_id as a
foreign key referencing users_new(id) and then add indexes for pubkey and
user_id plus a unique composite index on (pubkey, user_id) to improve query
performance and enforce uniqueness.
Comment on lines
12
to
19
| -- New users table | ||
| CREATE TABLE users_new ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| pubkey TEXT NOT NULL, | ||
| metadata JSONB, | ||
| created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP | ||
| ); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add UNIQUE constraint and index on pubkey for users_new table
The pubkey field should likely be unique to prevent duplicate users.
CREATE TABLE users_new (
id INTEGER PRIMARY KEY AUTOINCREMENT,
- pubkey TEXT NOT NULL,
+ pubkey TEXT NOT NULL UNIQUE,
metadata JSONB,
created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP,
updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP
);
+
+-- Add index for pubkey lookups
+CREATE INDEX idx_users_new_pubkey ON users_new(pubkey);📝 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
| -- New users table | |
| CREATE TABLE users_new ( | |
| id INTEGER PRIMARY KEY AUTOINCREMENT, | |
| pubkey TEXT NOT NULL, | |
| metadata JSONB, | |
| created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP | |
| ); | |
| -- New users table | |
| CREATE TABLE users_new ( | |
| id INTEGER PRIMARY KEY AUTOINCREMENT, | |
| pubkey TEXT NOT NULL UNIQUE, | |
| metadata JSONB, | |
| created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP | |
| ); | |
| -- Add index for pubkey lookups | |
| CREATE INDEX idx_users_new_pubkey ON users_new(pubkey); |
🤖 Prompt for AI Agents
In db_migrations/0005_new_tables.sql around lines 12 to 19, the users_new table
lacks a uniqueness constraint on pubkey; update the migration so pubkey is
enforced unique (since it's NOT NULL) by adding a UNIQUE constraint to the
CREATE TABLE (e.g., include UNIQUE(pubkey)) or by adding a separate CREATE
UNIQUE INDEX idx_users_new_pubkey ON users_new(pubkey) after the table creation;
ensure the chosen approach matches the target DB dialect and migration ordering
so the unique constraint/index is created as part of this migration.
Comment on lines
21
to
28
| -- New account_followers table | ||
| CREATE TABLE account_followers ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| account_id INTEGER NOT NULL, | ||
| followed_id INTEGER NOT NULL, -- This is the user_id of the followed user | ||
| created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP | ||
| ); |
Contributor
There was a problem hiding this comment.
Add foreign key constraints and prevent duplicate follows
The account_followers table needs foreign key constraints and should prevent duplicate follow relationships.
CREATE TABLE account_followers (
id INTEGER PRIMARY KEY AUTOINCREMENT,
- account_id INTEGER NOT NULL,
- followed_id INTEGER NOT NULL,
+ account_id INTEGER NOT NULL REFERENCES accounts_new(id),
+ followed_id INTEGER NOT NULL REFERENCES users_new(id),
created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP,
- updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP
+ updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP,
+ UNIQUE(account_id, followed_id)
);
+
+-- Add indexes for foreign keys
+CREATE INDEX idx_account_followers_account_id ON account_followers(account_id);
+CREATE INDEX idx_account_followers_followed_id ON account_followers(followed_id);🤖 Prompt for AI Agents
In db_migrations/0005_new_tables.sql around lines 21 to 28, the
account_followers table lacks foreign key constraints and a uniqueness
constraint to prevent duplicate follow relationships; modify the CREATE TABLE to
add FOREIGN KEY (account_id) REFERENCES accounts(id) ON DELETE CASCADE and
FOREIGN KEY (followed_id) REFERENCES accounts(id) ON DELETE CASCADE, and add a
UNIQUE(account_id, followed_id) constraint to prevent duplicates (optionally
also add a CHECK(account_id <> followed_id) to prevent self-follows).
Comment on lines
+30
to
+36
| -- New relays table | ||
| CREATE TABLE relays ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| url TEXT NOT NULL, | ||
| created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP | ||
| ); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add UNIQUE constraint on URL for relays table
Relay URLs should be unique to prevent duplicates.
CREATE TABLE relays (
id INTEGER PRIMARY KEY AUTOINCREMENT,
- url TEXT NOT NULL,
+ url TEXT NOT NULL UNIQUE,
created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP,
updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP
);🤖 Prompt for AI Agents
In db_migrations/0005_new_tables.sql around lines 30 to 36, the relays table
allows duplicate URLs; add a UNIQUE constraint to the url column to prevent
duplicates by either declaring "url TEXT NOT NULL UNIQUE" in the CREATE TABLE
statement or adding "CREATE UNIQUE INDEX idx_relays_url ON relays(url);"
immediately after the table creation; pick the approach consistent with our
migrations (prefer inline UNIQUE in the CREATE TABLE) and ensure the migration
will fail if existing duplicate URLs exist so they can be resolved before
applying.
Comment on lines
+38
to
+46
| -- New user_relays table | ||
| CREATE TABLE user_relays ( | ||
| id INTEGER PRIMARY KEY AUTOINCREMENT, | ||
| user_id INTEGER NOT NULL, | ||
| relay_id INTEGER NOT NULL, | ||
| relay_type TEXT NOT NULL, | ||
| created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, | ||
| updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP | ||
| ); |
Contributor
There was a problem hiding this comment.
Add constraints and validation for user_relays table
The table needs foreign key constraints, unique constraint to prevent duplicates, and validation for relay_type.
CREATE TABLE user_relays (
id INTEGER PRIMARY KEY AUTOINCREMENT,
- user_id INTEGER NOT NULL,
- relay_id INTEGER NOT NULL,
- relay_type TEXT NOT NULL,
+ user_id INTEGER NOT NULL REFERENCES users_new(id),
+ relay_id INTEGER NOT NULL REFERENCES relays(id),
+ relay_type TEXT NOT NULL CHECK(relay_type IN ('read', 'write', 'both')),
created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP,
- updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP
+ updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP,
+ UNIQUE(user_id, relay_id, relay_type)
);
+
+-- Add indexes for foreign keys
+CREATE INDEX idx_user_relays_user_id ON user_relays(user_id);
+CREATE INDEX idx_user_relays_relay_id ON user_relays(relay_id);📝 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
| -- New user_relays table | |
| CREATE TABLE user_relays ( | |
| id INTEGER PRIMARY KEY AUTOINCREMENT, | |
| user_id INTEGER NOT NULL, | |
| relay_id INTEGER NOT NULL, | |
| relay_type TEXT NOT NULL, | |
| created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP | |
| ); | |
| -- New user_relays table | |
| CREATE TABLE user_relays ( | |
| id INTEGER PRIMARY KEY AUTOINCREMENT, | |
| user_id INTEGER NOT NULL REFERENCES users_new(id), | |
| relay_id INTEGER NOT NULL REFERENCES relays(id), | |
| relay_type TEXT NOT NULL CHECK(relay_type IN ('read', 'write', 'both')), | |
| created_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| updated_at DATETIME NOT NULL DEFAULT CURRENT_TIMESTAMP, | |
| UNIQUE(user_id, relay_id, relay_type) | |
| ); | |
| -- Add indexes for foreign keys | |
| CREATE INDEX idx_user_relays_user_id ON user_relays(user_id); | |
| CREATE INDEX idx_user_relays_relay_id ON user_relays(relay_id); |
🤖 Prompt for AI Agents
In db_migrations/0005_new_tables.sql around lines 38 to 46, the user_relays
table needs referential integrity and validation: modify the CREATE TABLE to add
FOREIGN KEY (user_id) REFERENCES users(id) ON DELETE CASCADE and FOREIGN KEY
(relay_id) REFERENCES relays(id) ON DELETE CASCADE, add a UNIQUE constraint on
(user_id, relay_id) to prevent duplicate entries, and add a CHECK constraint on
relay_type restricting it to the allowed set (e.g. CHECK (relay_type IN
('TYPE_A','TYPE_B')) — replace TYPE_A/TYPE_B with your app’s actual relay type
values); ensure the constraints are included in the CREATE TABLE statement so
migrations run atomically.
jgmontoya
approved these changes
Aug 11, 2025
delcin-raj
reviewed
Aug 11, 2025
| ); | ||
|
|
||
| -- New relays table | ||
| CREATE TABLE relays ( |
Contributor
There was a problem hiding this comment.
It's best to not have this table.
A single table user_relays with user_id being a foreign key referenceing users table with
UNIQUE (user_id, url)constraint will be optimal
Contributor
There was a problem hiding this comment.
a single relay can belong to multiple lists so we're using the relay table to avoid having to duplicate the relay information/url. It will also allow us to do some additional stuff in the future if we want, like this: #296 (comment)
Contributor
There was a problem hiding this comment.
Then we need a Unique(user_id, relay_id, relay_type) constraint for the user_relays table.
No need to have id PRIMARY KEY
This was referenced Aug 11, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We'll change the names of the files and tables to drop the
newparts once we've fully migrated thingsSummary by CodeRabbit