feat: sqlite hardcoded #39
Draft
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
by geoffreycopin/rqlite, thank u 💋
# Conflicts: # Cargo.lock # Cargo.toml # database/src/bindings/mod.rs # database/src/lib.rs # shadowsniff/browsers/src/lib.rs # utils/src/lib.rs
Owner
Author
|
@sourcery-ai review |
Reviewer's GuideThis PR replaces the previous C-bindings based SQLite reader with a self-contained, pure Rust implementation under a new ER diagram for new SQLite table metadata extractionerDiagram
TABLE_METADATA {
name string
columns ColumnDef[]
first_page int
}
COLUMN_DEF {
name string
col_type Type
}
TABLE_METADATA ||--o{ COLUMN_DEF : has
COLUMN_DEF }o--|| TYPE : uses
Class diagram for new SQLite pure Rust implementationclassDiagram
class SqliteDatabase {
- tables_metadata: Vec<TableMetadata>
- pager: Pager
+ scanner(page: usize) Scanner
+ read_table<S>(table_name: S) Option<SqliteTableIterator>
}
class TableMetadata {
+ name: Arc<str>
+ columns: Vec<ColumnDef>
+ first_page: usize
+ from_cursor(cursor: Cursor) -> Option<TableMetadata>
}
class SqliteTableIterator {
- fields: Vec<usize>
- scanner: Scanner
- row_buffer: Vec<Value>
+ next() Option<SqliteTableRecord>
}
class SqliteTableRecord {
- row: Arc<[Value]>
+ get_value(key: usize) Option<Value>
}
class Pager {
+ read_page(n: usize) Arc<Page>
+ read_overflow(n: usize) Arc<OverflowPage>
}
class Scanner {
+ next_record() Option<Cursor>
}
class Cursor {
+ field(n: usize) Option<Value>
}
class Value {
+ String(Arc<str>)
+ Integer(i64)
+ Float(f64)
+ Blob(Arc<[u8]>)
+ Null
+ as_str() Option<Arc<str>>
+ as_integer() Option<i64>
+ as_blob() Option<Arc<[u8]>>
}
SqliteDatabase --> TableMetadata
SqliteDatabase --> Pager
SqliteDatabase --> SqliteTableIterator
SqliteTableIterator --> Scanner
SqliteTableIterator --> SqliteTableRecord
SqliteTableRecord --> Value
Pager --> Page
Pager --> OverflowPage
Scanner --> Cursor
Cursor --> Value
TableMetadata --> ColumnDef
ColumnDef --> Type
Class diagram for refactored Value and browser data typesclassDiagram
class Value {
+ String(Arc<str>)
+ Integer(i64)
+ Float(f64)
+ Blob(Arc<[u8]>)
+ Null
+ as_str() Option<Arc<str>>
+ as_integer() Option<i64>
+ as_blob() Option<Arc<[u8]>>
}
class Cookie {
+ host_key: Arc<str>
+ name: Arc<str>
+ value: Arc<str>
+ path: Arc<str>
+ expires_utc: i64
}
class AutoFill {
+ name: Arc<str>
+ value: Arc<str>
+ last_used: i64
}
class CreditCard {
+ name_on_card: Arc<str>
+ expiration_month: i64
+ expiration_year: i64
+ card_number: Arc<str>
+ use_count: i64
}
class Download {
+ saved_as: Arc<str>
+ url: Arc<str>
}
class Password {
+ origin: Option<Arc<str>>
+ username: Option<Arc<str>>
+ password: Option<Arc<str>>
}
class History {
+ url: Arc<str>
+ title: Arc<str>
+ last_visit_time: i64
}
Class diagram for new half-io VecReader utilityclassDiagram
class VecReader {
- buffer: Vec<u8>
- pos: usize
+ new(data: Vec<u8>)
+ read(buf: &mut [u8]) -> usize
+ seek(pos: SeekFrom) -> u64
}
VecReader ..|> Read
VecReader ..|> Seek
VecReader ..|> ErrorType
File-Level Changes
Assessment against linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @sqlerrorthing - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- SqliteTableIterator::next() always returns None. (link)
- read_i24_at uses i32::from_be_bytes on a 3-byte slice, which will panic. (link)
- read_i48_at uses i64::from_be_bytes on a 6-byte slice, which will panic. (link)
General comments:
- SqliteTableIterator.next currently returns None unconditionally—update it to return Some(SqliteTableRecord) after filling the row_buffer.
- row_buffer is allocated with length zero (you capture its length before pushing fields) so size it using the number of fields (e.g. fields.len()) to match the number of columns.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- SqliteTableIterator.next currently returns None unconditionally—update it to return Some(SqliteTableRecord) after filling the row_buffer.
- row_buffer is allocated with length zero (you capture its length before pushing fields) so size it using the number of fields (e.g. fields.len()) to match the number of columns.
## Individual Comments
### Comment 1
<location> `database/src/sqlite/db.rs:83` </location>
<code_context>
-impl Iterator for SqliteIterator {
- type Item = SqliteRow;
-
- fn next(&mut self) -> Option<Self::Item> {
- self.rows.next()
- }
</code_context>
<issue_to_address>
SqliteTableIterator::next() always returns None.
Currently, next() does not yield any items, resulting in an empty iterator. Consider returning Some(SqliteTableRecord { ... }) after populating row_buffer.
</issue_to_address>
### Comment 2
<location> `half-io/src/lib.rs:42` </location>
<code_context>
+
+impl Read for VecReader {
+ fn read(&mut self, buf: &mut [u8]) -> Result<usize, Self::Error> {
+ let available = self.buffer.len().saturating_add(self.pos);
+ let to_read = core::cmp::min(available, buf.len());
+
</code_context>
<issue_to_address>
Incorrect calculation of available bytes in VecReader::read().
The calculation should use buffer.len().saturating_sub(self.pos) to correctly determine the remaining bytes to read.
</issue_to_address>
### Comment 3
<location> `half-io/src/lib.rs:49` </location>
<code_context>
+ return Ok(0);
+ }
+
+ buf[..to_read].copy_from_slice(&self.buffer[self.pos..self.pos + to_read]);
+ self.pos += to_read;
+
</code_context>
<issue_to_address>
Potential out-of-bounds access in VecReader::read().
Ensure that self.pos + to_read does not exceed self.buffer.len() to prevent panics. Correcting the available bytes calculation should address this, but always validate slice bounds.
</issue_to_address>
### Comment 4
<location> `database/src/sqlite/cursor.rs:174` </location>
<code_context>
+ i16::from_be_bytes(input[offset..offset + 2].try_into().unwrap()) as i64
+}
+
+fn read_i24_at(input: &[u8], offset: usize) -> i64 {
+ (i32::from_be_bytes(input[offset..offset + 3].try_into().unwrap()) & 0x00FFFFFF) as i64
+}
</code_context>
<issue_to_address>
read_i24_at uses i32::from_be_bytes on a 3-byte slice, which will panic.
i32::from_be_bytes requires a 4-byte array; passing a 3-byte slice will cause a panic. Manually assemble the 24-bit value from the bytes instead.
</issue_to_address>
### Comment 5
<location> `database/src/sqlite/cursor.rs:182` </location>
<code_context>
+ i32::from_be_bytes(input[offset..offset + 4].try_into().unwrap()) as i64
+}
+
+fn read_i48_at(input: &[u8], offset: usize) -> i64 {
+ i64::from_be_bytes(input[offset..offset + 6].try_into().unwrap()) & 0x0000FFFFFFFFFFFF
+}
</code_context>
<issue_to_address>
read_i48_at uses i64::from_be_bytes on a 6-byte slice, which will panic.
You must pad the 6-byte slice to 8 bytes before calling i64::from_be_bytes to avoid a runtime panic.
</issue_to_address>
### Comment 6
<location> `database/src/sqlite/cursor.rs:148` </location>
<code_context>
+ record_field.offset,
+ ))),
+ RecordFieldType::String(length) => {
+ let value = str::from_utf8(
+ &self.payload[record_field.offset..record_field.offset + length],
+ )
</code_context>
<issue_to_address>
No bounds check for payload slice in String/Blob field extraction.
Add a check to ensure record_field.offset + length does not exceed payload length to prevent panics with malformed data.
</issue_to_address>
### Comment 7
<location> `database/src/sqlite/pager.rs:124` </location>
<code_context>
+ }
+
+ fn load_raw(&self, n: usize) -> anyhow::Result<Vec<u8>> {
+ let offset = n.saturating_sub(1) * self.header.page_size as usize;
+
+ let mut input_guard = self
</code_context>
<issue_to_address>
Pager::load_raw() does not check for out-of-bounds page numbers.
Validate that n and offset are within valid bounds to prevent panics or invalid data access.
</issue_to_address>
### Comment 8
<location> `database/src/sqlite/sql/parser.rs:10` </location>
<code_context>
+use anyhow::{bail, Context};
+
+#[derive(Debug)]
+struct ParserState {
+ tokens: Vec<Token>,
+ pos: usize,
</code_context>
<issue_to_address>
Consider replacing the custom ParserState logic with a concise regex-based approach for parsing CREATE TABLE statements.
Here’s a sketch of how you can drop the entire `ParserState` machinery and replace it with a ~30 LOC regex + split approach. It covers the same functionality (extract table name + cols + types) and will be far easier to maintain:
```rust
use crate::sqlite::sql::ast::{ColumnDef, CreateTableStatement, Type};
use anyhow::{bail, Context, Result};
use regex::Regex;
lazy_static::lazy_static! {
// case‐insensitive CREATE TABLE (name) (col1 TYPE, col2 TYPE, …)
static ref CREATE_RE: Regex =
Regex::new(r"(?i)^\s*CREATE\s+TABLE\s+([A-Za-z_]\w*)\s*\((.+)\)\s*$").unwrap();
}
pub fn parse_create_statement(input: &str) -> Result<CreateTableStatement> {
let caps = CREATE_RE
.captures(input)
.context("not a CREATE TABLE statement")?;
let name = caps[1].to_string();
let cols = &caps[2];
let columns = cols
.split(',')
.map(str::trim)
.map(|col_def| {
let mut parts = col_def.split_whitespace();
let col_name = parts
.next()
.context("missing column name")?
.to_string();
let typ = parts
.next()
.map(str::to_lowercase)
.context("missing column type")?;
let col_type = match typ.as_str() {
"integer" => Type::Integer,
"real" => Type::Real,
"blob" => Type::Blob,
"text" | "string" => Type::Text,
other => bail!("unsupported type: {}", other),
};
Ok(ColumnDef {
name: col_name,
col_type,
})
})
.collect::<Result<Vec<_>>>()?;
Ok(CreateTableStatement { name, columns })
}
```
Steps to adopt:
1. Remove `ParserState`, tokenizer imports and all its helper methods.
2. Add the `CREATE_RE` regex above.
3. Replace your existing `parse_create_statement` with the snippet above.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| impl Iterator for SqliteTableIterator { | ||
| type Item = SqliteTableRecord; | ||
|
|
||
| fn next(&mut self) -> Option<Self::Item> { |
There was a problem hiding this comment.
issue (bug_risk): SqliteTableIterator::next() always returns None.
Currently, next() does not yield any items, resulting in an empty iterator. Consider returning Some(SqliteTableRecord { ... }) after populating row_buffer.
|
|
||
| impl Read for VecReader { | ||
| fn read(&mut self, buf: &mut [u8]) -> Result<usize, Self::Error> { | ||
| let available = self.buffer.len().saturating_add(self.pos); |
There was a problem hiding this comment.
issue (bug_risk): Incorrect calculation of available bytes in VecReader::read().
The calculation should use buffer.len().saturating_sub(self.pos) to correctly determine the remaining bytes to read.
| return Ok(0); | ||
| } | ||
|
|
||
| buf[..to_read].copy_from_slice(&self.buffer[self.pos..self.pos + to_read]); |
There was a problem hiding this comment.
issue (bug_risk): Potential out-of-bounds access in VecReader::read().
Ensure that self.pos + to_read does not exceed self.buffer.len() to prevent panics. Correcting the available bytes calculation should address this, but always validate slice bounds.
| i16::from_be_bytes(input[offset..offset + 2].try_into().unwrap()) as i64 | ||
| } | ||
|
|
||
| fn read_i24_at(input: &[u8], offset: usize) -> i64 { |
There was a problem hiding this comment.
issue (bug_risk): read_i24_at uses i32::from_be_bytes on a 3-byte slice, which will panic.
i32::from_be_bytes requires a 4-byte array; passing a 3-byte slice will cause a panic. Manually assemble the 24-bit value from the bytes instead.
| i32::from_be_bytes(input[offset..offset + 4].try_into().unwrap()) as i64 | ||
| } | ||
|
|
||
| fn read_i48_at(input: &[u8], offset: usize) -> i64 { |
There was a problem hiding this comment.
issue (bug_risk): read_i48_at uses i64::from_be_bytes on a 6-byte slice, which will panic.
You must pad the 6-byte slice to 8 bytes before calling i64::from_be_bytes to avoid a runtime panic.
| record_field.offset, | ||
| ))), | ||
| RecordFieldType::String(length) => { | ||
| let value = str::from_utf8( |
There was a problem hiding this comment.
issue (bug_risk): No bounds check for payload slice in String/Blob field extraction.
Add a check to ensure record_field.offset + length does not exceed payload length to prevent panics with malformed data.
| } | ||
|
|
||
| fn load_raw(&self, n: usize) -> anyhow::Result<Vec<u8>> { | ||
| let offset = n.saturating_sub(1) * self.header.page_size as usize; |
There was a problem hiding this comment.
issue (bug_risk): Pager::load_raw() does not check for out-of-bounds page numbers.
Validate that n and offset are within valid bounds to prevent panics or invalid data access.
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.
Closes #38