mirror of
https://github.com/lancedb/lancedb.git
synced 2026-09-22 04:55:39 +00:00
fix: return InvalidTableName instead of panicking in open_table/create_table (#4192)
Passing an invalid table name to `open_table` or `create_table` panics
instead of returning an error:
thread '...' panicked at rust/lancedb/src/database/listing.rs:1155:62:
called `Result::unwrap()` on an `Err` value: InvalidTableName { name:
"my table", ... }
Both call sites build the table URI with
`request.location.clone().unwrap_or_else(||
self.table_uri(&request.name).unwrap())`, and `table_uri` is the
function that validates the name — so every rejected name (empty,
spaces, slashes, non-ASCII) hits the inner `unwrap`.
`Error::InvalidTableName` clearly is the intended contract here: the
variant exists for exactly this, and the Python binding maps it to
`ValueError`.
Replaced the closure with a `match` that propagates the validation
error; behavior with an explicit `location` is unchanged (the name is
not validated on that path, as before). Added tests asserting
`InvalidTableName` for `create_table` and `open_table` over a set of
rejected names — both panic without the fix. Full `cargo test -p lancedb
--lib --features remote`: 1214 passed; clippy/fmt clean; `cargo check
--workspace --all-targets` clean.
Co-authored-by: Xuanwo <github@xuanwo.io>
This commit is contained in:
@@ -1033,10 +1033,10 @@ impl Database for ListingDatabase {
|
||||
return self.namespace_database().create_table(request).await;
|
||||
}
|
||||
// Use provided location if available, otherwise derive from table name
|
||||
let table_uri = request
|
||||
.location
|
||||
.clone()
|
||||
.unwrap_or_else(|| self.table_uri(&request.name).unwrap());
|
||||
let table_uri = match request.location.clone() {
|
||||
Some(location) => location,
|
||||
None => self.table_uri(&request.name)?,
|
||||
};
|
||||
|
||||
let mut write_params = request
|
||||
.write_options
|
||||
@@ -1149,10 +1149,10 @@ impl Database for ListingDatabase {
|
||||
return self.namespace_database().open_table(request).await;
|
||||
}
|
||||
// Use provided location if available, otherwise derive from table name
|
||||
let table_uri = request
|
||||
.location
|
||||
.clone()
|
||||
.unwrap_or_else(|| self.table_uri(&request.name).unwrap());
|
||||
let table_uri = match request.location.clone() {
|
||||
Some(location) => location,
|
||||
None => self.table_uri(&request.name)?,
|
||||
};
|
||||
|
||||
// Only modify the storage options if we actually have something to
|
||||
// inherit. There is a difference between storage_options=None and
|
||||
@@ -1696,6 +1696,64 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
/// The names a table cannot have. A name is what the database builds the table's
|
||||
/// location out of, so one it cannot build a location from is refused rather than
|
||||
/// turned into some other path.
|
||||
const INVALID_TABLE_NAMES: [&str; 4] = ["", "has space", "bad/name", "a!b"];
|
||||
|
||||
/// Creating a table under an invalid name is an error the caller can handle, not a
|
||||
/// panic: the name comes from the caller, and the bindings turn the error into their
|
||||
/// own (`ValueError` in Python).
|
||||
#[tokio::test]
|
||||
async fn test_create_table_rejects_invalid_names() {
|
||||
let (_tempdir, db) = setup_database().await;
|
||||
let schema = Arc::new(Schema::new(vec![Field::new("id", DataType::Int32, false)]));
|
||||
|
||||
for name in INVALID_TABLE_NAMES {
|
||||
let result = db
|
||||
.create_table(CreateTableRequest {
|
||||
name: name.to_string(),
|
||||
namespace_path: vec![],
|
||||
data: Box::new(RecordBatch::new_empty(schema.clone())) as Box<dyn Scannable>,
|
||||
mode: CreateTableMode::Create,
|
||||
write_options: Default::default(),
|
||||
location: None,
|
||||
namespace_client: None,
|
||||
})
|
||||
.await;
|
||||
|
||||
assert!(
|
||||
matches!(result, Err(Error::InvalidTableName { .. })),
|
||||
"creating {name:?} did not report an invalid table name"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
/// Opening a table under an invalid name is likewise an error rather than a panic.
|
||||
#[tokio::test]
|
||||
async fn test_open_table_rejects_invalid_names() {
|
||||
let (_tempdir, db) = setup_database().await;
|
||||
|
||||
for name in INVALID_TABLE_NAMES {
|
||||
let result = db
|
||||
.open_table(OpenTableRequest {
|
||||
name: name.to_string(),
|
||||
namespace_path: vec![],
|
||||
index_cache_size: None,
|
||||
lance_read_params: None,
|
||||
location: None,
|
||||
namespace_client: None,
|
||||
managed_versioning: None,
|
||||
})
|
||||
.await;
|
||||
|
||||
assert!(
|
||||
matches!(result, Err(Error::InvalidTableName { .. })),
|
||||
"opening {name:?} did not report an invalid table name"
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
async fn setup_database() -> (tempfile::TempDir, ListingDatabase) {
|
||||
let tempdir = tempdir().unwrap();
|
||||
let uri = tempdir.path().to_str().unwrap();
|
||||
|
||||
Reference in New Issue
Block a user