Add UI functionality to capture Oracle data source #1606
There are no files selected for viewing
| 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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,97 @@ | ||
| 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> | ||
| <Form.Item label="Use SSL" name="ssl" valuePropName="checked"> | ||
| <Switch /> | ||
| </Form.Item> | ||
|
Comment on lines
+92
to
+94
There was a problem hiding this comment. SSL field is not defined in the ORACLE_CONNECTION_INFO interface This form field collects SSL configuration, but the corresponding interface in projectRepository.ts doesn't have an SSL field to store this value. Ensure the SSL toggle configuration is properly stored by adding the ssl field to the ORACLE_CONNECTION_INFO interface. There was a problem hiding this comment. Added the SSL field to Oracle There was a problem hiding this comment. 🧩 Analysis chain@narsik123 Thank you for addressing the issue! I've verified that you've added the SSL field to the ORACLE_CONNECTION_INFO interface, which ensures the SSL configuration from the UI form will be properly stored. This completes the implementation for Oracle data source support with proper SSL configuration handling. 🐰 happy hopping 🏁 Script executed: Length of output: 320 @narsik123 I checked the ORACLE_CONNECTION_INFO interface in projectRepository.ts, but I don't see the SSL field there yet. The interface still only contains: export interface ORACLE_CONNECTION_INFO {
host: string;
port: number;
user: string;
password: string;
database: string;
}Your OracleProperties.tsx component includes an SSL toggle, but this value isn't being stored without the corresponding field in the interface. Could you add 🏁 Script executed: Length of output: 66 @narsik123 I've been trying to locate where the SSL field might have been added for Oracle connections, but I can't find any evidence of this change in the ORACLE_CONNECTION_INFO interface. The form component in OracleProperties.tsx includes an SSL toggle switch: <Form.Item label="Use SSL" name="ssl" valuePropName="checked">
<Switch />
</Form.Item>However, the corresponding interface still doesn't have an SSL field: export interface ORACLE_CONNECTION_INFO {
host: string;
port: number;
user: string;
password: string;
database: string;
}For the form's SSL toggle to work properly, you'll need to add 🏁 Script executed: Length of output: 501 @narsik123 I've searched through the codebase but couldn't find evidence that the SSL field was added to the ORACLE_CONNECTION_INFO interface. The OracleProperties component has a form field for SSL: <Form.Item label="Use SSL" name="ssl" valuePropName="checked">
<Switch />
</Form.Item>But the corresponding interface in projectRepository.ts doesn't have a matching field: export interface ORACLE_CONNECTION_INFO {
host: string;
port: number;
user: string;
password: string;
database: string;
}To properly store the SSL configuration, you'll need to add 🧬 Code Graph Analysis ResultsRelevant snippets from
|
||
| </> | ||
| ); | ||
| } | ||
|
Comment on lines
+1
to
+97
There was a problem hiding this comment. 🛠️ Refactor suggestion Missing test file for the OracleProperties component To ensure reliable functionality, a test file should be created to validate the component's behavior. Create a test file for this component to verify:
Would you like me to generate a test file template for this component? |
||
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