Skip to content

Commit

Permalink
Overhaul schema command, remove database name (#7344)
Browse files Browse the repository at this point in the history
This PR changes the `schema` command for viewing the schema of a SQLite
database file. It removes 1 level of nesting (intended to handle
multiple databases in the same connection) that I believe is
unnecessary.

### Before

![image](https://user-images.githubusercontent.com/26268125/205467643-05df0f64-bc8f-4135-9ff1-f978cc7a12bd.png)

### After

![image](https://user-images.githubusercontent.com/26268125/205467655-c4783184-9bde-46e2-9316-0f06acd1abe1.png)

## Rationale

A SQLite database connection can technically be associated with multiple
non-temporary databases using [the ATTACH DATABASE
command](https://www.sqlite.org/lang_attach.html). But it's not possible
to do that _in the context of Nushell_, and so I believe that there is
no benefit to displaying the schema as if there could be multiple
databases.

I initially raised this concern back in April, but we decided to keep
the database nesting because at the time we were still looking into more
generalized database functionality (i.e. not just SQLite). I believe
that rationale no longer applies.

Also, the existing code would not have worked correctly even if a
connection had multiple databases; for every database, it was looking up
tables without filtering them by database:

https://github.com/nushell/nushell/blob/6295b205450bde26ed62f08cc825005c2267d90d/crates/nu-command/src/database/values/sqlite.rs#L104-L118

## Future Work

I'd like to add information on views+triggers to the `schema` output.
I'm also working on making it possible to `ctrl+c` reading from a
database (which is turning into a massive yak shave).
  • Loading branch information
rgwood committed Dec 4, 2022
1 parent 6295b20 commit e8a55aa
Show file tree
Hide file tree
Showing 4 changed files with 49 additions and 139 deletions.
129 changes: 48 additions & 81 deletions crates/nu-command/src/database/commands/schema.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::super::SQLiteDatabase;
use crate::database::values::definitions::{db::Db, db_row::DbRow, db_table::DbTable};
use crate::database::values::definitions::{db_row::DbRow, db_table::DbTable};
use nu_protocol::{
ast::Call,
engine::{Command, EngineState, Stack},
Expand All @@ -22,7 +22,7 @@ impl Command for SchemaDb {
}

fn usage(&self) -> &str {
"Show SQLite database information, including its schema."
"Show the schema of a SQLite database."
}

fn examples(&self) -> Vec<Example> {
Expand Down Expand Up @@ -50,77 +50,64 @@ impl Command for SchemaDb {

let sqlite_db = SQLiteDatabase::try_from_pipeline(input, span)?;
let conn = open_sqlite_db_connection(&sqlite_db, span)?;
let dbs = get_databases_and_tables(&sqlite_db, &conn, span)?;
let tables = sqlite_db.get_tables(&conn).map_err(|e| {
ShellError::GenericError(
"Error reading tables".into(),
e.to_string(),
Some(span),
None,
Vec::new(),
)
})?;

cols.push("db_filename".into());
vals.push(Value::String {
val: sqlite_db.path.to_string_lossy().into(),
span,
});
let mut table_names = vec![];
let mut table_values = vec![];
for table in tables {
let column_info = get_table_columns(&sqlite_db, &conn, &table, span)?;
let constraint_info = get_table_constraints(&sqlite_db, &conn, &table, span)?;
let foreign_key_info = get_table_foreign_keys(&sqlite_db, &conn, &table, span)?;
let index_info = get_table_indexes(&sqlite_db, &conn, &table, span)?;

for db in dbs {
let tables = get_database_tables(&db);
let mut table_list: Vec<Value> = vec![];
let mut table_names = vec![];
let mut table_values = vec![];
for table in tables {
let column_info = get_table_columns(&sqlite_db, &conn, &table, span)?;
let constraint_info = get_table_constraints(&sqlite_db, &conn, &table, span)?;
let foreign_key_info = get_table_foreign_keys(&sqlite_db, &conn, &table, span)?;
let index_info = get_table_indexes(&sqlite_db, &conn, &table, span)?;
let mut cols = vec![];
let mut vals = vec![];

table_names.push(table.name);
table_values.push(Value::Record {
cols: vec![
"columns".into(),
"constraints".into(),
"foreign_keys".into(),
"indexes".into(),
],
vals: vec![
Value::List {
vals: column_info,
span,
},
Value::List {
vals: constraint_info,
span,
},
Value::List {
vals: foreign_key_info,
span,
},
Value::List {
vals: index_info,
span,
},
],
span,
});
}
table_list.push(Value::Record {
cols: table_names,
vals: table_values,
cols.push("columns".into());
vals.push(Value::List {
vals: column_info,
span,
});

cols.push("databases".into());

let mut rcols = vec![];
let mut rvals = vec![];
rcols.push("name".into());
rvals.push(Value::string(db.name().to_string(), span));
cols.push("constraints".into());
vals.push(Value::List {
vals: constraint_info,
span,
});

rcols.push("tables".into());
rvals.append(&mut table_list);
cols.push("foreign_keys".into());
vals.push(Value::List {
vals: foreign_key_info,
span,
});

vals.push(Value::Record {
cols: rcols,
vals: rvals,
cols.push("indexes".into());
vals.push(Value::List {
vals: index_info,
span,
});

table_names.push(table.name);
table_values.push(Value::Record { cols, vals, span });
}

cols.push("tables".into());
vals.push(Value::Record {
cols: table_names,
vals: table_values,
span,
});

// TODO: add views and triggers

Ok(PipelineData::Value(
Value::Record { cols, vals, span },
None,
Expand All @@ -140,26 +127,6 @@ fn open_sqlite_db_connection(db: &SQLiteDatabase, span: Span) -> Result<Connecti
})
}

fn get_databases_and_tables(
db: &SQLiteDatabase,
conn: &Connection,
span: Span,
) -> Result<Vec<Db>, ShellError> {
db.get_databases_and_tables(conn).map_err(|e| {
ShellError::GenericError(
"Error getting databases and tables".into(),
e.to_string(),
Some(span),
None,
Vec::new(),
)
})
}

fn get_database_tables(db: &Db) -> Vec<DbTable> {
db.tables()
}

fn get_table_columns(
db: &SQLiteDatabase,
conn: &Connection,
Expand Down
27 changes: 0 additions & 27 deletions crates/nu-command/src/database/values/definitions/db.rs

This file was deleted.

1 change: 0 additions & 1 deletion crates/nu-command/src/database/values/definitions/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
pub mod db;
pub mod db_column;
pub mod db_constraint;
pub mod db_foreignkey;
Expand Down
31 changes: 1 addition & 30 deletions crates/nu-command/src/database/values/sqlite.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::definitions::{
db::Db, db_column::DbColumn, db_constraint::DbConstraint, db_foreignkey::DbForeignKey,
db_column::DbColumn, db_constraint::DbConstraint, db_foreignkey::DbForeignKey,
db_index::DbIndex, db_table::DbTable,
};

Expand Down Expand Up @@ -101,35 +101,6 @@ impl SQLiteDatabase {
Ok(conn)
}

pub fn get_databases_and_tables(&self, conn: &Connection) -> Result<Vec<Db>, rusqlite::Error> {
let mut db_query = conn.prepare("SELECT name FROM pragma_database_list")?;

let databases = db_query.query_map([], |row| {
let name: String = row.get(0)?;
Ok(Db::new(name, self.get_tables(conn)?))
})?;

let mut db_list = vec![];
for db in databases {
db_list.push(db?);
}

Ok(db_list)
}

pub fn get_databases(&self, conn: &Connection) -> Result<Vec<String>, rusqlite::Error> {
let mut db_query = conn.prepare("SELECT name FROM pragma_database_list")?;

let mut db_list = vec![];
let _ = db_query.query_map([], |row| {
let name: String = row.get(0)?;
db_list.push(name);
Ok(())
})?;

Ok(db_list)
}

pub fn get_tables(&self, conn: &Connection) -> Result<Vec<DbTable>, rusqlite::Error> {
let mut table_names =
conn.prepare("SELECT name FROM sqlite_master WHERE type = 'table'")?;
Expand Down

0 comments on commit e8a55aa

Please sign in to comment.