TableDefinition::qualified_name() quotes conditionally, so a reserved-word table name breaks the inserters and to_drop_sql
Nobody has claimed this yet.
Assessment
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Newbie friendliness
- 68/100
- Issue type
- Bug
- Clarity
- Mostly clear
- Activity status
- Active
- Domain
- backend-api-design, databases
Research direction
Start in hyperdb-api-core/src/protocol/escape.rs and hyperdb-api/src/table_definition.rs, comparing QuotedIdentifier and quoted_qualified_name() with the existing qualified_name() paths. Trace the inserter entry points listed in the issue and the COPY construction in async_connection.rs and connection.rs. Done means generated DROP and COPY statements safely handle reserved-word table names without changing the display-oriented qualified_name() behavior.
Written by the indexing model from the issue text.
Description
Summary
TableDefinition::qualified_name() formats identifiers through SqlIdentifier, which omits the quotes when a name already looks like a legal bare identifier. That "looks legal" check has no reserved-word list, so an all-lowercase SQL keyword is emitted unquoted and the resulting statement is a syntax error. The inserters and to_drop_sql build their SQL from qualified_name(), so a table named order (or select, group, user, …) can be created but then cannot be inserted into or dropped through those paths.
The check documents its own gap:
// hyperdb-api-core/src/protocol/escape.rs:110-117
/// Checks if a string is a valid unquoted identifier.
///
/// Valid unquoted identifiers:
/// - Start with a letter (a-z, A-Z) or underscore
/// - Contain only letters, digits (0-9), underscores, and dollar signs
/// - Are not SQL reserved words (this function doesn't check for reserved words)
#[must_use]
pub fn is_valid_unquoted_identifier(s: &str) -> bool {
and SqlIdentifier quotes only when that check fails or the name carries uppercase:
// hyperdb-api-core/src/protocol/escape.rs:58-59
let needs_quoting =
!is_valid_unquoted_identifier(self.0) || self.0.chars().any(char::is_uppercase);
so SqlIdentifier("order") renders as bare order, and that is what qualified_name() returns:
// hyperdb-api/src/table_definition.rs:816-829
pub fn qualified_name(&self) -> String {
match (&self.database, &self.schema) {
// ...
(None, None) => format!("{}", SqlIdentifier(&self.name)),
}
}
Verified against a running engine
Executed against hyperd (the pinned release), not inferred:
CREATE TABLE "order" ("id" INTEGER, "select" TEXT)— accepted. Keyword-named tables are legal, so they do occur in practice.DROP TABLE IF EXISTS order— the exact shapeto_drop_sqlemits — rejected:ERROR: syntax error: got ORDER, expected <identifier> (42601).COPY order ("id", "select") FROM STDIN WITH (FORMAT HYPERBINARY)— the exact shape the inserters build — rejected:ERROR: syntax error: got ORDER, expected one of: <identifier>, '(' (42601).- The same
COPYwith"order"quoted parses and proceeds to await the COPY stream, isolating the quoting as the cause rather than anything else about the statement.
Call sites still routing through qualified_name() on main
The COPY statement is assembled here, interpolating the table name verbatim:
// hyperdb-api-core/src/client/async_connection.rs:691
// (sync twin: hyperdb-api-core/src/client/connection.rs:878)
let query = format!("COPY {table_name}{column_list} FROM STDIN WITH (FORMAT {format})");
and table_name comes from qualified_name() at:
hyperdb-api/src/inserter.rs:481and:713— syncInserterhyperdb-api/src/async_inserter.rs:252—AsyncInserterhyperdb-api/src/arrow_inserter.rs:189—ArrowInserterhyperdb-api/src/async_arrow_inserter.rs:115and:460—AsyncArrowInserter/AsyncArrowInserterOwnedhyperdb-api-node/src/inserter.rs:245— Node bindingshyperdb-api/src/table_definition.rs:987—to_drop_sql
Worth noting the columns in a COPY are already quoted unconditionally (hyperdb-api-core/src/client/async_connection.rs:685), so a keyword column name is fine. Only the table name is exposed.
#258 fixed this class of bug, deliberately without touching qualified_name()
#258 introduced QuotedIdentifier, which quotes unconditionally, for exactly this failure — its doc names select and order:
// hyperdb-api-core/src/protocol/escape.rs:187-195
/// A SQL identifier that is **always** quoted, whatever it contains.
///
/// [`SqlIdentifier`] omits the quotes when a name is already a legal bare
/// identifier, which is fine for display but unsafe for generated DDL:
/// [`is_valid_unquoted_identifier`] deliberately does not know the reserved
/// word list, so an all-lowercase keyword such as `select` or `order` passes
/// the check and is emitted bare, producing a syntax error.
and a private quoted_qualified_name() (hyperdb-api/src/table_definition.rs:792) used by the DDL it generates:
// hyperdb-api/src/table_definition.rs:929 (to_create_sql)
sql.push_str(&self.quoted_qualified_name());
So to_create_sql is already correct on main. to_drop_sql at :987 and every inserter site above still use qualified_name(). #258 left the public qualified_name() alone on purpose — changing it churns public doctests and the examples that print it — so this is the deliberately-deferred remainder, not a regression from that PR.
Impact
Latent, but a hard failure once hit: CREATE TABLE succeeds and then every insert into that table fails with a syntax error naming a SQL keyword rather than the user's table, which reads as a library bug. Most affected users won't have chosen the name — it arrives from a CSV header, a reflected schema, or an upstream system.
Fix direction
Follow #258's shape and route the SQL-generating paths through QuotedIdentifier, rather than loosening SqlIdentifier (which is doing legitimate work for display). to_drop_sql can switch to quoted_qualified_name() outright — it generates SQL and nothing inspects its output format.
The inserters need more care. They take &str table names internally, so they could receive the quoted form at construction, but two things want an audit first: callers that read qualified_name() for display, and any caller passing an already-quoted name in (which would then be double-quoted). Keeping qualified_name() as the display-oriented accessor it has effectively become, and adding a documented SQL-safe counterpart that the SQL paths use, keeps the change additive.
- Dominant language
- Rust
- Stars
- 2
- Forks
- 2
- Avg merge
- 12h 2m
- Merged PRs (30d)
- 60
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
More from tableau/hyper-api-rust
-
Difficulty 2/5 1-3 hours Newbie friendliness 68/100
tableau/hyper-api-rust#294 ·
-
Difficulty 4/5 3-5 days Newbie friendliness 35/100
tableau/hyper-api-rust#311 ·
-
Difficulty 4/5 3-5 days Newbie friendliness 45/100
tableau/hyper-api-rust#305 ·
-
Windows Named Pipe: verify DACL denies other users, and measure read-path perf for MCP workloads Open
Difficulty 4/5 3-5 days Newbie friendliness 38/100
tableau/hyper-api-rust#302 ·
-
Difficulty 3/5 1-2 days Newbie friendliness 72/100
tableau/hyper-api-rust#300 ·
All issues in tableau/hyper-api-rust
Similar issues
-
risk:low runtime status:in-progress type:test
Difficulty 1/5 Under an hour Newbie friendliness 92/100
zeroclaw-labs/zeroclaw#11023 ·
-
good first issue refactor
Difficulty 2/5 1-3 hours Newbie friendliness 72/100
-
Difficulty 2/5 1-3 hours Newbie friendliness 86/100
kwakseongjae/auto-hwp#319 ·
-
area:cli bug filter-quality good first issue priority:medium
Difficulty 2/5 1-3 hours Newbie friendliness 84/100
-
Difficulty 1/5 Under an hour Newbie friendliness 72/100
bevyengine/bevy#25861 ·