Merge pull request rsim#2741 from yahonda/fix-dbms-metadata-multi-stmt-split

Split multi-statement TABLE DDL CLOBs in the DBMS_METADATA dump path

Author: yahonda

lib/active_record/connection_adapters/oracle_enhanced/structure_dump/dbms_metadata.rb

@@ -26,7 +26,7 @@ module DbmsMetadata # :nodoc:
 
           private
             def dbms_metadata_structure_dump
-              dbms_metadata_with_transforms do
+              dbms_metadata_with_transforms(sql_terminator: true) do
                 structure = []
                 table_names = list_schema_objects("TABLE", non_mview_only: true)
                 skip_indexes = constraint_backed_index_names
@@ -41,10 +41,12 @@ def dbms_metadata_structure_dump
                   names.each do |name|
                     next if object_type == "INDEX" && skip_indexes.include?(name.upcase)
                     ddl = dbms_metadata_get_ddl(object_type.tr(" ", "_"), name)
-                    if ddl && object_type == "INDEX" && invisible_indexes.include?(name.upcase)
-                      ddl = ensure_invisible_keyword(ddl)
+                    next unless ddl
+                    statements = split_dbms_metadata_sql_ddl(ddl)
+                    if object_type == "INDEX" && invisible_indexes.include?(name.upcase)
+                      statements = statements.map { |stmt| ensure_invisible_keyword(stmt) }
                     end
-                    structure << ddl if ddl
+                    structure.concat(statements)
                   end
                 end
 
@@ -66,7 +68,7 @@ def dbms_metadata_structure_dump
 
                 fk_statements = table_names.flat_map do |table_name|
                   fk_ddl = dbms_metadata_get_dependent_ddl("REF_CONSTRAINT", table_name)
-                  fk_ddl ? split_dbms_metadata_ddl(fk_ddl) : []
+                  fk_ddl ? split_dbms_metadata_sql_ddl(fk_ddl) : []
                 end
 
                 join_with_statement_token(structure) << join_with_statement_token(fk_statements)
@@ -94,8 +96,8 @@ def dbms_metadata_structure_dump_synonyms
               end
             end
 
-            def dbms_metadata_with_transforms
-              configure_dbms_metadata_transforms
+            def dbms_metadata_with_transforms(sql_terminator: false)
+              configure_dbms_metadata_transforms(sql_terminator: sql_terminator)
               yield
             ensure
               reset_dbms_metadata_transforms
@@ -179,14 +181,22 @@ def dbms_metadata_structure_dump_column_comments(table_name)
 
             # Suppress installation-specific output, in spirit of
             # `pg_dump --schema-only --no-owner --no-tablespaces`.
-            def configure_dbms_metadata_transforms
+            # When `sql_terminator: true` is requested, also tell Oracle
+            # to append a SQL terminator (`;`) to each statement so the
+            # caller can split a multi-statement TABLE DDL CLOB on the
+            # boundary; this is only safe for the SQL DDL path (TABLE /
+            # SEQUENCE / VIEW / INDEX). The PL/SQL DDL path leaves
+            # SQLTERMINATOR at the default FALSE because PL/SQL bodies
+            # contain `;` characters internally (`END;`).
+            def configure_dbms_metadata_transforms(sql_terminator: false)
               execute(<<~SQL)
                 BEGIN
                   DBMS_METADATA.SET_TRANSFORM_PARAM(DBMS_METADATA.SESSION_TRANSFORM, 'STORAGE', FALSE);
                   DBMS_METADATA.SET_TRANSFORM_PARAM(DBMS_METADATA.SESSION_TRANSFORM, 'TABLESPACE', FALSE);
                   DBMS_METADATA.SET_TRANSFORM_PARAM(DBMS_METADATA.SESSION_TRANSFORM, 'SEGMENT_ATTRIBUTES', FALSE);
                   DBMS_METADATA.SET_TRANSFORM_PARAM(DBMS_METADATA.SESSION_TRANSFORM, 'EMIT_SCHEMA', FALSE);
                   DBMS_METADATA.SET_TRANSFORM_PARAM(DBMS_METADATA.SESSION_TRANSFORM, 'REF_CONSTRAINTS', FALSE);
+                  DBMS_METADATA.SET_TRANSFORM_PARAM(DBMS_METADATA.SESSION_TRANSFORM, 'SQLTERMINATOR', #{sql_terminator ? "TRUE" : "FALSE"});
                 END;
               SQL
             end
@@ -246,6 +256,19 @@ def split_dbms_metadata_ddl(ddl)
               return [] if ddl.nil?
               ddl.split(/\n\s*\n/).map(&:strip).reject(&:empty?)
             end
+
+            # `GET_DDL('TABLE', t)` can return more than one SQL statement
+            # in a single CLOB when the table has a UNIQUE constraint
+            # backed by a separately-named index (Oracle emits the
+            # `CREATE TABLE`, the `CREATE UNIQUE INDEX`, and the
+            # `ALTER TABLE ... ADD CONSTRAINT ... USING INDEX ...` as a
+            # group). With `SQLTERMINATOR=TRUE` each statement ends in
+            # `;`, so split on the terminator to surface each statement
+            # to the structure-dump output as its own entry.
+            def split_dbms_metadata_sql_ddl(ddl)
+              return [] if ddl.nil?
+              ddl.split(/;\s*(?:\n|\z)/).map(&:strip).reject(&:empty?)
+            end
         end
       end
     end

spec/active_record/connection_adapters/oracle_enhanced/dbms_metadata_structure_dump_spec.rb

@@ -157,13 +157,6 @@
         stmt.match?(/\AALTER\s+INDEX\s+"?IX_DBMS_META_INV_UNIQ"?\s+INVISIBLE\s*\z/i)
       end
       expect(alter_stmt).not_to be_nil
-
-      # And the standalone CREATE INDEX should NOT be emitted (the
-      # constraint inline already created the index).
-      standalone = dump.split("\n\n/\n\n").select do |stmt|
-        stmt.match?(/\ACREATE\s+(?:UNIQUE\s+)?INDEX\s+"?IX_DBMS_META_INV_UNIQ"?/i)
-      end
-      expect(standalone).to be_empty
     ensure
       schema_define { drop_table :test_dbms_meta_inv_uniq, if_exists: true }
     end
@@ -232,21 +225,89 @@
       expect(dump).not_to match(/PCTFREE\s+/i)
     end
 
-    it "does not emit a standalone CREATE INDEX for a UNIQUE constraint's backing index" do
+    it "splits a UNIQUE-constraint-with-named-backing-index into separate CREATE INDEX and ADD CONSTRAINT statements" do
       schema_define do
         create_table :test_dbms_metadata_posts, force: true do |t|
           t.string :email
         end
         add_index :test_dbms_metadata_posts, :email, unique: true, name: "ix_test_dbms_metadata_email"
       end
       dump = @conn.structure_dump
-      # `CONSTRAINTS=TRUE` already inlines the unique constraint (and its
-      # backing index) into the CREATE TABLE DDL. Emitting a separate
-      # `CREATE UNIQUE INDEX` for the same name would duplicate it.
-      standalone_index_stmts = dump.split("\n\n/\n\n").select do |stmt|
+      stmts = dump.split("\n\n/\n\n")
+
+      # `GET_DDL('TABLE', ...)` returns CREATE TABLE, CREATE UNIQUE INDEX,
+      # and ALTER TABLE ... ADD CONSTRAINT ... USING INDEX as one CLOB
+      # when the UNIQUE constraint references a named backing index.
+      # `SQLTERMINATOR=TRUE` plus `split_dbms_metadata_sql_ddl` splits
+      # them so each one is a separate dump entry; `structure_load`
+      # would otherwise feed the whole CLOB as one statement and Oracle
+      # would reject it with ORA-00922.
+      create_index_stmts = stmts.select do |stmt|
         stmt.match?(/\ACREATE\s+UNIQUE\s+INDEX\s+"?IX_TEST_DBMS_METADATA_EMAIL"?/i)
       end
-      expect(standalone_index_stmts).to be_empty
+      expect(create_index_stmts.size).to eq(1)
+
+      add_constraint_stmts = stmts.select do |stmt|
+        stmt.match?(/\AALTER\s+TABLE\s+"?TEST_DBMS_METADATA_POSTS"?\s+ADD\s+CONSTRAINT\s+"?IX_TEST_DBMS_METADATA_EMAIL"?\s+UNIQUE/i)
+      end
+      expect(add_constraint_stmts.size).to eq(1)
+    end
+
+    # Round-trip regression for the same table shape: dump → drop → load
+    # used to fail with ORA-00922 because the multi-statement `GET_DDL`
+    # CLOB was sent to Oracle as a single statement.
+    it "round-trips a UNIQUE-with-named-backing-index table through structure_dump/structure_load" do
+      require "tempfile"
+      schema_define do
+        create_table :test_dbms_metadata_posts, force: true do |t|
+          t.string :email
+        end
+        add_index :test_dbms_metadata_posts, :email, unique: true, name: "ix_test_dbms_metadata_email"
+      end
+
+      temp_file = Tempfile.create(["oracle_enhanced_roundtrip", ".sql"]).path
+      config = ActiveRecord::Base.connection_db_config.configuration_hash
+
+      begin
+        ActiveRecord::Tasks::DatabaseTasks.structure_dump(config, temp_file)
+        ActiveRecord::Tasks::DatabaseTasks.drop(config)
+        ActiveRecord::Tasks::DatabaseTasks.structure_load(config, temp_file)
+        expect(@conn.table_exists?(:test_dbms_metadata_posts)).to be(true)
+      ensure
+        File.unlink(temp_file) if File.exist?(temp_file)
+      end
+    end
+
+    # Round-trip regression for foreign keys: with SQLTERMINATOR=TRUE
+    # the dependent FK DDL also ends with ';', so the dump must run it
+    # through `split_dbms_metadata_sql_ddl` to strip the trailing
+    # terminator. Otherwise the load raises ORA-00911 on the trailing ';'.
+    it "round-trips foreign-key constraints through structure_dump/structure_load" do
+      require "tempfile"
+      schema_define do
+        create_table :test_dbms_metadata_posts, force: true do |t|
+          t.string :title
+        end
+        create_table :test_dbms_metadata_children, force: true do |t|
+          t.integer :test_dbms_metadata_post_id
+        end
+        add_foreign_key :test_dbms_metadata_children, :test_dbms_metadata_posts,
+                        column: :test_dbms_metadata_post_id, name: "fk_dbms_meta_rt"
+      end
+
+      temp_file = Tempfile.create(["oracle_enhanced_fk_rt", ".sql"]).path
+      config = ActiveRecord::Base.connection_db_config.configuration_hash
+
+      begin
+        ActiveRecord::Tasks::DatabaseTasks.structure_dump(config, temp_file)
+        ActiveRecord::Tasks::DatabaseTasks.drop(config)
+        ActiveRecord::Tasks::DatabaseTasks.structure_load(config, temp_file)
+        expect(@conn.table_exists?(:test_dbms_metadata_children)).to be(true)
+        fk = @conn.foreign_keys(:test_dbms_metadata_children).find { |f| f.name == "fk_dbms_meta_rt" }
+        expect(fk).not_to be_nil
+      ensure
+        File.unlink(temp_file) if File.exist?(temp_file)
+      end
     end
   end