Skip to content

feat(spi): add quoteIdentifierAlways to ISQLIdentifierProcessor - #2234

Merged
openai0229 merged 2 commits into
OtterMind:mainfrom
Aias00:fix/spi-quote-identifier-always
Jul 29, 2026
Merged

feat(spi): add quoteIdentifierAlways to ISQLIdentifierProcessor#2234
openai0229 merged 2 commits into
OtterMind:mainfrom
Aias00:fix/spi-quote-identifier-always

Conversation

@Aias00

@Aias00 Aias00 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Summary

Addresses the maintainer review on #2187 requesting the shared SPI contract be extended before rebasing the individual dialect quoting PRs.

Adds quoteIdentifierAlways(String) to ISQLIdentifierProcessor — an unconditional quoting method that preserves the exact identifier name regardless of case or keyword status. Embedded delimiter characters are escaped per the dialect convention. Also fixes removeIdentifierQuote to unescape doubled delimiters, satisfying the round-trip contract: removeIdentifierQuote(quoteIdentifierAlways(raw)).equals(raw).

Changes

SPI Interface (ISQLIdentifierProcessor)

  • New method quoteIdentifierAlways(String) — always quotes, preserves exact case, escapes embedded delimiters

Default Implementation (DefaultSQLIdentifierProcessor)

  • quoteIdentifierAlways: wraps in "...", escapes """
  • removeIdentifierQuote: replaced regex-based approach with strip-and-unescape (handles """)

Dialect Overrides

Processor Quote char Escape rule
DefaultSQLIdentifierProcessor " """
MysqlIdentifierProcessor backtick ```
SqlServerIdentifierProcessor [...] ]]]

PostgreSQL, KingBase, Oracle, DM, Oscar, SQLite all use " — inherit from Default without override.

Snowflake Accessor

  • Added SNOWFLAKE_SQL_IDENTIFIER_PROCESSOR static field to SnowflakeMetaData, mirroring PostgreSQL's POSTGRE_SQL_IDENTIFIER_PROCESSOR and KingBase's KINGBASE_SQL_IDENTIFIER_PROCESSOR. This gives Snowflake column-type enums access to the processor for downstream quoting PRs.

Tests (36 tests, all passing)

DefaultSQLIdentifierProcessorTest (16 tests)

  • quoteIdentifierAlways: wraps simple name, preserves mixed case, escapes embedded ", handles null, handles reserved keywords
  • quoteIdentifier (conditional): doesn't quote lowercase valid, quotes invalid
  • removeIdentifierQuote: strips and unescapes, passes through unquoted
  • Round-trip: quoteIdentifierAlwaysremoveIdentifierQuote restores raw (tested with plain, mixed-case, embedded delimiter, reserved keyword, empty)

MysqlIdentifierProcessorTest (10 tests)

  • quoteIdentifierAlways: wraps in backtick, escapes embedded backtick, preserves mixed case
  • removeIdentifierQuote: strips backtick + double-quote, unescapes
  • Round-trip with backtick

SqlServerIdentifierProcessorTest (10 tests)

  • quoteIdentifierAlways: wraps in brackets, escapes embedded ]
  • removeIdentifierQuote: strips brackets + double-quote, unescapes
  • Round-trip with brackets

Verification

# SPI tests
mvn -B -f chat2db-community-server/pom.xml -pl chat2db-community-spi -am \
  -Dmaven.test.skip=false -DskipTests=false \
  -Dtest=DefaultSQLIdentifierProcessorTest \
  -Dsurefire.failIfNoSpecifiedTests=false -Dmaven.test.failure.ignore=false test
# → Tests run: 16, Failures: 0

# MySQL tests
mvn -B -f chat2db-community-server/pom.xml -pl chat2db-community-plugins/chat2db-community-mysql -am \
  -Dmaven.test.skip=false -DskipTests=false \
  -Dtest=MysqlIdentifierProcessorTest \
  -Dsurefire.failIfNoSpecifiedTests=false -Dmaven.test.failure.ignore=false test
# → Tests run: 10, Failures: 0

# SqlServer tests
mvn -B -f chat2db-community-server/pom.xml -pl chat2db-community-plugins/chat2db-community-sqlserver -am \
  -Dmaven.test.skip=false -DskipTests=false \
  -Dtest=SqlServerIdentifierProcessorTest \
  -Dsurefire.failIfNoSpecifiedTests=false -Dmaven.test.failure.ignore=false test
# → Tests run: 10, Failures: 0

Downstream (Phase 2)

Once this SPI extension merges, the 5 quoting PRs (#2145, #2169, #2185, #2186, #2187) will be reworked to use processor.quoteIdentifierAlways(name) instead of direct " concatenation:

PR Plugin Use
#2145 KingBase KingBaseMetaData.KINGBASE_SQL_IDENTIFIER_PROCESSOR.quoteIdentifierAlways(name)
#2169 Snowflake SnowflakeMetaData.SNOWFLAKE_SQL_IDENTIFIER_PROCESSOR.quoteIdentifierAlways(name)
#2185 Snowflake SnowflakeMetaData.SNOWFLAKE_SQL_IDENTIFIER_PROCESSOR.quoteIdentifierAlways(schemaName)
#2186 Snowflake quoteIdentifierAlways(oldName) + quoteIdentifierAlways(newName) in RENAME COLUMN
#2187 PG/KingBase PostgreSQLMetaData.POSTGRE_SQL_IDENTIFIER_PROCESSOR / KingBaseMetaData.KINGBASE_SQL_IDENTIFIER_PROCESSOR

Contributor declaration

  • I tested the affected behavior and reported the actual results above.
  • I did not include credentials, private data, or generated build output.
  • I disclosed substantial AI assistance below.

AI assistance: The implementation, tests, verification, and PR description were produced with Claude Code assistance.

@Aias00
Aias00 requested a review from openai0229 as a code owner July 27, 2026 05:45
Copilot AI review requested due to automatic review settings July 27, 2026 05:45

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@openai0229 openai0229 moved this to In Review in Chat2DB Community Jul 27, 2026
Aias00 added a commit to Aias00/Chat2DB that referenced this pull request Jul 27, 2026
…chema and table in REFERENCES

Replaces direct " concatenation with the dialect-specific processor:
PostgreSQLMetaData.POSTGRE_SQL_IDENTIFIER_PROCESSOR and
KingBaseMetaData.KINGBASE_SQL_IDENTIFIER_PROCESSOR.
Built on the SPI extension (OtterMind#2234).

Closes OtterMind#2187 (superseded by this SPI-based approach)

Co-Authored-By: Claude <noreply@anthropic.com>
@Aias00
Aias00 force-pushed the fix/spi-quote-identifier-always branch from 6c6f341 to 4630a50 Compare July 27, 2026 16:09
Aias00 added a commit to Aias00/Chat2DB that referenced this pull request Jul 27, 2026
Replaces direct backtick concatenation with
KingBaseMetaData.KINGBASE_SQL_IDENTIFIER_PROCESSOR.quoteIdentifierAlways,
per the maintainer review on OtterMind#2187. Built on the SPI extension (OtterMind#2234).

Closes OtterMind#2145 (superseded by this SPI-based approach)

Co-Authored-By: Claude <noreply@anthropic.com>
Aias00 added a commit to Aias00/Chat2DB that referenced this pull request Jul 27, 2026
…ENAME

Replaces direct " concatenation with
SnowflakeMetaData.SNOWFLAKE_SQL_IDENTIFIER_PROCESSOR.quoteIdentifierAlways
for DROP COLUMN, CREATE TABLE schema qualifier, and RENAME COLUMN.
CHANGE COLUMN (MySQL syntax) is replaced with RENAME COLUMN (Snowflake).
Built on the SPI extension (OtterMind#2234).

Closes OtterMind#2169, OtterMind#2185, OtterMind#2186 (superseded by this SPI-based approach)

Co-Authored-By: Claude <noreply@anthropic.com>
Aias00 added a commit to Aias00/Chat2DB that referenced this pull request Jul 27, 2026
…chema and table in REFERENCES

Replaces direct " concatenation with the dialect-specific processor:
PostgreSQLMetaData.POSTGRE_SQL_IDENTIFIER_PROCESSOR and
KingBaseMetaData.KINGBASE_SQL_IDENTIFIER_PROCESSOR.
Built on the SPI extension (OtterMind#2234).

Closes OtterMind#2187 (superseded by this SPI-based approach)

Co-Authored-By: Claude <noreply@anthropic.com>
@Aias00

Aias00 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the CI failure: the test mock class BacktickIdentifierProcessor in SqlCompletionMetadataProviderAdapterTest was missing the new quoteIdentifierAlways override required by the updated ISQLIdentifierProcessor interface. Added the method (backtick + escape ` -> ```). All 3 dependent PRs (#2261, #2262, #2265) have been rebased onto this fix.

@openai0229 openai0229 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shared SPI direction and delimiter round-trip tests are good, but this is not ready to merge yet. ISQLIdentifierProcessor is the public plugin SPI; adding a new non-default abstract method is source-incompatible for external implementations. Please provide a backward-compatible default or capability/versioned migration path and test that compatibility. This PR also changes removeIdentifierQuote semantics used by SQL parsing/completion callers, so add regression coverage at those caller boundaries rather than only direct processor round-trip tests.

@Aias00
Aias00 force-pushed the fix/spi-quote-identifier-always branch from 4630a50 to dcb7cad Compare July 28, 2026 14:24
@Aias00

Aias00 commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Changed quoteIdentifierAlways from an abstract method to a default method for backward compatibility with external implementations. The default delegates to quoteIdentifierIgnoreCase as a best-effort fallback; dialect implementations (Default, MySQL, SqlServer) still override for correct always-quote semantics. All 16 existing tests pass, and the test mock BacktickIdentifierProcessor still compiles because the override is now optional. All 3 dependent PRs (#2261, #2262, #2265) rebased.

Aias00 and others added 2 commits July 29, 2026 11:04
Add a new SPI method that unconditionally quotes an identifier,
preserving the exact name regardless of case or keyword status.
Embedded delimiter characters are escaped per the dialect convention
(double-quote "" for ", backtick `` for `, bracket ]] for ]).

Fix removeIdentifierQuote to unescape doubled delimiters, satisfying
the round-trip contract: removeIdentifierQuote(quoteIdentifierAlways(raw)).equals(raw).

Implementations:
- DefaultSQLIdentifierProcessor: double-quote + escape " -> ""
- MysqlIdentifierProcessor: backtick + escape ` -> ``
- SqlServerIdentifierProcessor: bracket + escape ] -> ]]

Also add SNOWFLAKE_SQL_IDENTIFIER_PROCESSOR static field to
SnowflakeMetaData, mirroring PostgreSQL/KingBase, to give Snowflake
column-type enums access to the processor for downstream quoting PRs.

Tests: 36 tests covering conditional vs always-quote, embedded
delimiters, round-trip, reserved keywords, and case preservation
across Default, MySQL, and SqlServer processors.

Addresses the maintainer review on OtterMind#2187 requesting the shared SPI
contract be extended before rebasing the individual dialect quoting PRs.

Co-Authored-By: Claude <noreply@anthropic.com>
@openai0229
openai0229 force-pushed the fix/spi-quote-identifier-always branch from dcb7cad to 85fe1ff Compare July 29, 2026 03:14

@openai0229 openai0229 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maintainer follow-up completed on the original branch:

  • rebased onto current main;
  • kept the SPI source-compatible with a default method;
  • made the fallback fail fast when an external processor cannot actually quote unconditionally;
  • preserved the quote/remove round-trip for embedded delimiters;
  • removed the unrelated Snowflake constant;
  • added parser and completion caller-boundary regressions plus a legacy-implementation compatibility test.

Verification: the full test suites for chat2db-community-spi, chat2db-community-domain-core, chat2db-community-mysql, and chat2db-community-sqlserver passed with maven.test.skip=false and skipTests=false.

@openai0229
openai0229 merged commit 5c70406 into OtterMind:main Jul 29, 2026
17 checks passed
@openai0229 openai0229 moved this from In Review to Done in Chat2DB Community Jul 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

3 participants