Skip to content

feat: add Oracle connector with Testcontainers integration test - #433

Merged
tianzhou merged 6 commits into
mainfrom
claude/bold-hawking-2n28vd
Sep 19, 2026
Merged

tianzhou merged 6 commits into
mainfrom
claude/bold-hawking-2n28vd

Conversation

@tianzhou

@tianzhou tianzhou commented Sep 19, 2026

Copy link
Copy Markdown
Member

Why now

Oracle was held back because there was no workable way to integration-test it. That has changed:

  • Testcontainers ships an official Oracle module, @testcontainers/oraclefree, wrapping the freely redistributable gvenzl/oracle-free 23ai image (no license acceptance, amd64 and arm64).
  • node-oracledb runs in Thin mode: pure JavaScript, no Oracle Instant Client. It keeps the zero-native-dependency install and works in the Docker image and MCPB bundle.

What's in the connector (src/connectors/oracle/index.ts)

  • DSN oracle://user:pass@host:1521/service (path is the service name), ?sid= for SID addressing, sslmode=require|verify-full for TCPS.
  • Metadata via ALL_TABLES / ALL_TAB_COLUMNS / ALL_INDEXES / ALL_CONSTRAINTS / ALL_OBJECTS / ALL_ARGUMENTS / ALL_SOURCE, plus table and column comments. Column types are rendered as DDL spells them (VARCHAR2(100), NUMBER(10,2), NUMBER(*,0)).
  • Identifier case: unquoted names fold to upper case once in TypeScript (foldIdentifier), so callers can pass the names they wrote in DDL and every catalog predicate stays an indexed equality. Mixed-case names are taken as spelled.
  • Batches are split statement by statement, one round trip each. A PL/SQL block anywhere in the batch (or the DDL creating one, compound triggers included) is kept whole by tracking BEGIN/CASEEND depth on comment-blanked text; package specs/bodies open at IS/AS; SQL*Plus / lines are boundaries.
  • Binds: :N placeholders are bound by name per statement, so a placeholder can repeat or appear out of order and each statement in a batch gets only the binds it uses.
  • Values: NUMBER is fetched exactly and converted to number, BigInt (above 2^53) or float; CLOB/NCLOB as strings; BLOB as Buffer.
  • Read-only backstop via SET TRANSACTION READ ONLY, always rolled back. DDL is not covered by it (DDL implicitly commits), which is why the keyword classifier stays first line of defense.
  • max_rows via an outer FETCH FIRST n ROWS ONLY inline view, with the probe-row truncation flag. A FOR UPDATE statement is left uncapped since Oracle allows neither form with it.
  • EXPLAIN [PLAN FOR] served through EXPLAIN PLAN + DBMS_XPLAN.DISPLAY, never executing the statement; PLAN_TABLE rows are cleaned up.
  • Pooling with connect and query timeouts from ConnectorConfig. Driver error properties (code, errorNum) survive wrapping so connection failures classify.

Shared plumbing

  • "oracle" added to ConnectorType and every per-dialect table (keywords, parser scanner, parameter style, error classifier, identifier quoting, default port 1521, env/TOML validation, OpenAPI enum, frontend DatabaseType + logo). TOML accepts sslmode = "verify-full" for Oracle; sslrootcert is rejected for every non-PostgreSQL source rather than silently ignored.
  • SQL parser understands Oracle q'[...]' alternative quoting so a literal containing a quote cannot leak into the read-only classifier or bind counting.
  • Custom tools use :1-style bind placeholders on Oracle; the parameter mapper scans with the connector's dialect and rejects a zero index in every style.
  • Read-only classifier rejects UTL_* / DBMS_* package member calls reachable from a SELECT (network, filesystem, code execution). Matched as package.member(, so a column or qualified reference merely named after a package is fine.
  • NJS-503 / ORA-01017 / ORA-28000 classified as unreachable / auth failure.
  • Driver externalized in tsup and picked up by the MCPB bundle via optionalDependencies.
  • LEADING_SQL_NOISE lifted from the SQL Server connector into sql-parser and shared.
  • Postgres getTableIndexes now returns column_names as a real array. array_agg over attname yields name[], which node-pg does not parse, so it was silently a {id} string. Surfaced by the test-base change below.

Tests

  • src/connectors/__tests__/oracle.integration.test.ts runs the shared suite plus Oracle-specific cases: identifier case, DDL type rendering, views, procedure/function detail, named/repeated binds, multi-statement batches including PL/SQL in the middle, max_rows truncation, big NUMBER values, BLOBs, q-quotes, driver error codes, read-only rejection (ORA-01456), and all three EXPLAIN paths.
  • Shared test base now looks up columns and row keys case-insensitively (Oracle reports USERS, ID, NAME); the engine-specific suites assert exact casing.
  • Unit tests cover the Oracle DSN parser, the statement splitter, bind mapping, NUMBER conversion, and the new parser, classifier, row limiter, parameter style, and error classification paths.

Verification

  • CI green on each pushed head: Unit Tests and Integration Tests. The Oracle integration suite runs live in CI and passed on its first run; the container starts in about 80 seconds on the runner.
  • Two Copilot reviews (5 inline findings, 4 inline findings, 7 summary-only findings) all addressed.

Not included

  • health_check for Oracle. The tool already reports unsupported connectors explicitly.

Docs

README, CLAUDE.md, .env.example, dbhub.toml.example, the testing skill, and the plugin/MCPB manifests list Oracle.

🤖 Generated with Claude Code

https://claude.ai/code/session_0189BHv585xi8iqEp9JvmgKY

Oracle was held back for lack of a workable integration-test path. That
is no longer the case: Testcontainers ships an official Oracle module
(@testcontainers/oraclefree, wrapping the freely redistributable
gvenzl/oracle-free 23ai image) and node-oracledb runs in Thin mode, a
pure-JavaScript driver that needs no Oracle Instant Client.

Connector (src/connectors/oracle):
- DSN oracle://user:pass@host:1521/service, with ?sid= for SID
  addressing and sslmode=require|verify-full for TCPS
- Metadata via ALL_TABLES / ALL_TAB_COLUMNS / ALL_INDEXES /
  ALL_CONSTRAINTS / ALL_OBJECTS / ALL_ARGUMENTS / ALL_SOURCE plus
  table and column comments; names matched as given or upper-cased so
  lower-case DDL spellings resolve, returned as the catalog holds them
- Batches split on top-level semicolons, one round trip per statement;
  PL/SQL blocks sent whole; trailing ; and / terminators handled
- Read-only backstop via SET TRANSACTION READ ONLY, always rolled back
- max_rows via an outer FETCH FIRST n ROWS ONLY inline view, with the
  probe-row truncation flag
- EXPLAIN [PLAN FOR] served through EXPLAIN PLAN + DBMS_XPLAN.DISPLAY,
  never executing the statement; PLAN_TABLE rows cleaned up
- Connection pooling with connect/query timeouts from ConnectorConfig

Shared plumbing: "oracle" ConnectorType; dialect scanner that
understands q'[...]' alternative quoting; :1-style bind placeholders
for custom tools; read-only classifier rejects UTL_*/DBMS_* package
calls reachable from a SELECT; NJS-503 / ORA-01017 / ORA-28000 error
classification; default port 1521 in env, TOML and DSN helpers; driver
externalized in tsup and picked up by the MCPB bundle.

Tests: oracle.integration.test.ts runs the shared suite plus
Oracle-specific cases (identifier case, PL/SQL, binds, batches,
read-only rejection, truncation, EXPLAIN). The shared test base gains
an identifierCase hook because Oracle folds unquoted identifiers to
upper case. Unit tests cover the new parser, classifier, row limiter
and parameter-style paths. Docs and manifests list Oracle.

Not yet implemented: health_check for Oracle (the tool reports it as
unsupported for this connector).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189BHv585xi8iqEp9JvmgKY
- Fold identifiers once in TypeScript (OracleConnector.foldIdentifier)
  instead of `x = :b OR x = UPPER(:b)` in every catalog query, so each
  predicate is a plain indexed equality and the rule lives in one place
- Drop the pool guards shadowed by acquire(); flatten executeSQL's
  nested try/catch; destructure the row-limiter rewrite; reuse query()
  and disconnect() in connect(); DSN parser returns pool attributes
- getStoredProcedureDetail runs its three lookups on one connection,
  the two dependent ones concurrently
- Share the leading-noise regex (LEADING_SQL_NOISE) between the SQL
  Server and Oracle connectors via sql-parser
- Keep escape-hatch keywords in one table with a per-dialect call
  suffix rather than an Oracle-only override
- Scope NJS-503 to Oracle via NETWORK_CODES_BY_TYPE instead of the
  socket-code set
- One helper for sequential $N / @pn / :N placeholder counting
- Test base: case-insensitive column/row lookups replace the
  identifierCase config knob

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189BHv585xi8iqEp9JvmgKY
Copilot AI lite review requested due to automatic review settings September 19, 2026 15:30
array_agg over pg_attribute.attname produces name[], which node-pg has
no type parser for, so getTableIndexes handed back the raw '{id}'
string despite the string[] type. The shared integration test only
noticed once its lookup became case-insensitive (Array.prototype.some
on a string). Cast to text[] so the driver parses it.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189BHv585xi8iqEp9JvmgKY

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

The review found correctness issues in mixed PL/SQL batches, Oracle custom-tool parameter parsing, and TOML verify-full configuration.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 High severity · 3 Medium severity · 1 Low severity

Open (5)
What changed in this PR

Adds Oracle support using node-oracledb Thin mode, including metadata, execution, read-only enforcement, row limits, EXPLAIN, configuration, and Testcontainers integration.

Changes:

  • Added and registered the Oracle connector with DSN, pooling, TLS, metadata, and PL/SQL support.
  • Extended shared SQL parsing, parameter binding, classification, configuration, and API types.
  • Added Oracle integration tests, unit coverage, dependency updates, and documentation.
File Description
tsup.config.ts Externalizes oracledb.
src/​utils/​sql-row-limiter.ts Adds Oracle row limiting.
src/​utils/​sql-parser.ts Adds Oracle quoting and shared SQL noise handling.
src/​utils/​parameter-mapper.ts Adds colon-numbered binds.
src/​utils/​identifier-quoter.ts Adds Oracle identifier quoting.
src/​utils/​error-classifier.ts Classifies Oracle connection errors.
src/​utils/​dsn-obfuscate.ts Adds Oracle DSN and port handling.
src/​utils/​allowed-keywords.ts Adds Oracle read-only rules.
src/​utils/​__tests__/​* Tests Oracle utility behavior.
src/​types/​config.ts Adds Oracle configuration type.
src/​index.ts Loads the Oracle connector.
src/​connectors/​sqlserver/​index.ts Reuses shared leading-noise handling.
src/​connectors/​oracle/​index.ts Implements the Oracle connector.
src/​connectors/​interface.ts Adds Oracle as a connector type.
src/​connectors/​__tests__/​shared/​integration-test-base.ts Supports case-insensitive identifiers.
src/​connectors/​__tests__/​oracle.integration.test.ts Adds Oracle Testcontainers coverage.
src/​config/​toml-loader.ts Accepts Oracle TOML sources.
src/​config/​env.ts Adds Oracle environment configuration.
src/​config/​__tests__/​* Tests Oracle configuration.
src/​api/​openapi.yaml Adds Oracle to the API enum.
src/​api/​openapi.d.ts Updates generated API types.
README.md Documents Oracle support.
pnpm-lock.yaml Locks Oracle dependencies.
plugin/​skills/​setup/​SKILL.md Adds Oracle DSN guidance.
plugin/​skills/​explore/​SKILL.md Documents Oracle row limiting.
plugin/​README.md Lists Oracle support.
plugin/​.claude-plugin/​plugin.json Updates plugin metadata.
package.json Adds Oracle driver and test dependencies.
mcpb/​manifest.json Adds Oracle bundle metadata.
dbhub.toml.example Adds Oracle configuration examples.
CLAUDE.md Documents Oracle architecture and DSNs.
.env.example Adds Oracle environment examples.
.claude/​skills/​testing/​SKILL.md Documents Oracle integration testing.
Files not reviewed (1)
  • pnpm-lock.yaml: Generated file

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/connectors/oracle/index.ts Outdated
Comment thread dbhub.toml.example
Comment thread src/utils/allowed-keywords.ts
Comment thread src/utils/parameter-mapper.ts
Comment thread src/connectors/oracle/index.ts
- Split Oracle batches statement by statement: a PL/SQL block (or the
  DDL creating one) anywhere in the batch is kept whole by tracking
  BEGIN/CASE ... END depth on the comment/string-blanked text; package
  specs/bodies and type bodies open at IS/AS. SQL*Plus `/` lines are
  boundaries. Previously only a batch that *started* with PL/SQL was
  kept whole, so INSERT; BEGIN ... END; SELECT was split mid-block.
- Allow sslmode = verify-full for Oracle sources in TOML validation
  (verify-ca stays PostgreSQL-only), so the documented TCPS DSN loads.
- Match Oracle escape-hatch packages only on a member call
  (`pkg.member(`), not on any qualified name such as `dbms_sql.foo`.
- Thread the connector type through the parameter mapper so an Oracle
  q'[...]' literal containing `:1` no longer miscounts binds.
- Unit tests for the Oracle DSN parser (service/SID/TCPS/timeouts/pool)
  and the splitter; integration case for a mixed batch with PL/SQL.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189BHv585xi8iqEp9JvmgKY

Copilot AI 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.

Copilot review overview

🟡 Changes recommended

Oracle bind handling, numeric precision, statement splitting, and related integration paths still contain correctness issues.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 2 High severity · 2 Medium severity

Open (4)
Resolved since last review (5)
Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Previously missed (7)

In code that hasn't changed since last review

Medium severity Add Oracle to frontend database types and logos

src/​api/​openapi.yaml:105

The API can now return type: "oracle", but the workbench still excludes it from DatabaseType and DB_LOGOS (frontend/src/types/datasource.ts:1, frontend/src/lib/db-logos.ts:8-14). Oracle sources therefore render with an undefined image URL and the frontend contract no longer matches the API. Add Oracle to the frontend type and provide a logo or explicit fallback.

Medium severity Fetch Oracle BLOBs as buffers

src/​connectors/​oracle/​index.ts:254

BLOB columns fall through to node-oracledb's default Lob stream representation. These rows are serialized directly into MCP JSON responses, so callers receive stream internals rather than the binary value. Configure BLOBs to be fetched as buffers alongside the existing CLOB/NCLOB conversion.

Medium severity Preserve scale for NUMBER(*, scale) metadata

src/​connectors/​oracle/​index.ts:405

For an Oracle column declared as NUMBER(*, scale), DATA_PRECISION is null while DATA_SCALE is populated. This branch returns bare NUMBER and loses the scale constraint, so schema metadata is inaccurate for types such as NUMBER(*,2).

Medium severity Use named binds for repeated Oracle placeholders

src/​connectors/​oracle/​index.ts:599

Passing binds as an array makes node-oracledb bind SQL placeholders by occurrence, but the parameter mapper treats :N as an indexed, reusable name. As a result, SELECT :1, :1 validates with one parameter but execution requires two positional values; reordered placeholders receive swapped values, and split batches receive unrelated extra binds. Build a per-statement bind object keyed by the numeric names ({"1": parameters[0]}), including in explainQuery, or explicitly reject these unsupported forms.

Medium severity Preserve Oracle driver error properties

src/​connectors/​oracle/​index.ts:623

Wrapping the driver exception in a new Error discards node-oracledb fields such as code, errno, and errorNum. A connection loss during execution, including NJS-503, can no longer be recognized by tryClassifyConnectionError and is returned as a generic execution failure. Preserve the original error properties when adding context; apply the same correction to the EXPLAIN wrapper at line 756.

Medium severity Reject zero-based Oracle parameter indices

src/​utils/​parameter-mapper.ts:135

The new indexed-parameter helper accepts :0: maxIndex becomes zero, the sequential-validation loop never runs, and registration reports that no values are required. A custom Oracle tool using :0 can therefore pass validation and fail only during execution. Reject index zero before calculating the maximum; this also closes the equivalent $0 and @p0 gap.

Medium severity Preserve or reject max_rows with Oracle FOR UPDATE

src/​utils/​sql-row-limiter.ts:342

The wrapper also rewrites valid Oracle locking reads such as SELECT ... FOR UPDATE. Oracle does not permit FOR UPDATE in this inline-view shape, and the outer row-limiting clause cannot preserve the lock semantics, so enabling max_rows turns the query into an Oracle error. Detect this form and either reject the incompatible combination explicitly or use a lock-preserving cap.

Comment thread src/connectors/oracle/index.ts Outdated
Comment thread src/connectors/oracle/index.ts
Comment thread src/config/toml-loader.ts
Comment thread src/connectors/oracle/index.ts
- Fetch NUMBER as its decimal string and convert: safe integers to
  number, larger integers to BigInt (serialized as strings by the
  response formatter), fractions/exponents to number. The driver's
  default rounded integers above 2^53.
- Splitter: `COMPOUND TRIGGER` opens depth so section terminators
  (`END BEFORE STATEMENT;` ...) no longer end the DDL early; plain SQL
  ends at a `;` or a `/` line; empty slices from `;;` are skipped.
- Reject sslrootcert for non-PostgreSQL sources instead of silently
  ignoring a trust anchor on Oracle verify-full.
- Tests for each, plus an integration case for big NUMBER values.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189BHv585xi8iqEp9JvmgKY
Items the second review listed without inline threads:
- Bind :N placeholders by name per statement (bindsFor) instead of a
  positional array, so a placeholder can repeat or appear out of order
  and each statement of a batch receives only the binds it uses
- Fetch BLOB as Buffer instead of a Lob stream
- Render NUMBER(*, s) (INTEGER is NUMBER(*,0)) instead of bare NUMBER
- Keep driver error properties (code, errorNum, offset, cause) when
  wrapping execution/explain errors so connection errors still classify
- Reject a zero index ($0 / @p0 / :0) in custom-tool placeholders
- Leave an Oracle FOR UPDATE statement uncapped: neither an inline view
  nor a row-limiting clause is allowed with it
- Frontend: add "oracle" to DatabaseType with a neutral logo mark

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0189BHv585xi8iqEp9JvmgKY

Copilot AI 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.

Copilot review overview

🔵 Needs a closer look

Several Oracle execution, statement-splitting, row-limiting, and metadata edge cases remain unresolved.

Review effort: Balanced
Findings: None

Resolved since last review (4)
Files not reviewed (1)
  • pnpm-lock.yaml: Generated file
Previously missed (8)

In code that hasn't changed since last review

Medium severity Recognize labeled PL/SQL blocks in statement splitting

src/​connectors/​oracle/​index.ts:156

A valid labeled anonymous block begins with <<label>>, so the anchored PL/SQL check treats it as plain SQL and splits at its first internal semicolon. For example, <<outer>> BEGIN NULL; END outer; cannot be executed. Recognize an optional leading PL/SQL label before BEGIN or DECLARE, including quoted label identifiers, and add a splitter regression test.

Medium severity Fetch Oracle BLOBs as buffers for self-contained JSON

src/​connectors/​oracle/​index.ts:263

BLOB columns still use node-oracledb's default Lob stream representation. executeSQL closes the pooled connection before the MCP response is serialized, so the stream is no longer usable and the result is not self-contained JSON. Fetch BLOBs as buffers, as CLOBs are already fetched as strings.

Medium severity Preserve exact case for quoted Oracle identifiers

src/​connectors/​oracle/​index.ts:291

foldIdentifier uppercases every all-lowercase name. This breaks metadata round-tripping for valid quoted objects such as "users": getTables() returns users, but getTableSchema, indexes, comments, and row-count lookups query USERS, so search_objects loses their details. Preserve an exact catalog match before applying the unquoted uppercase fallback across these lookup methods.

Medium severity Preserve Oracle NUMBER scale without precision

src/​connectors/​oracle/​index.ts:431

Oracle reports NUMBER(*, scale) with DATA_PRECISION = NULL and a non-null DATA_SCALE. Returning bare NUMBER loses the enforced scale from schema metadata. Preserve the scale when precision is absent.

Medium severity Handle EXPLAIN statements within mixed batches

src/​connectors/​oracle/​index.ts:605

EXPLAIN is routed through explainQuery only when it starts the entire input. In a mixed batch such as SELECT 1 FROM dual; EXPLAIN PLAN FOR SELECT ..., the later statement bypasses DBMS_XPLAN formatting and PLAN_TABLE cleanup; a later bare EXPLAIN fails outright. Handle EXPLAIN per statement or explicitly reject mixed EXPLAIN batches.

Medium severity Reject parameterized multi-statement Oracle queries

src/​connectors/​oracle/​index.ts:606

The same bind array is passed to every statement in a batch. A custom tool such as SELECT :1 FROM dual; SELECT 1 FROM dual passes validation, but the second execution receives an extra bind and fails. Match the PostgreSQL and SQLite behavior by rejecting parameterized multi-statement queries before acquiring a connection.

Medium severity Handle package body initialization sections correctly

src/​connectors/​oracle/​index.ts:706

A package body with an initialization section is combined with the following statement. isUnit starts at synthetic depth 1, the initialization BEGIN increments it, and the final package END only returns it to 1, so no terminating semicolon is recognized. Track package-body initialization separately, or close the synthetic unit depth when its final END is reached, and add a regression test with a following statement.

Medium severity Handle FOR UPDATE with max_rows safely

src/​utils/​sql-row-limiter.ts:342

This wrapper makes valid SELECT ... FOR UPDATE statements invalid whenever max_rows is configured: Oracle does not allow FOR UPDATE inside the generated inline view, and its row-limiting clause is incompatible with FOR UPDATE. Detect a top-level locking clause and either use a compatible policy or return an explicit unsupported-combination error instead of sending invalid SQL.

@tianzhou
tianzhou merged commit 910a41f into main Sep 19, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants