Skip to content

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

Description

@StefanSteiner

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.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions