fix(oracle): remove trailing semicolons from prebuilt tools and clean…#3215
fix(oracle): remove trailing semicolons from prebuilt tools and clean…#3215Deeven-Seru wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes trailing semicolons from SQL statements in the Oracle prebuilt configurations and deletes debug print statements from the Oracle SQL tool. The review feedback highlights discrepancies between the tool descriptions and their SQL implementations in oracledb.yaml, specifically noting missing columns and hardcoded parameters. Furthermore, a suggestion was made to implement structured logging in oraclesql.go instead of simply removing the debug output to preserve visibility for troubleshooting.
…or parameter binding
Deeven-Seru
force-pushed
the
fix-3181-oracle-prebuilt
branch
from
08d6bf0 to
2809af2
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request enhances the Oracle database tools by adding filtering and dynamic sorting capabilities to prebuilt SQL queries, such as table listing and top SQL analysis. It also replaces standard output debugging with structured logging in the Go implementation. Feedback focuses on correcting documentation discrepancies regarding query scope, improving the robustness of SQL string filtering to handle case and whitespace, and enriching log messages with specific tool names and parameters for better observability.
| parameters: | ||
| - name: table_names_csv | ||
| type: string | ||
| description: "Optional: A comma-separated list of table names. If empty, details for all tables in user-accessible schemas will be listed." |
There was a problem hiding this comment.
The parameter description for table_names_csv states that it lists tables in "user-accessible schemas", which typically refers to the ALL_TABLES view. However, the query on line 41 uses USER_TABLES, which is restricted to the current user's schema. This discrepancy should be resolved by correcting the description to reflect that it only lists tables in the current schema, matching the tool's overall description.
| t.last_analyzed | ||
| FROM user_tables t | ||
| LEFT JOIN user_segments s ON t.table_name = s.segment_name AND s.segment_type IN ('TABLE', 'TABLE PARTITION', 'TABLE SUBPARTITION') | ||
| WHERE (:1 IS NULL OR INSTR(',' || :1 || ',', ',' || t.table_name || ',') > 0) |
There was a problem hiding this comment.
The INSTR logic for filtering by a comma-separated list is sensitive to spaces and case. If a user provides TABLE1, TABLE2, the match for TABLE2 will fail due to the leading space in the parameter string. Additionally, Oracle table names are usually uppercase. It is recommended to make the filter more robust by stripping spaces and converting to uppercase for comparison.
WHERE (:1 IS NULL OR INSTR(',' || UPPER(REPLACE(:1, ' ', '')) || ',', ',' || UPPER(t.table_name) || ',') > 0)| return nil, util.NewClientServerError("error getting logger", http.StatusInternalServerError, err) | ||
| } | ||
| fmt.Printf("\n") | ||
| logger.DebugContext(ctx, "executing %s tool query: %s", resourceType, newStatement) |
There was a problem hiding this comment.
Logging the resourceType ("oracle-sql") is helpful, but logging the specific tool name (t.Name) would provide much better context when multiple Oracle tools are being used. Also, including the parameter values in the debug log would greatly assist in troubleshooting query execution issues.
| logger.DebugContext(ctx, "executing %s tool query: %s", resourceType, newStatement) | |
| logger.DebugContext(ctx, "executing tool %q (%s) query: %s with params: %v", t.Name, resourceType, newStatement, sliceParams) |
2 participants
Fixes #3181
Summary
Oracle prebuilt tools defined in
internal/prebuiltconfigs/tools/oracledb.yamlwere failing with:when invoked via the
oracle-sqltool type.Root cause: the prebuilt SQL statements in the YAML included trailing semicolons. When passed as full SQL
strings through the driver, those semicolons cause Oracle to reject the statement as not properly ended.
This PR removes the trailing semicolons from the prebuilt SQL and cleans up a leftover debug print in the
oracle-sqltool implementation.