fix(Oracle DB Node): Resolve SQL compatibility issues and correct outBind generation #21489
Conversation
n8n-assistant
bot
added
community
|
Hey @sudarshan12s, Thank you for your contribution. We appreciate the time and effort you’ve taken to submit this pull request. Before we can proceed, please ensure the following: Regarding new nodes: If your node integrates with an AI service that you own or represent, please email [email protected] and we will be happy to discuss the best approach. About review timelines: Thank you again for contributing to n8n. |
| rollback: jest.fn(), | ||
| }; | ||
|
|
||
| Object.defineProperty(fakeConnection, 'oracleServerVersion', { |
There was a problem hiding this comment.
Perhaps call the variable mockConnection instead of fakeConnection
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues found across 6 files
Prompt for AI agents (all 4 issues)
Understand the root cause of the following 4 issues and fix them.
<file name="packages/nodes-base/nodes/Oracle/Sql/helpers/utils.ts">
<violation number="1" location="packages/nodes-base/nodes/Oracle/Sql/helpers/utils.ts:848">
Null BLOB bind values now trigger a UserError. Allow nulls to pass through so existing workflows can still write NULL into BLOB columns.</violation>
<violation number="2" location="packages/nodes-base/nodes/Oracle/Sql/helpers/utils.ts:856">
Rule violated: **Prefer Typeguards over Type casting**
Avoid the `(bindVal as any)` assertions here and introduce a proper type guard (e.g. `isSerializedBuffer`) so the Buffer shape is validated without bypassing the type system.
(Based on your team's feedback about replacing `as` casts around Buffer handling with runtime guards.) [FEEDBACK_USED]</violation>
</file>
<file name="packages/nodes-base/nodes/Oracle/Sql/helpers/interfaces.ts">
<violation number="1" location="packages/nodes-base/nodes/Oracle/Sql/helpers/interfaces.ts:132">
Please type CHAR_LENGTH as number | null since ALL_TAB_COLUMNS.CHAR_LENGTH is NULL for non-character columns; keeping it as number can lead callers to assume it’s always numeric and skip null checks.</violation>
</file>
<file name="packages/nodes-base/credentials/OracleDBApi.credentials.ts">
<violation number="1" location="packages/nodes-base/credentials/OracleDBApi.credentials.ts:17">
Rule violated: **Prefer Typeguards over Type casting**
Casting the `oracledb` module to `any` to access privilege constants suppresses its declared typings and violates the "Prefer Typeguards over Type casting" rule; please rely on typed keys (e.g., via `satisfies`/`as const`) so the lookup stays type-safe.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| case 'blob': | ||
| bindVal = item.valueBlob; | ||
| break; | ||
| if (Buffer.isBuffer(bindVal)) { |
There was a problem hiding this comment.
Null BLOB bind values now trigger a UserError. Allow nulls to pass through so existing workflows can still write NULL into BLOB columns.
Prompt for AI agents
Address the following comment on packages/nodes-base/nodes/Oracle/Sql/helpers/utils.ts at line 848:
<comment>Null BLOB bind values now trigger a UserError. Allow nulls to pass through so existing workflows can still write NULL into BLOB columns.</comment>
<file context>
@@ -795,7 +845,23 @@ export function getBindParameters(
case 'blob':
bindVal = item.valueBlob;
- break;
+ if (Buffer.isBuffer(bindVal)) {
+ break;
+ }
</file context>
✅ Addressed in 9a7ddfb
There was a problem hiding this comment.
Added NULL check
| COLUMN_NAME: string; | ||
| DATA_TYPE: string; | ||
| DATA_LENGTH: number; | ||
| CHAR_LENGTH: number; |
There was a problem hiding this comment.
Please type CHAR_LENGTH as number | null since ALL_TAB_COLUMNS.CHAR_LENGTH is NULL for non-character columns; keeping it as number can lead callers to assume it’s always numeric and skip null checks.
Prompt for AI agents
Address the following comment on packages/nodes-base/nodes/Oracle/Sql/helpers/interfaces.ts at line 132:
<comment>Please type CHAR_LENGTH as number | null since ALL_TAB_COLUMNS.CHAR_LENGTH is NULL for non-character columns; keeping it as number can lead callers to assume it’s always numeric and skip null checks.</comment>
<file context>
@@ -122,3 +123,16 @@ export type ExecuteOpBindParam =
+ COLUMN_NAME: string;
+ DATA_TYPE: string;
+ DATA_LENGTH: number;
+ CHAR_LENGTH: number;
+ DEFAULT_LENGTH: number | null;
+ NULLABLE: 'Y' | 'N';
</file context>
| CHAR_LENGTH: number; | |
| CHAR_LENGTH: number | null; |
There was a problem hiding this comment.
The CHAR_LENGTH will come as 0 if its not valid like for number type.
There was a problem hiding this comment.
Thanks for the feedback! I've saved this as a new learning to improve future reviews.
|
|
||
| const privilegeOptions = privilegeKeys.map((key) => ({ | ||
| name: key, | ||
| value: (oracledb as any)[key], |
There was a problem hiding this comment.
Rule violated: Prefer Typeguards over Type casting
Casting the oracledb module to any to access privilege constants suppresses its declared typings and violates the "Prefer Typeguards over Type casting" rule; please rely on typed keys (e.g., via satisfies/as const) so the lookup stays type-safe.
Prompt for AI agents
Address the following comment on packages/nodes-base/credentials/OracleDBApi.credentials.ts at line 17:
<comment>Casting the `oracledb` module to `any` to access privilege constants suppresses its declared typings and violates the "Prefer Typeguards over Type casting" rule; please rely on typed keys (e.g., via `satisfies`/`as const`) so the lookup stays type-safe.</comment>
<file context>
@@ -1,4 +1,21 @@
+
+const privilegeOptions = privilegeKeys.map((key) => ({
+ name: key,
+ value: (oracledb as any)[key],
+}));
</file context>
There was a problem hiding this comment.
The map is static and keys are always exposed by oracledb module.
| if ( | ||
| bindVal && | ||
| typeof bindVal === 'object' && | ||
| (bindVal as any).type === 'Buffer' && |
There was a problem hiding this comment.
Prompt for AI agents
~~~ Address the following comment on packages/nodes-base/nodes/Oracle/Sql/helpers/utils.ts at line 856: Avoid the `(bindVal as any)` assertions here and introduce a proper type guard (e.g. `isSerializedBuffer`) so the Buffer shape is validated without bypassing the type system. (Based on your team's feedback about replacing `as` casts around Buffer handling with runtime guards.) @@ -795,7 +845,23 @@ export function getBindParameters( + if ( + bindVal && + typeof bindVal === 'object' && + (bindVal as any).type === 'Buffer' && + Array.isArray((bindVal as any).data) + ) { ~~~
There was a problem hiding this comment.
Added isSerializedBuffer to check the type.
| displayName: 'Privilege', | ||
| name: 'privilege', | ||
| type: 'options', | ||
| description: 'The privilege to use when establishing connection to the database', |
There was a problem hiding this comment.
either "a connection" or "connections". Or change "establishing connection" to "connecting"
There was a problem hiding this comment.
changed to The privilege to use when connecting to the database
| break; | ||
| } | ||
| throw new UserError( | ||
| 'BLOB data must be valid Buffer or \'{ type: "Buffer", data: [...] }\'', |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
|
|
||
| const conn = await pool.getConnection(); | ||
| const isCDBSupported = conn.oracleServerVersion >= 1200000000; | ||
| await conn.close(); |
There was a problem hiding this comment.
Bug: Connection leak from unclosed database connection
Connection resource leak: The database connection is acquired but not protected by try-finally. If an error occurs after getConnection() on line 90 but before close() on line 92 (e.g., if accessing conn.oracleServerVersion throws an error), the connection will not be closed and will leak. This should be wrapped in a try-finally block to ensure the connection is always closed.
There was a problem hiding this comment.
oracleServerVersion is a public exported attribute exposed from oracledb module which is always available. It should not cause an exception.
sudarshan12s
changed the title
| // Allow null or undefined to represent SQL NULL BLOB values | ||
| if (bindVal === null) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
Bug: Standardize BLOB Null Value Checks
The comment states both null and undefined should represent SQL NULL BLOB values, but the code only checks for null. When bindVal is undefined, it bypasses the null check and eventually throws an error. The condition should be if (bindVal === null || bindVal === undefined) to match the comment's stated intent and handle optional BLOB values consistently.
|
@RomanDavydchuk Request your help with this review |
3 participants
Summary
Avoid
IDENTITY_COLUMNinall_tab_columnstable andFETCH FIRSTsql syntax on Oracle Database versions, where CDB support is not there.Fix outBinds handling in Execute node UI which chooses to return outbinds as objects. — Handle the outbinds returned as object and populate them correctly.
Add privilege login option — expose privilege options like
SYSDBA, SYSPRELIM, SYSOPER, ...for connections that require elevated privileges .Allow BLOB data as Serialized form of Buffer (
{ type: 'Buffer', data: [...] }) in Execute node UIRelated Linear tickets, Github issues, and Community forum posts
Review / Merge checklist
release/backport(if the PR is an urgent fix that needs to be backported)Note
Adds privilege-based login, fixes SQL/version compatibility (FETCH FIRST/ROWNUM, metadata queries), and normalizes outBinds incl. BLOB buffers in Oracle node.
FETCH FIRSTandROWNUM, and use version-appropriate column metadata queries (with/withoutIDENTITY_COLUMN).selectto build an inner query before applying WHERE/ORDER BY/LIMIT.outBindsreturned as objects; support BLOB binds from Buffers or serialized{ type: 'Buffer', data: [...] }.privilegeoption tooracleDBApicredentials and pass through in transport config.TableColumnRowtype; extendOracleDBNodeCredentialswithprivilege.Written by Cursor Bugbot for commit f17991e. This will update automatically on new commits. Configure here.