-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Add UI functionality to capture Oracle data source #1606
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
229c236
Add UI functionality to capture Oracle data source
narsik123 2058526
Update projectRepository.ts
narsik123 e9b1cbb
Removed the SSL checkbox. Need to put it back in once engine starts o…
narsik123 b1001b7
Merge branch 'Canner:main' into main
narsik123 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Submodule wren-engine
updated
907 files
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -49,6 +49,7 @@ export const typeDefs = gql` | |
| DUCKDB | ||
| POSTGRES | ||
| MYSQL | ||
| ORACLE | ||
| MSSQL | ||
| CLICK_HOUSE | ||
| TRINO | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
94 changes: 94 additions & 0 deletions
94
wren-ui/src/components/pages/setup/dataSources/OracleProperties.tsx
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,94 @@ | ||
| import { Form, Input, Switch } from 'antd'; | ||
| import { ERROR_TEXTS } from '@/utils/error'; | ||
| import { FORM_MODE } from '@/utils/enum'; | ||
| import { hostValidator } from '@/utils/validator'; | ||
|
|
||
| interface Props { | ||
| mode?: FORM_MODE; | ||
| } | ||
|
|
||
| export default function OracleProperties(props: Props) { | ||
| const { mode } = props; | ||
| const isEditMode = mode === FORM_MODE.EDIT; | ||
| return ( | ||
| <> | ||
| <Form.Item | ||
| label="Display name" | ||
| name="displayName" | ||
| required | ||
| rules={[ | ||
| { | ||
| required: true, | ||
| message: ERROR_TEXTS.CONNECTION.DISPLAY_NAME.REQUIRED, | ||
| }, | ||
| ]} | ||
| > | ||
| <Input /> | ||
| </Form.Item> | ||
| <Form.Item | ||
| label="Host" | ||
| name="host" | ||
| required | ||
| rules={[ | ||
| { | ||
| required: true, | ||
| validator: hostValidator, | ||
| }, | ||
| ]} | ||
| > | ||
| <Input placeholder="10.1.1.1" disabled={isEditMode} /> | ||
| </Form.Item> | ||
| <Form.Item | ||
| label="Port" | ||
| name="port" | ||
| required | ||
| rules={[ | ||
| { | ||
| required: true, | ||
| message: ERROR_TEXTS.CONNECTION.PORT.REQUIRED, | ||
| }, | ||
| ]} | ||
| > | ||
| <Input placeholder="1521" disabled={isEditMode} /> | ||
| </Form.Item> | ||
| <Form.Item | ||
| label="Username" | ||
| name="user" | ||
| rules={[ | ||
| { | ||
| required: true, | ||
| message: ERROR_TEXTS.CONNECTION.USERNAME.REQUIRED, | ||
| }, | ||
| ]} | ||
| > | ||
| <Input /> | ||
| </Form.Item> | ||
| <Form.Item | ||
| label="Password" | ||
| name="password" | ||
| required | ||
| rules={[ | ||
| { | ||
| required: true, | ||
| message: ERROR_TEXTS.CONNECTION.PASSWORD.REQUIRED, | ||
| }, | ||
| ]} | ||
| > | ||
| <Input.Password placeholder="input password" /> | ||
| </Form.Item> | ||
| <Form.Item | ||
| label="Database name" | ||
| name="database" | ||
| required | ||
| rules={[ | ||
| { | ||
| required: true, | ||
| message: ERROR_TEXTS.CONNECTION.DATABASE.REQUIRED, | ||
| }, | ||
| ]} | ||
| > | ||
| <Input placeholder="Oracle database name" disabled={isEditMode} /> | ||
| </Form.Item> | ||
| </> | ||
| ); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oracle implementation missing SSL handling
The implementation doesn't account for the SSL option that exists in the OracleProperties UI component. This will cause the SSL setting to be ignored when connecting to Oracle databases.
Reference the ClickHouse implementation (lines 180-199) on how to handle SSL parameters:
// Oracle [DataSourceName.ORACLE]: { sensitiveProps: ['password'], toIbisConnectionInfo(connectionInfo) { const decryptedConnectionInfo = decryptConnectionInfo( DataSourceName.ORACLE, connectionInfo, ); - const { host, port, database, user, password } = + const { host, port, database, user, password, ssl } = decryptedConnectionInfo as ORACLE_CONNECTION_INFO; - return { host, port, database, user, password }; + return { + host, + port, + database, + user, + password, + ...(ssl && { kwargs: { ssl: true } }) + }; }, } as IDataSourceConnectionInfo< ORACLE_CONNECTION_INFO, HostBasedConnectionInfo >,📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added the first comment alone. I'm not sure if ...(ssl && { kwargs: { ssl: true } }) would be necessary
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
@narsik123 I reviewed the code and can see that your OracleProperties component includes an SSL toggle switch:
Since you've added this SSL option in the UI, you should handle it in the implementation to ensure the user's selection is respected when connecting to Oracle databases.
The suggested approach using
...(ssl && { kwargs: { ssl: true } })follows the same pattern used by MS SQL Server in your codebase:This is the standard pattern in your codebase for passing optional connection parameters via the kwargs property when using host-based connections. Without this implementation, the SSL toggle in your UI won't have any effect on the actual connection.
I recommend including the SSL handling as suggested to maintain consistency with how other database types handle connection parameters.
🏁 Script executed:
Length of output: 1917
🏁 Script executed:
Length of output: 2329
@narsik123 I've found an important inconsistency that needs addressing:
Your OracleProperties component has an SSL toggle:
But the ORACLE_CONNECTION_INFO interface doesn't include an ssl property:
This means even if users toggle SSL in the UI, that setting won't be captured in the connection info. You need to:
ssl: boolean;property to the ORACLE_CONNECTION_INFO interfaceThe suggested pattern
...(ssl && { kwargs: { ssl: true } })follows how MS SQL handles its trustServerCertificate option and would be appropriate once the interface includes the ssl property.Without these changes, the SSL toggle in your UI won't actually do anything.
🧬 Code Graph Analysis Results
Relevant snippets from
wren-ui/src/apollo/server/repositories/projectRepository.ts(lines 13-82)These define the TypeScript interfaces for connection info objects used in the main file, including the
sslproperty for Postgres and ClickHouse, andtrustServerCertificatefor MS SQL:Relevant snippets from
wren-ui/src/apollo/server/adaptors/ibisAdaptor.ts(lines 30-67)These define the Ibis connection info interfaces returned by the
toIbisConnectionInfofunctions, showing the expected structure for Postgres, Trino, Snowflake, BigQuery, etc.:Summary of
wren-ui/src/apollo/server/dataSource.tsdataSourceobject keyed byDataSourceNameenum values.sensitiveProps(e.g.,password,credentials) and atoIbisConnectionInfomethod that converts the decrypted connection info into the expected Ibis connection info format.toIbisConnectionInfomethod:host,port,database,user,password, andssl.sslmode=requireifsslis true.kwargsproperty added iftrustServerCertificateis true.secure=1ifsslis true.https://ifsslis true, otherwisehttp://.(ssl && { kwargs: { ssl: true } })pattern, which is similar to the existing pattern used for MS SQL withtrustServerCertificate.Possibly relevant snippet from migration
wren-ui/migrations/20240530062809_transfer_project_table_data.js(lines 40-41)This snippet shows how the
sslflag might be set from project configurations, relevant to understanding howsslis used in connection info.Possibly relevant snippet from
wren-ui/src/apollo/server/config.ts(lines 153-155)This shows the
getConfigfunction used indataSource.tsto obtain configuration, which is then used to initialize theEncryptor.Possibly relevant snippet from
wren-ui/src/pages/api/graphql.ts(lines 24-28)This snippet is unrelated to connection info or SSL handling but is included for completeness.
[End of relevant snippets]
✏️ Learnings added