From e00fbabb353484428e68673b8a6628ba887eeea3 Mon Sep 17 00:00:00 2001 From: Mo-Moore <33098900+Mo-Moore@users.noreply.github.com> Date: Thu, 17 Sep 2026 19:28:50 +0800 Subject: [PATCH] fix(oracle): include dictionary comments in table and view DDL --- agents/drivers/oracle-go/main.go | 58 ++- agents/drivers/oracle-go/main_test.go | 183 ++++++++ crates/dbx-core/src/object_source_sql.rs | 49 ++- crates/dbx-core/src/schema.rs | 519 ++++++++++++++++++++++- 4 files changed, 791 insertions(+), 18 deletions(-) diff --git a/agents/drivers/oracle-go/main.go b/agents/drivers/oracle-go/main.go index 9802f64172..e003a37b3f 100644 --- a/agents/drivers/oracle-go/main.go +++ b/agents/drivers/oracle-go/main.go @@ -3389,10 +3389,64 @@ func (s *server) buildViewDDL(schema, name string) (string, error) { } trimmed := strings.TrimSpace(source) upperSource := strings.ToUpper(trimmed) + var ddl string if strings.HasPrefix(upperSource, "CREATE ") || strings.HasPrefix(upperSource, "ALTER ") { - return trimmed, nil + ddl = trimmed + } else { + ddl = fmt.Sprintf("CREATE OR REPLACE VIEW %s.%s AS\n%s", quoteIdentifier(schema), quoteIdentifier(name), trimmed) } - return fmt.Sprintf("CREATE OR REPLACE VIEW %s.%s AS\n%s", quoteIdentifier(schema), quoteIdentifier(name), trimmed), nil + return s.appendViewCommentDDLs(schema, name, ddl), nil +} + +// appendViewCommentDDLs appends COMMENT ON TABLE/COLUMN statements for views. +// Oracle stores view comments in ALL_TAB_COMMENTS / ALL_COL_COMMENTS the same way as tables. +func (s *server) appendViewCommentDDLs(schema, name, viewDDL string) string { + comments, err := s.loadTableCommentDDLs(schema, name) + if err != nil || len(comments) == 0 { + return viewDDL + } + var builder strings.Builder + builder.WriteString(terminateOracleViewDDL(strings.TrimSpace(viewDDL))) + for _, comment := range comments { + appendOracleDDLFragment(&builder, comment) + } + return builder.String() +} + +func terminateOracleViewDDL(ddl string) string { + var lastCode byte + trailingLineComment := false + for pos := 0; pos < len(ddl); pos++ { + if isSQLWhitespace(ddl[pos]) { + continue + } + if ddl[pos] == '-' && pos+1 < len(ddl) && ddl[pos+1] == '-' { + pos = skipLineCommentSQL(ddl, pos) + trailingLineComment = true + continue + } + if ddl[pos] == '/' && pos+1 < len(ddl) && ddl[pos+1] == '*' { + pos = skipBlockCommentSQL(ddl, pos) + trailingLineComment = false + continue + } + if end, ok := skipOracleAlternativeQuotedSQL(ddl, pos); ok { + pos = end + } else if ddl[pos] == '\'' { + pos = skipSingleQuotedSQL(ddl, pos) + } else if ddl[pos] == '"' { + pos = skipDoubleQuotedSQL(ddl, pos) + } + lastCode = ddl[pos] + trailingLineComment = false + } + if lastCode == ';' || lastCode == '/' { + return ddl + } + if trailingLineComment { + return ddl + "\n;" + } + return ddl + ";" } func (s *server) getViewSource(schema, name string) (string, error) { diff --git a/agents/drivers/oracle-go/main_test.go b/agents/drivers/oracle-go/main_test.go index 5c3668fc2f..31e8c5607e 100644 --- a/agents/drivers/oracle-go/main_test.go +++ b/agents/drivers/oracle-go/main_test.go @@ -629,6 +629,189 @@ func TestGetTableDDLAppendsIndexesTriggersAndComments(t *testing.T) { } } +func TestBuildViewDDLAppendsComments(t *testing.T) { + const schema = "HR" + const view = "ACTIVE_ORDERS" + const viewText = `SELECT "ID", "STATUS" FROM "HR"."ORDERS" WHERE "STATUS" = 'OPEN'` + db, scripted := openOracleViewSourceTestDB(t, []oracleViewSourceQueryStep{ + { + queryContains: "FROM ALL_VIEWS", + args: []driver.Value{schema, view}, + rows: [][]driver.Value{{viewText}}, + }, + { + queryContains: "FROM ALL_TAB_COMMENTS", + args: []driver.Value{schema, view}, + rows: [][]driver.Value{{"Open orders view"}}, + }, + { + queryContains: "FROM ALL_COL_COMMENTS", + args: []driver.Value{schema, view}, + columns: []string{"COLUMN_NAME", "COMMENTS"}, + rows: [][]driver.Value{{"STATUS", "Order status"}}, + }, + }) + s := newServer() + s.db = db + + got, err := s.buildViewDDL(schema, view) + if err != nil { + t.Fatal(err) + } + for _, fragment := range []string{ + `CREATE OR REPLACE VIEW "HR"."ACTIVE_ORDERS" AS`, + viewText, + `COMMENT ON TABLE "HR"."ACTIVE_ORDERS" IS 'Open orders view';`, + `COMMENT ON COLUMN "HR"."ACTIVE_ORDERS"."STATUS" IS 'Order status';`, + } { + if !strings.Contains(got, fragment) { + t.Fatalf("buildViewDDL() missing %q:\n%s", fragment, got) + } + } + if !strings.Contains(got, viewText+";\n\nCOMMENT ON TABLE") { + t.Fatalf("view DDL should be terminated before comment DDL:\n%s", got) + } + if scripted.next != len(scripted.steps) { + t.Fatalf("expected %d queries, got %d", len(scripted.steps), scripted.next) + } +} + +func TestBuildViewDDLCommentBoundaries(t *testing.T) { + const query = `SELECT 1 AS "ID" FROM DUAL` + for _, test := range []struct { + name string + source string + terminated string + noComments bool + tableError bool + columnError bool + columnOnly bool + tableOnly bool + metadataFallback bool + }{ + {name: "plain", source: query, terminated: query + ";"}, + {name: "line comment", source: query + " -- trailing", terminated: query + " -- trailing\n;"}, + {name: "semicolon in comment", source: query + " -- trailing;", terminated: query + " -- trailing;\n;"}, + {name: "slash in comment", source: query + " -- trailing /", terminated: query + " -- trailing /\n;"}, + {name: "existing terminator", source: query + ";", terminated: query + ";"}, + {name: "slash delimiter", source: query + "\n/", terminated: query + "\n/"}, + {name: "terminated before line comment", source: query + "; -- trailing;", terminated: query + "; -- trailing;"}, + {name: "terminated before block comment", source: query + "; /* trailing; */", terminated: query + "; /* trailing; */"}, + {name: "block comment", source: query + " /* trailing; */", terminated: query + " /* trailing; */;"}, + {name: "quoted comment markers", source: `SELECT '--;', q'[owner's --;]' AS "--ID" FROM DUAL -- tail`, terminated: `SELECT '--;', q'[owner's --;]' AS "--ID" FROM DUAL -- tail` + "\n;"}, + {name: "full create", source: `CREATE VIEW "HR"."ACTIVE_ORDERS" AS ` + query + " -- tail", terminated: `CREATE VIEW "HR"."ACTIVE_ORDERS" AS ` + query + " -- tail\n;"}, + {name: "metadata fallback", source: `CREATE VIEW "HR"."ACTIVE_ORDERS" AS ` + query + " -- tail;", terminated: `CREATE VIEW "HR"."ACTIVE_ORDERS" AS ` + query + " -- tail;\n;", metadataFallback: true}, + {name: "column only", source: query + " -- tail;", terminated: query + " -- tail;\n;", columnOnly: true}, + {name: "table only", source: query + " -- tail;", terminated: query + " -- tail;\n;", tableOnly: true}, + {name: "no comments", source: query, noComments: true}, + {name: "no comments with trailing comment", source: query + " -- tail;\n", noComments: true}, + {name: "table lookup failure", source: query + " -- tail", tableError: true}, + {name: "column lookup failure", source: query + " -- tail", columnError: true}, + } { + t.Run(test.name, func(t *testing.T) { + tableStep := oracleViewSourceQueryStep{queryContains: "FROM ALL_TAB_COMMENTS", args: []driver.Value{"HR", "ACTIVE_ORDERS"}} + columnStep := oracleViewSourceQueryStep{queryContains: "FROM ALL_COL_COMMENTS", args: []driver.Value{"HR", "ACTIVE_ORDERS"}, columns: []string{"COLUMN_NAME", "COMMENTS"}} + if !test.noComments { + if !test.columnOnly { + tableStep.rows = [][]driver.Value{{"View's comment"}} + } + if !test.tableOnly { + columnStep.rows = [][]driver.Value{{"ID", "Column's comment"}} + } + } + if test.tableError { + tableStep.err = errors.New("dictionary denied") + } + if test.columnError { + columnStep.err = errors.New("dictionary denied") + } + steps := []oracleViewSourceQueryStep{ + {queryContains: "FROM ALL_VIEWS", args: []driver.Value{"HR", "ACTIVE_ORDERS"}, rows: [][]driver.Value{{test.source}}}, + } + if test.metadataFallback { + steps[0].err = errors.New("view text unavailable") + steps = append(steps, oracleViewSourceQueryStep{ + queryContains: "DBMS_METADATA.GET_DDL('VIEW'", args: []driver.Value{"ACTIVE_ORDERS", "HR"}, rows: [][]driver.Value{{test.source}}, + }) + } + steps = append(steps, tableStep) + if !test.tableError { + steps = append(steps, columnStep) + } + database, scripted := openOracleViewSourceTestDB(t, steps) + server := newServer() + server.db = database + got, err := server.buildViewDDL("HR", "ACTIVE_ORDERS") + if err != nil { + t.Fatal(err) + } + want := test.terminated + if test.noComments || test.tableError || test.columnError { + want = strings.TrimSpace(test.source) + } else { + if !test.columnOnly { + want += "\n\nCOMMENT ON TABLE \"HR\".\"ACTIVE_ORDERS\" IS 'View''s comment';" + } + if !test.tableOnly { + want += "\n\nCOMMENT ON COLUMN \"HR\".\"ACTIVE_ORDERS\".\"ID\" IS 'Column''s comment';" + } + } + if !strings.HasPrefix(want, "CREATE ") { + want = "CREATE OR REPLACE VIEW \"HR\".\"ACTIVE_ORDERS\" AS\n" + want + } + if got != want { + t.Fatalf("buildViewDDL() = %q, want %q", got, want) + } + if scripted.next != len(scripted.steps) { + t.Fatalf("expected %d queries, got %d", len(scripted.steps), scripted.next) + } + }) + } +} + +func TestGetTableDDLForViewAppendsComments(t *testing.T) { + const schema = "HR" + const view = "ACTIVE_ORDERS" + const viewText = `SELECT 1 AS "ID" FROM DUAL` + db, scripted := openOracleViewSourceTestDB(t, []oracleViewSourceQueryStep{ + { + queryContains: "FROM ALL_VIEWS", + args: []driver.Value{schema, view}, + rows: [][]driver.Value{{viewText}}, + }, + { + queryContains: "FROM ALL_TAB_COMMENTS", + args: []driver.Value{schema, view}, + rows: [][]driver.Value{{"Active orders"}}, + }, + { + queryContains: "FROM ALL_COL_COMMENTS", + args: []driver.Value{schema, view}, + columns: []string{"COLUMN_NAME", "COMMENTS"}, + rows: [][]driver.Value{{"ID", "Row id"}}, + }, + }) + s := newServer() + s.db = db + + got, err := s.getTableDDL(schema, view, "VIEW") + if err != nil { + t.Fatal(err) + } + if !strings.Contains(got, `CREATE OR REPLACE VIEW "HR"."ACTIVE_ORDERS" AS`) { + t.Fatalf("expected view create DDL, got:\n%s", got) + } + if !strings.Contains(got, `COMMENT ON TABLE "HR"."ACTIVE_ORDERS" IS 'Active orders';`) { + t.Fatalf("expected view table comment, got:\n%s", got) + } + if !strings.Contains(got, `COMMENT ON COLUMN "HR"."ACTIVE_ORDERS"."ID" IS 'Row id';`) { + t.Fatalf("expected view column comment, got:\n%s", got) + } + if scripted.next != len(scripted.steps) { + t.Fatalf("expected %d queries, got %d", len(scripted.steps), scripted.next) + } +} + func TestGetPortableTableDDLDisablesAndRestoresSegmentAttributes(t *testing.T) { const schema = "HR" const table = "ORDERS" diff --git a/crates/dbx-core/src/object_source_sql.rs b/crates/dbx-core/src/object_source_sql.rs index 1f2a464559..fb7cb144ff 100644 --- a/crates/dbx-core/src/object_source_sql.rs +++ b/crates/dbx-core/src/object_source_sql.rs @@ -1,5 +1,7 @@ use regex::Regex; use serde::{Deserialize, Serialize}; +use sqlparser::dialect::OracleDialect; +use sqlparser::tokenizer::{Token, Tokenizer, Whitespace}; use crate::models::connection::DatabaseType; use crate::types::ObjectSourceKind; @@ -225,8 +227,16 @@ pub fn build_editable_object_source(input: EditableObjectSourceSqlInput) -> Stri pub fn build_view_ddl_sql(input: BuildViewDdlInput) -> String { let source = input.source.trim(); + let terminated_source = if matches!( + input.database_type, + Some(DatabaseType::Oracle | DatabaseType::OceanbaseOracle | DatabaseType::Dameng) + ) { + ensure_oracle_ddl_terminated(source) + } else { + ensure_semicolon(source) + }; if Regex::new(r"(?i)^(?:CREATE|ALTER)\s+").unwrap().is_match(source) { - return ensure_semicolon(source); + return terminated_source; } // Backtick-quoting is used for MySQL-family engines and for connections @@ -248,10 +258,10 @@ pub fn build_view_ddl_sql(input: BuildViewDdlInput) -> String { || database_type == DatabaseType::Questdb }) { - return format!("CREATE OR REPLACE VIEW {qualified_name} AS\n{}", ensure_semicolon(source)); + return format!("CREATE OR REPLACE VIEW {qualified_name} AS\n{terminated_source}"); } - format!("CREATE VIEW {qualified_name} AS\n{}", ensure_semicolon(source)) + format!("CREATE VIEW {qualified_name} AS\n{terminated_source}") } pub fn build_export_object_source_sql( @@ -366,6 +376,23 @@ fn quote_mysql_identifier(value: &str) -> String { format!("`{}`", value.replace('`', "``")) } +pub(crate) fn ensure_oracle_ddl_terminated(sql: &str) -> String { + let trimmed = sql.trim_end(); + if trimmed.trim().is_empty() { + return String::new(); + } + if let Ok(tokens) = Tokenizer::new(&OracleDialect {}, trimmed).tokenize() { + let last_code = tokens.iter().rev().find(|token| !matches!(token, Token::Whitespace(_))); + if matches!(last_code, Some(Token::SemiColon | Token::Div)) { + return trimmed.to_string(); + } + if !matches!(tokens.last(), Some(Token::Whitespace(Whitespace::SingleLineComment { .. }))) { + return format!("{trimmed};"); + } + } + format!("{trimmed}\n;") +} + fn ensure_semicolon(sql: &str) -> String { let trimmed = sql.trim(); if trimmed.ends_with(';') { @@ -967,6 +994,22 @@ fn parse_object_source_kind(value: &str) -> Option { mod tests { use super::*; + #[test] + fn oracle_ddl_terminator_respects_quoted_tokens_and_slash_delimiters() { + for (source, expected) in [ + ("SELECT q'[owner's --;]' FROM DUAL -- tail;", "SELECT q'[owner's --;]' FROM DUAL -- tail;\n;"), + ("SELECT q'[owner's --;]' FROM DUAL; -- tail;", "SELECT q'[owner's --;]' FROM DUAL; -- tail;"), + ("SELECT 1 AS \"名--称\" FROM DUAL -- tail;", "SELECT 1 AS \"名--称\" FROM DUAL -- tail;\n;"), + ("SELECT 1 FROM DUAL\n/", "SELECT 1 FROM DUAL\n/"), + ("SELECT 1 FROM DUAL;\n/", "SELECT 1 FROM DUAL;\n/"), + ("SELECT 1 FROM DUAL\n/\n-- tail;", "SELECT 1 FROM DUAL\n/\n-- tail;"), + (" CREATE TABLE t (id NUMBER);\n", " CREATE TABLE t (id NUMBER);"), + (" \n", ""), + ] { + assert_eq!(ensure_oracle_ddl_terminated(source), expected); + } + } + fn input(database_type: DatabaseType, object_type: ObjectSourceKind, source: &str) -> EditableObjectSourceSqlInput { EditableObjectSourceSqlInput { database_type, diff --git a/crates/dbx-core/src/schema.rs b/crates/dbx-core/src/schema.rs index c6d6505113..49dd37c3de 100644 --- a/crates/dbx-core/src/schema.rs +++ b/crates/dbx-core/src/schema.rs @@ -1232,6 +1232,18 @@ async fn get_table_comment_core_for_session( { let pool_handle = state.pool_handle(&pool_key).await; try_sqlserver!(pool_handle, get_table_comment, schema, table); + if let Some(PoolKind::ExternalDriver { config, session, .. }) = pool_handle.as_ref() { + if is_oracle_external_driver_config(config.as_ref()) { + return external_driver_oracle_table_comment( + session.clone(), + config.as_ref(), + database, + schema, + table, + ) + .await; + } + } if let Some(client) = extract_pool!(pool_handle.as_ref(), Agent) { if db_config.as_ref().is_some_and(|config| { matches!(config.db_type, DatabaseType::Oracle | DatabaseType::OceanbaseOracle) @@ -3180,12 +3192,12 @@ mod tests { presto_like_columns_from_query_result, presto_like_information_schema_columns_sql, presto_like_information_schema_tables_sql, presto_like_tables_from_query_result, reference_key_columns_from_indexes, reference_keys_from_indexes, replace_metadata_runtime, - should_query_oracle_columns_via_sql_first, table_comments_from_query_result, table_name_filter_matches, - tdengine_table_comment_like_pattern, tdengine_table_comment_sql, tdengine_table_comments_sql, - uses_mongodb_agent_collection_listing, visible_schema_filter, ExternalDriverStatisticsDialect, - MetadataErrorAction, MysqlTableListSource, OracleObjectRef, OracleSynonymResolver, ReferenceKeyInfo, - TableNameFilter, ORACLE_CURRENT_SCHEMA_SQL, ORACLE_SYNONYM_MAX_DEPTH, TDENGINE_COMMENT_SEARCH_TIMEOUT, - TDENGINE_LIKE_PATTERN_MAX_BYTES, + should_append_oracle_style_comment_ddl, should_query_oracle_columns_via_sql_first, + table_comments_from_query_result, table_name_filter_matches, tdengine_table_comment_like_pattern, + tdengine_table_comment_sql, tdengine_table_comments_sql, uses_mongodb_agent_collection_listing, + visible_schema_filter, ExternalDriverStatisticsDialect, MetadataErrorAction, MysqlTableListSource, + OracleObjectRef, OracleSynonymResolver, ReferenceKeyInfo, TableNameFilter, ORACLE_CURRENT_SCHEMA_SQL, + ORACLE_SYNONYM_MAX_DEPTH, TDENGINE_COMMENT_SEARCH_TIMEOUT, TDENGINE_LIKE_PATTERN_MAX_BYTES, }; use super::{list_databases_core, list_tables_core}; use super::{ @@ -3709,6 +3721,270 @@ done assert!(!is_oracle_external_driver_config(&config)); } + #[cfg(unix)] + mod oracle_ddl_regression_tests { + use super::*; + use crate::types::ObjectSourceKind; + use serde_json::{json, Value}; + use std::os::unix::fs::PermissionsExt; + use std::path::PathBuf; + + async fn scripted_oracle_driver(source: Value, columns: Value, comment: Value) -> (AppState, PathBuf) { + let dir = std::env::temp_dir().join(format!("dbx-oracle-ddl-test-{}", uuid::Uuid::new_v4())); + std::fs::create_dir_all(&dir).unwrap(); + for (method, response) in [("getObjectSource", source), ("getColumns", columns), ("executeQuery", comment)] + { + std::fs::write(dir.join(format!("{method}.json")), response.to_string()).unwrap(); + } + let executable = dir.join("plugin.sh"); + std::fs::write( + &executable, + r#"#!/bin/sh +base=$(dirname "$0") +while IFS= read -r line; do + printf '%s\n' "$line" >> "$base/calls.log" + id=$(printf '%s' "$line" | sed -E 's/.*"id":([0-9]+).*/\1/') + method=$(printf '%s' "$line" | sed -E 's/.*"method":"([^"]+)".*/\1/') + case "$method" in + getObjectSource|getColumns|executeQuery) + response=$(cat "$base/$method.json") + printf '%s\n' "$response" | sed 's/^{/{"id":'"$id"',/' + ;; + *) printf '{"id":%s,"error":{"message":"unsupported test method"}}\n' "$id" ;; + esac +done +"#, + ) + .unwrap(); + std::fs::set_permissions(&executable, std::fs::Permissions::from_mode(0o755)).unwrap(); + let plugin = InstalledPlugin { + manifest: PluginManifest { + id: "jdbc".into(), + name: "JDBC".into(), + version: "test".into(), + protocol_version: 1, + executable: Some("plugin.sh".into()), + drivers: vec![PluginDriverManifest { + id: "jdbc".into(), + label: "JDBC".into(), + kind: "external".into(), + database_type: Some("jdbc".into()), + }], + ..Default::default() + }, + path: dir.clone(), + compatibility: crate::plugins::PluginCompatibility { + compatible: true, + backend_executable: Some(executable), + ..Default::default() + }, + }; + let session = std::sync::Arc::new( + PluginDriverSession::start_for_test(plugin, "jdbc".into(), PluginRuntimeEnv::default()).await.unwrap(), + ); + let state = AppState::new(Storage::open(&dir.join("storage.db")).await.unwrap()); + let mut config = test_connection_config(DatabaseType::Jdbc); + config.id = "oracle-ddl".into(); + config.database = Some("demo".into()); + config.connection_string = Some("jdbc:oracle:thin:@127.0.0.1:1521/demo".into()); + config.jdbc_driver_class = Some("oracle.jdbc.OracleDriver".into()); + state.configs.write().await.insert(config.id.clone(), config.clone()); + state + .update_connection_pools(|connections| { + connections.insert( + config.id.clone(), + PoolKind::ExternalDriver { + driver_id: "jdbc".into(), + config: std::sync::Arc::new(config), + session, + }, + ); + }) + .await; + (state, dir) + } + + fn column_response() -> Value { + json!({"result": [db::ColumnInfo { + name: "ID".into(), comment: Some("Column's comment".into()), ..Default::default() + }]}) + } + + fn comment_response() -> Value { + json!({"result": {"columns": ["COMMENTS"], "rows": [["View's comment"]], "affected_rows": 0, "execution_time_ms": 0}}) + } + + fn driver_calls(dir: &std::path::Path) -> Vec { + std::fs::read_to_string(dir.join("calls.log")) + .unwrap() + .lines() + .map(|line| serde_json::from_str(line).unwrap()) + .collect() + } + + #[tokio::test] + async fn canonical_schema_reaches_queries_and_identifiers() { + for (requested, returned, expected) in [ + ("hr", Some(json!("HR")), "HR"), + ("", Some(json!("HR")), "HR"), + ("mixed_owner", Some(json!("mixed_owner")), "mixed_owner"), + ("Mixed\"Owner", Some(json!("Mixed\"Owner")), "Mixed\"Owner"), + ("FallbackOwner", None, "FallbackOwner"), + ("FallbackOwner", Some(Value::Null), "FallbackOwner"), + ("FallbackOwner", Some(json!("")), "FallbackOwner"), + ("FallbackOwner", Some(json!(" ")), "FallbackOwner"), + ("", None, ""), + ] { + for shape in ["table", "view body", "view ddl"] { + let object_type = (shape != "table").then_some(ObjectSourceKind::View); + let qualified = if expected.is_empty() { + "\"ORDERS\"".to_string() + } else { + format!("{}.\"ORDERS\"", crate::schema::oracle_ident(expected)) + }; + let source = match shape { + "table" => format!("CREATE TABLE {qualified} (\"ID\" NUMBER)"), + "view ddl" => format!("CREATE VIEW {qualified} AS SELECT 1 FROM DUAL -- tail;"), + _ => "SELECT 1 FROM DUAL -- tail;".to_string(), + }; + let mut result = json!({"name": "ORDERS", "object_type": if object_type.is_some() { "VIEW" } else { "TABLE" }, "source": source}); + if let Some(returned) = returned.clone() { + result["schema"] = returned; + } + let (state, dir) = + scripted_oracle_driver(json!({"result": result}), column_response(), comment_response()).await; + let ddl = crate::schema::get_table_display_ddl_core( + &state, + "oracle-ddl", + "demo", + requested, + "ORDERS", + object_type, + ) + .await + .unwrap(); + assert!( + ddl.starts_with(&format!( + "CREATE {} {qualified}", + if shape == "table" { "TABLE" } else { "VIEW" } + )), + "{ddl}" + ); + assert!(ddl.contains(&format!("COMMENT ON TABLE {qualified} IS 'View''s comment';")), "{ddl}"); + assert!( + ddl.contains(&format!("COMMENT ON COLUMN {qualified}.\"ID\" IS 'Column''s comment';")), + "{ddl}" + ); + assert_eq!( + crate::sql::split_sql_statements_for_database(&ddl, DatabaseType::Oracle).len(), + 3, + "{ddl}" + ); + let calls = driver_calls(&dir); + let source_call = calls.iter().find(|call| call["method"] == "getObjectSource").unwrap(); + assert_eq!(source_call["params"]["schema"], requested); + let columns_call = calls.iter().find(|call| call["method"] == "getColumns").unwrap(); + assert_eq!(columns_call["params"]["schema"], expected); + let comment_call = calls + .iter() + .find(|call| call["params"]["sql"].as_str().is_some_and(|sql| sql.contains("ALL_TAB_COMMENTS"))) + .unwrap(); + assert_eq!(comment_call["params"]["schema"], expected); + assert_eq!( + comment_call["params"]["sql"], + crate::schema::oracle_table_comment_sql(expected, "ORDERS") + ); + state.shutdown(Duration::from_secs(1)).await; + std::fs::remove_dir_all(dir).unwrap(); + } + } + } + + #[tokio::test] + async fn optional_metadata_failures_preserve_source_and_available_comments() { + let error = json!({"error": {"message": "dictionary denied"}}); + for (columns, comment, comment_count) in [ + ( + json!({"result": []}), + json!({"result": {"columns": ["COMMENTS"], "rows": [], "affected_rows": 0, "execution_time_ms": 0}}), + 0, + ), + (error.clone(), comment_response(), 1), + (column_response(), error.clone(), 1), + (error.clone(), error, 0), + ] { + for object_type in [None, Some(ObjectSourceKind::View)] { + let source = if object_type.is_none() { + "CREATE TABLE \"HR\".\"ORDERS\" (\"ID\" NUMBER);" + } else { + "CREATE VIEW \"HR\".\"ORDERS\" AS SELECT 1 FROM DUAL -- tail;" + }; + let (state, dir) = scripted_oracle_driver( + json!({"result": {"name": "ORDERS", "object_type": if object_type.is_some() { "VIEW" } else { "TABLE" }, "schema": "HR", "source": source}}), + columns.clone(), comment.clone(), + ).await; + let ddl = crate::schema::get_table_display_ddl_core( + &state, + "oracle-ddl", + "demo", + "hr", + "ORDERS", + object_type, + ) + .await + .unwrap(); + assert!(ddl.starts_with(source), "{ddl}"); + assert_eq!(ddl.matches("COMMENT ON").count(), comment_count, "{ddl}"); + assert_eq!( + crate::sql::split_sql_statements_for_database(&ddl, DatabaseType::Oracle).len(), + 1 + comment_count, + "{ddl}" + ); + state.shutdown(Duration::from_secs(1)).await; + std::fs::remove_dir_all(dir).unwrap(); + } + } + } + + #[tokio::test] + async fn missing_or_failed_source_stops_before_comment_lookups() { + for (object_type, response) in [ + (None, json!({"result": {}})), + (None, json!({"result": {"source": " "}})), + (None, json!({"error": {"message": "source denied"}})), + (Some(ObjectSourceKind::View), json!({"error": {"message": "source denied"}})), + ] { + let (state, dir) = scripted_oracle_driver(response, column_response(), comment_response()).await; + let result = crate::schema::get_table_display_ddl_core( + &state, + "oracle-ddl", + "demo", + "hr", + "ORDERS", + object_type, + ) + .await; + assert!(result.is_err()); + assert!(driver_calls(&dir).iter().all(|call| call["method"] == "getObjectSource")); + state.shutdown(Duration::from_secs(1)).await; + std::fs::remove_dir_all(dir).unwrap(); + } + } + } + + #[test] + fn should_append_oracle_style_comment_ddl_for_oracle_family() { + assert!(should_append_oracle_style_comment_ddl(Some(&test_connection_config(DatabaseType::Oracle)))); + assert!(should_append_oracle_style_comment_ddl(Some(&test_connection_config(DatabaseType::OceanbaseOracle)))); + assert!(should_append_oracle_style_comment_ddl(Some(&test_connection_config(DatabaseType::Dameng)))); + assert!(!should_append_oracle_style_comment_ddl(Some(&test_connection_config(DatabaseType::Mysql)))); + assert!(!should_append_oracle_style_comment_ddl(None)); + + let mut jdbc_oracle = test_connection_config(DatabaseType::Jdbc); + jdbc_oracle.connection_string = Some("jdbc:oracle:thin:@localhost:1521/ORCL".to_string()); + assert!(should_append_oracle_style_comment_ddl(Some(&jdbc_oracle))); + } + #[test] fn external_driver_generic_ddl_applies_to_unmatched_jdbc_connections() { // JDBCX-style wrappers match no vendor DDL dialect and fall back to the @@ -8092,18 +8368,37 @@ async fn get_table_ddl_core_with_options( None, ) .await?; - let database_type = connection_config(state, connection_id).await.map(|config| config.db_type); + let db_config = connection_config(state, connection_id).await; + let append_oracle_comments = should_append_oracle_style_comment_ddl(db_config.as_ref()); + let schema = if append_oracle_comments { + source.schema.as_deref().filter(|schema| !schema.trim().is_empty()).unwrap_or(schema) + } else { + schema + }; + let database_type = db_config.as_ref().map(|config| { + if is_oracle_external_driver_config(config) { + DatabaseType::Oracle + } else { + config.db_type + } + }); // Kingbase MySQL compatibility mode reports a backtick identifier // quote; thread it through so the view DDL wraps hyphenated schema // names in backticks instead of double quotes the server rejects. let identifier_quote = state.connection_identifier_quote(connection_id, Some(database)).await.ok().flatten(); - return Ok(crate::object_source_sql::build_view_ddl_sql(crate::object_source_sql::BuildViewDdlInput { + let ddl = crate::object_source_sql::build_view_ddl_sql(crate::object_source_sql::BuildViewDdlInput { database_type, schema: if schema.trim().is_empty() { None } else { Some(schema.to_string()) }, name: table.to_string(), source: source.source, identifier_quote, - })); + }); + // Oracle-family comments live in dictionary tables, not inside CREATE VIEW. + // Append COMMENT ON so table-properties DDL / hover match the structure editor. + if append_oracle_comments { + return Ok(enrich_ddl_with_oracle_style_comments(state, connection_id, database, schema, table, &ddl).await); + } + return Ok(ddl); } if matches!(object_type, Some(db::ObjectSourceKind::MaterializedView)) { let source = get_object_source_core( @@ -9780,11 +10075,10 @@ fn append_oracle_comments_to_ddl( table_comment: Option<&str>, columns: &[db::ColumnInfo], ) -> String { - let mut result = ddl.trim_end().trim_end_matches(';').to_string(); + let mut result = crate::object_source_sql::ensure_oracle_ddl_terminated(ddl); if result.trim().is_empty() { return result; } - result.push(';'); let existing_ddl_upper = ddl.to_ascii_uppercase(); let table_ref = if schema.trim().is_empty() { @@ -9812,6 +10106,75 @@ fn append_oracle_comments_to_ddl( result } +fn should_append_oracle_style_comment_ddl(config: Option<&ConnectionConfig>) -> bool { + config.is_some_and(|config| { + matches!(config.db_type, DatabaseType::Oracle | DatabaseType::OceanbaseOracle | DatabaseType::Dameng) + || is_oracle_external_driver_config(config) + }) +} + +/// Enrich display DDL with dictionary comments for Oracle-family engines. +/// Failures are logged and the original DDL is returned unchanged. +async fn enrich_ddl_with_oracle_style_comments( + state: &AppState, + connection_id: &str, + database: &str, + schema: &str, + table: &str, + ddl: &str, +) -> String { + let columns = match get_columns_core(state, connection_id, database, schema, table).await { + Ok(columns) => columns, + Err(error) => { + log::debug!( + "[schema][oracle:display-ddl:columns-for-comments-failed] connection_id={} schema={} table={} error={}", + connection_id, + schema, + table, + error + ); + Vec::new() + } + }; + let table_comment = match get_table_comment_core(state, connection_id, database, schema, table).await { + Ok(comment) => comment, + Err(error) => { + log::debug!( + "[schema][oracle:display-ddl:table-comment-failed] connection_id={} schema={} table={} error={}", + connection_id, + schema, + table, + error + ); + None + } + }; + append_oracle_comments_to_ddl(ddl, schema, table, table_comment.as_deref(), &columns) +} + +async fn external_driver_oracle_table_comment( + session: std::sync::Arc, + config: &ConnectionConfig, + database: &str, + schema: &str, + table: &str, +) -> Result, String> { + let result: db::QueryResult = session + .invoke_with_timeout( + "executeQuery", + serde_json::json!({ + "connection": config, + "database": database, + "schema": schema, + "sql": oracle_table_comment_sql(schema, table), + "maxRows": 1 + }), + agent_metadata_timeout(Some(config)), + ) + .await?; + oracle_table_comment_from_query_result(result) +} + async fn db2_agent_table_ddl( client: Arc, database: &str, @@ -10450,6 +10813,95 @@ mod object_source_tests { assert!(!ddl.contains("EMPTY_COMMENT\" IS")); } + #[test] + fn appends_oracle_comments_to_view_ddl() { + let column = db::ColumnInfo { + name: "STATUS".to_string(), + data_type: "VARCHAR2(20)".to_string(), + is_nullable: true, + column_default: None, + is_primary_key: false, + extra: None, + comment: Some("Order status".to_string()), + numeric_precision: None, + numeric_scale: None, + character_maximum_length: None, + enum_values: None, + ..Default::default() + }; + + let ddl = append_oracle_comments_to_ddl( + "CREATE OR REPLACE VIEW \"HR\".\"ACTIVE_ORDERS\" AS\nSELECT \"STATUS\" FROM \"HR\".\"ORDERS\"", + "HR", + "ACTIVE_ORDERS", + Some("Open orders view"), + &[column], + ); + + assert!(ddl.contains("CREATE OR REPLACE VIEW \"HR\".\"ACTIVE_ORDERS\" AS")); + assert!(ddl.contains("COMMENT ON TABLE \"HR\".\"ACTIVE_ORDERS\" IS 'Open orders view';")); + assert!(ddl.contains("COMMENT ON COLUMN \"HR\".\"ACTIVE_ORDERS\".\"STATUS\" IS 'Order status';")); + } + + #[test] + fn oracle_view_comment_ddl_preserves_statement_boundaries() { + use crate::object_source_sql::{build_view_ddl_sql, BuildViewDdlInput}; + use crate::sql::split_sql_statements_for_database; + + let column = + db::ColumnInfo { name: "ID".into(), comment: Some("Column's comment".into()), ..Default::default() }; + for (source, terminated) in [ + ("SELECT 1 FROM DUAL", "SELECT 1 FROM DUAL;"), + ("SELECT 1 FROM DUAL;", "SELECT 1 FROM DUAL;"), + ("SELECT 1 FROM DUAL -- tail", "SELECT 1 FROM DUAL -- tail\n;"), + ("SELECT 1 FROM DUAL -- tail;\n", "SELECT 1 FROM DUAL -- tail;\n;"), + ("SELECT 1 FROM DUAL -- tail /", "SELECT 1 FROM DUAL -- tail /\n;"), + ("SELECT 1 FROM DUAL; -- tail;", "SELECT 1 FROM DUAL; -- tail;"), + ("SELECT 1 FROM DUAL; /* tail; */", "SELECT 1 FROM DUAL; /* tail; */"), + ("SELECT 1 FROM DUAL /* tail; */", "SELECT 1 FROM DUAL /* tail; */;"), + ("SELECT '--;' AS \"--ID\" FROM DUAL -- tail", "SELECT '--;' AS \"--ID\" FROM DUAL -- tail\n;"), + ] { + for database_type in [DatabaseType::Oracle, DatabaseType::OceanbaseOracle, DatabaseType::Dameng] { + for full_source in [false, true] { + let prefix = "CREATE VIEW \"HR\".\"ACTIVE_ORDERS\" AS\n"; + let ddl = build_view_ddl_sql(BuildViewDdlInput { + database_type: Some(database_type), + schema: Some("HR".into()), + name: "ACTIVE_ORDERS".into(), + source: if full_source { format!("{prefix}{source}") } else { source.to_string() }, + identifier_quote: None, + }); + let expected_base = format!("{prefix}{terminated}"); + assert_eq!(ddl, expected_base, "{database_type:?}: {source}"); + for table_comment in [None, Some("View's comment")] { + for columns in [&[][..], std::slice::from_ref(&column)] { + let enriched = + append_oracle_comments_to_ddl(&ddl, "HR", "ACTIVE_ORDERS", table_comment, columns); + let mut expected = expected_base.clone(); + if table_comment.is_some() { + expected.push_str("\nCOMMENT ON TABLE \"HR\".\"ACTIVE_ORDERS\" IS 'View''s comment';"); + } + if !columns.is_empty() { + expected.push_str( + "\nCOMMENT ON COLUMN \"HR\".\"ACTIVE_ORDERS\".\"ID\" IS 'Column''s comment';", + ); + } + assert_eq!(enriched, expected); + let statements = split_sql_statements_for_database(&enriched, database_type); + assert_eq!( + statements.len(), + 1 + usize::from(table_comment.is_some()) + columns.len(), + "{enriched}" + ); + assert!(!statements[0].contains("COMMENT ON"), "{enriched}"); + } + } + } + } + } + assert_eq!(append_oracle_comments_to_ddl("", "HR", "EMPTY", Some("ignored"), &[column]), ""); + } + #[test] fn does_not_duplicate_existing_oracle_comment_ddl() { let column = db::ColumnInfo { @@ -11672,12 +12124,53 @@ async fn external_driver_oracle_ddl( agent_metadata_timeout(Some(config)), ) .await?; - result + let ddl = result .get("source") .and_then(serde_json::Value::as_str) .filter(|source| !source.trim().is_empty()) .map(str::to_string) - .ok_or_else(|| "JDBC Oracle plugin returned no table DDL".to_string()) + .ok_or_else(|| "JDBC Oracle plugin returned no table DDL".to_string())?; + let schema = result + .get("schema") + .and_then(serde_json::Value::as_str) + .filter(|schema| !schema.trim().is_empty()) + .unwrap_or(schema); + + // DBMS_METADATA.GET_DDL omits dictionary comments; append COMMENT ON like the native agent path. + let columns = session + .invoke_with_timeout::>( + "getColumns", + serde_json::json!({ + "connection": config, + "database": database, + "schema": schema, + "table": table, + }), + agent_metadata_timeout(Some(config)), + ) + .await + .unwrap_or_else(|error| { + log::debug!( + "[schema][jdbc-oracle:get_table_ddl:columns-for-comments-failed] schema={} table={} error={}", + schema, + table, + error + ); + Vec::new() + }); + let table_comment = match external_driver_oracle_table_comment(session, config, database, schema, table).await { + Ok(comment) => comment, + Err(error) => { + log::debug!( + "[schema][jdbc-oracle:get_table_ddl:table-comment-failed] schema={} table={} error={}", + schema, + table, + error + ); + None + } + }; + Ok(append_oracle_comments_to_ddl(&ddl, schema, table, table_comment.as_deref(), &columns)) } /// External JDBC connections that match no vendor DDL dialect (JDBCX wrappers,