Skip to content

Move Connection#describe to SchemaStatements helpers#2545

Merged
yahonda merged 1 commit into
rsim:masterfrom
yahonda:move-describe-to-schema-statements
Apr 20, 2026
Merged

Move Connection#describe to SchemaStatements helpers#2545
yahonda merged 1 commit into
rsim:masterfrom
yahonda:move-describe-to-schema-statements

Conversation

@yahonda
Copy link
Copy Markdown
Collaborator

@yahonda yahonda commented Apr 18, 2026

Summary

Alternative to #2498.

Instead of splitting OracleEnhanced::Connection into a dispatcher plus a DatabaseDescription module, this PR aligns with how Rails' bundled adapters (mysql2, trilogy, postgresql) handle schema introspection: the logic lives as private helpers on SchemaStatements, not on the connection wrapper.

  • Introduce two private helpers on OracleEnhanced::SchemaStatements:
    • extract_schema_qualified_name(string) — pure string parsing. Mirrors the helpers of the same name in Rails' PostgreSQL and MySQL adapters. Returns [schema, identifier]; a non-qualified name returns [nil, identifier]. Oracle-specific bits (rejecting db links, upcasing valid identifiers so catalog lookups match the stored upper-case names) live here and nowhere else.
    • resolve_data_source_name(name) — calls the extract helper, defaults the schema to _connection.owner when unqualified, runs the catalog query, and iteratively follows synonyms. A Set of visited [owner, identifier] pairs guards against looping synonym chains. Raises OracleEnhanced::ConnectionException when the object does not exist, or with "looping chain of synonyms" (Oracle's own ORA-01775 phrasing) if a cycle is detected. The inline doc comment explicitly contrasts _connection.owner (the adapter's configured default schema, from config[:schema] or config[:username]) against SYS_CONTEXT('USERENV', 'CURRENT_SCHEMA'), which can diverge after ALTER SESSION SET CURRENT_SCHEMA.
  • The catalog query runs through the adapter's select_one path, so it participates in logging, instrumentation, and the query cache. The previous describe bypassed all of that by driving a raw cursor through _select_one / prepare / fetch.
  • Replace all 10 _connection.describe(name) call sites in schema_statements.rb and oracle_enhanced_adapter.rb with resolve_data_source_name(name).
  • Remove describe, _select_one, and _select_value from OracleEnhanced::Connection. Expose attr_reader :owner so the helper can read the default schema.
  • Drop the def describe(name); super; end shims from OCIConnection and JDBCConnection.
  • Incidentally fix a latent bug: the original if name.include?("@") branch on master has had raise ArgumentError "db link is not supported" (missing comma) since 2018, so it silently never raised. The new helper uses the correct raise ArgumentError, "..." form, and a regression spec locks it in.

Why this shape

describe was effectively internal plumbing that every caller immediately destructured into [owner, table_name]. It did not need to be a public method on the connection wrapper, and putting it there made it awkward to route through the adapter's normal query path. Moving it to SchemaStatements matches the convention used by extract_schema_qualified_name / quoted_scope in other Rails adapters.

Splitting extract_schema_qualified_name out of resolve_data_source_name keeps the parse step pure: no database access, no session state. Whichever follow-up path we take on synonym resolution — all_objects (#2521) or DBMS_UTILITY.NAME_RESOLVE (#2531) — the catalog-lookup body is the only thing that needs to change.

Related

#2546 is a separate, independent cleanup that de-duplicates _oracle_downcase against Quoting#oracle_downcase. It can be merged before or after this PR.

Test plan

  • bundle exec rspec spec/active_record/connection_adapters/oracle_enhanced/connection_spec.rb (MRI / OCI)
  • Full suite: bundle exec rspec
  • resolve_data_source_name specs cover: existing table, non-existing table (raises ConnectionException), cross-schema table, cross-schema with mixed case, view, cross-schema view, materialized view, private synonym, public synonym, two-hop looping synonym chain (A↔B), three-hop looping synonym chain (A→B→C→A), and db-link rejection
  • New extract_schema_qualified_name specs cover unqualified / already upcase / lowercase qualified / mixed-case qualified / Symbol input / db-link rejection / non-valid-identifier passthrough
  • bundle exec rubocop
  • Spec-locked: resolve_data_source_name emits a SCHEMA sql.active_record event via AR's select_one, proving the refactor exercises the adapter's query machinery (the previous raw-cursor path did not)

🤖 Generated with Claude Code

@yahonda yahonda force-pushed the move-describe-to-schema-statements branch from c13cc7b to b17e082 Compare April 18, 2026 14:30
@yahonda yahonda force-pushed the move-describe-to-schema-statements branch from 7dfd27e to 4dd9c01 Compare April 19, 2026 13:18
@yahonda yahonda changed the title Move describe to schema_statements as resolve_data_source_name Move Connection#describe to SchemaStatements helpers Apr 19, 2026
@yahonda yahonda force-pushed the move-describe-to-schema-statements branch from 73363fd to 855f3ad Compare April 19, 2026 13:25
@yahonda yahonda marked this pull request as ready for review April 19, 2026 13:35
@yahonda yahonda requested a review from Copilot April 19, 2026 13:53
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR moves Oracle object “describe”/resolution logic off the low-level OracleEnhanced::Connection wrapper and into private SchemaStatements helpers, aligning Oracle Enhanced’s schema introspection structure with Rails’ bundled adapters and routing the catalog lookup through the adapter’s normal select_one path (logging/instrumentation/query cache).

Changes:

  • Add private resolve_data_source_name and extract_schema_qualified_name helpers to OracleEnhanced::SchemaStatements and use them in place of _connection.describe(...).
  • Remove describe (and related raw-cursor helpers) from OracleEnhanced::Connection, and drop the describe forwarding shims from OCIConnection and JDBCConnection.
  • Update connection specs to cover the new helper behavior, including db-link rejection regression coverage.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
spec/active_record/connection_adapters/oracle_enhanced/connection_spec.rb Replaces describe expectations with coverage for resolve_data_source_name / extract_schema_qualified_name, including db-link rejection tests.
lib/active_record/connection_adapters/oracle_enhanced_adapter.rb Switches adapter call sites from _connection.describe to resolve_data_source_name.
lib/active_record/connection_adapters/oracle_enhanced/schema_statements.rb Introduces the new private helpers and updates SchemaStatements call sites to use them.
lib/active_record/connection_adapters/oracle_enhanced/oci_connection.rb Removes the describe shim that previously exposed the base private method.
lib/active_record/connection_adapters/oracle_enhanced/jdbc_connection.rb Removes the describe shim that previously exposed the base private method.
lib/active_record/connection_adapters/oracle_enhanced/connection.rb Removes describe and raw cursor select helpers; exposes owner via attr_reader for default schema resolution.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@yahonda yahonda force-pushed the move-describe-to-schema-statements branch 3 times, most recently from b5420c7 to 479f4a5 Compare April 20, 2026 00:54
yahonda added a commit to yahonda/oracle-enhanced that referenced this pull request Apr 20, 2026
…data_source_name

Two follow-up specs surfaced in review of rsim#2545:

- Assert that `resolve_data_source_name` emits a `SCHEMA`
  `sql.active_record` event. The previous `Connection#describe` path drove
  a raw cursor and emitted no event; this locks in the switch to the
  adapter's `select_one` path so a future refactor cannot silently
  regress it.
- Extend synonym-cycle coverage beyond the A<->B case to a three-hop
  A->B->C->A chain, proving the `[owner, identifier]` visited Set
  catches cycles of arbitrary length, not only pairs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@yahonda yahonda force-pushed the move-describe-to-schema-statements branch from 78ce74c to b8b0e6a Compare April 20, 2026 02:49
Replace the private `describe(name)` method on `OracleEnhanced::Connection`
with a private `resolve_data_source_name(name)` helper on the adapter's
`SchemaStatements` module. Rails' bundled adapters (mysql2, trilogy,
postgresql) place introspection on the adapter itself rather than on a
connection wrapper -- this aligns oracle-enhanced with that pattern.

The new helper runs its catalog query through the adapter's `select_one`
path, so it participates in logging, instrumentation, and the query cache.
The previous implementation bypassed all of that via a raw cursor in
`_select_one`. A regression spec subscribes to `sql.active_record` and
asserts that a `SCHEMA` event fires, locking the instrumentation path in.

Synonym resolution runs as an iterative loop that tracks visited
`[owner, identifier]` pairs in a Set, so a circular synonym -- whether
two-hop (`A <-> B`) or multi-hop (`A -> B -> C -> A`) -- raises
OracleEnhanced::ConnectionException with "looping chain of synonyms"
(Oracle's own phrasing for ORA-01775) instead of recursing until the
Ruby stack overflows. Regression specs cover both shapes.

`def describe(name); super; end` shims on OCIConnection/JDBCConnection
and the `_select_one` / `_select_value` helpers that existed only to
support `describe` are removed. `_oracle_downcase` stays on the base
Connection class since OCI/JDBC still use it for column metadata.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@yahonda yahonda force-pushed the move-describe-to-schema-statements branch from b8b0e6a to f37ca48 Compare April 20, 2026 03:00
@yahonda yahonda merged commit 0584d32 into rsim:master Apr 20, 2026
12 checks passed
yahonda added a commit that referenced this pull request Apr 24, 2026
Move Connection#describe to SchemaStatements helpers
yahonda added a commit that referenced this pull request Apr 24, 2026
Backport #2545 and #2521 to release81: move Connection#describe helpers and optimize catalog lookup
@yahonda yahonda deleted the move-describe-to-schema-statements branch April 29, 2026 23:02
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.

2 participants