TableDefinition::qualified_name() quotes conditionally, so a reserved-word table name breaks the inserters and to_drop_sql

Open
#265 0 comments 0 reactions 0 assignees View on GitHub

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
Tech stack
rust, sql

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 shape to_drop_sql emits — 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 COPY with "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:481 and :713 — sync Inserter
  • hyperdb-api/src/async_inserter.rs:252AsyncInserter
  • hyperdb-api/src/arrow_inserter.rs:189ArrowInserter
  • hyperdb-api/src/async_arrow_inserter.rs:115 and :460AsyncArrowInserter / AsyncArrowInserterOwned
  • hyperdb-api-node/src/inserter.rs:245 — Node bindings
  • hyperdb-api/src/table_definition.rs:987to_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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from tableau/hyper-api-rust

All issues in tableau/hyper-api-rust

Similar issues

More Rust issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.