feat(wren-ui): Added FE Support for MySQL SSL Connection #1072
Conversation
WalkthroughThe pull request introduces comprehensive SSL configuration support for MySQL data sources across the Wren UI application. The changes span multiple files, adding new enumerations for SSL modes, updating connection information interfaces, and enhancing the MySQL properties component to support SSL certificate uploads. The modifications enable users to configure different SSL modes (DISABLED, ENABLED, VERIFY_CA) and upload SSL certificates when needed. Changes
Assessment against linked issues
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Hi, I’m exploring ways to allow users to select a file from their local system and display its absolute file path, instead of typing the path into the input box themselves. I’ve looked into the Ant Design |
|
hi @ongdisheng any reason you close this PR ? looks like a good contribution to me! I'm thinking we should provide some options for users to pick instead of a SSL Mode
If users choose cc @fredalai @andreashimin @onlyjackfrost chime in if you guys want. I think you might want to work on some implementation on backend as well. |
|
Hi @wwwy3y3, thank you for the feedback! I closed the PR earlier as I felt the current implementation wasn't quite up to the mark. I’ve reopened it as a draft and will continue working on it 😊. |
ongdisheng
force-pushed
the
issue-886
branch
2 times, most recently
from
0b6a2d3 to
1ea9289
Compare
ongdisheng
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
wren-ui/src/apollo/server/types/index.ts (1)
7-7: Leverage strongly-typed imports for maintainability.Exporting everything from
./sslModeis a valid approach for quick bundling. However, if the file grows, consider explicitly re-exporting only what's needed to maintain clarity and ensure only intended types or utilities are exposed.wren-ui/src/utils/error/dictionary.ts (1)
52-54: Provide additional clarity for SSL certificate requirements.The new
SSL_CERT.REQUIREDmessage is straightforward. To further assist users, consider including details on valid file formats or maximum file sizes (if applicable). This can reduce confusion about acceptable certificate files during upload.wren-ui/src/components/pages/setup/dataSources/MySQLProperties.tsx (2)
13-63: Consider improving file upload error handling and applying optional chaining.
TheUploadSSLcomponent is well-structured, but there are points you might refine:
- Potential file-read failures: The
readFileContentfunction does not handle errors. Consider addingreader.onerrorto handle read failures gracefully.- Optional chaining: Instead of using
onChange && onChange(...)at lines 41 and 49, you can use optional chaining (onChange?.(...)) in TypeScript/modern JS for cleaner code.- File size or type checks: If large or invalid files are a concern, consider validating them in the
onUploadChangeevent.Below is a suggested diff snippet for optional chaining:
- onChange && onChange(fileContent); + onChange?.(fileContent);- onChange && onChange(undefined); + onChange?.(undefined);🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 49-49: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
149-176: Enhance user guidance and default behaviors for SSL mode.
- User feedback: When users select a mode other than
DISABLE, consider giving tooltips or descriptions to clarify whatREQUIREvs.VERIFY_CAdoes.- Conditional display: The conditional rendering for
sslCAis correct but ensure it matches any server constraints (e.g., do you also need a client certificate or key?).- Preview uploaded file: If needed, allow users to confirm they uploaded the correct SSL certificate by showing a truncated snippet of file content or the file name.
Would you like help implementing additional user feedback or advanced upload validations?
wren-ui/src/apollo/server/dataSource.ts (1)
111-125: Verify base64 encoding and SSL property usage.
This block correctly includessslModeand optionally base64-encodessslCAbefore returning. A few considerations:
- Condition checks: If you add more SSL-related fields (e.g.,
sslKeyorsslCert) in the future, ensure consistent logic for each field.- Validate usage: Confirm that downstream code expects a base64 string for
sslCA.wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts (2)
48-49: Clarify SSL usage in test data.
You’ve specifiedsslMode: SSLMode.VERIFY_CAandsslCA: 'encrypted-certificate-string'. Make sure tests also cover other modes for completeness (e.g.,SSLMode.DISABLE,SSLMode.REQUIRE).
191-202: Check for consistent property naming and transformations.
Here, you mapsslCAtosslCA(base64-encoded). For uniformity, confirm that other SSL props (e.g.,sslKey) follow a similar approach to keep your code symmetrical.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts(3 hunks)wren-ui/src/apollo/server/dataSource.ts(1 hunks)wren-ui/src/apollo/server/repositories/projectRepository.ts(1 hunks)wren-ui/src/apollo/server/types/index.ts(1 hunks)wren-ui/src/apollo/server/types/sslMode.ts(1 hunks)wren-ui/src/components/pages/setup/dataSources/MySQLProperties.tsx(2 hunks)wren-ui/src/utils/enum/index.ts(1 hunks)wren-ui/src/utils/enum/sslMode.ts(1 hunks)wren-ui/src/utils/error/dictionary.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- wren-ui/src/apollo/server/types/sslMode.ts
- wren-ui/src/utils/enum/sslMode.ts
🧰 Additional context used
🪛 Biome (1.9.4)
wren-ui/src/components/pages/setup/dataSources/MySQLProperties.tsx
[error] 41-41: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 49-49: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
wren-ui/src/utils/enum/index.ts (1)
10-10: Centralize enum usage.Similarly, re-exporting all from
sslModemakes the enum easily accessible throughout the codebase. As the codebase evolves, ensure repeated or conflicting enum definitions are consolidated to maintain consistency.wren-ui/src/components/pages/setup/dataSources/MySQLProperties.tsx (2)
1-6: Ensure consistent import usage and check for unused imports.
These imports look fine for the newly introduced features, such asUploadand Ant Design icons. Verify that each imported item (e.g.,Button,Select) is actually used in this file.
68-69: Ensure SSL mode state updates are fully validated.
ThesslModestate andonSSLModeChangefunction are straightforward. Double-check that all form validations (e.g., required certificates on certain modes) are handled or triggered consistently in your parent form logic.wren-ui/src/apollo/server/dataSource.ts (1)
105-105: Add an extra safeguard for sensitive properties.
By addingsslCAtosensitiveProps, you ensure that it will be encrypted at rest. Confirm that your logs or error outputs do not inadvertently displaysslCAvalues in plaintext.wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts (2)
9-9: Consolidate or differentiate imports as needed.
The newSSLModeimport is straightforward. If multiple modules import the same enumerations, ensure they reference a shared location for consistency.
181-184: Consider verifying decryption fails gracefully ifsslCAis invalid.
Currently, you mock decryption to always succeed with ansslCAstring. Test how your code behaves if decryption fails or the certificate data is malformed.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
wren-ui/src/components/pages/setup/dataSources/MySQLProperties.tsx (4)
18-20: Prevent potential memory leaks in useEffectThe
useEffecthook should clean up the file list state when the component unmounts.useEffect(() => { if (!value) setFileList([]); + return () => setFileList([]); }, [value]);
36-50: Improve error handling and use optional chainingThe callback handling in
onUploadChangeandonRemovecould be improved with optional chaining and better error handling.const onUploadChange = (info) => { const { file, fileList } = info; if (fileList.length) { const uploadFile = fileList[0]; + if (!file.originFileObj) { + console.error('File object is missing'); + return; + } readFileContent(file.originFileObj, (fileContent: string) => { - onChange && onChange(fileContent); + onChange?.(fileContent); }); setFileList([uploadFile]); } }; const onRemove = () => { setFileList([]); - onChange && onChange(undefined); + onChange?.(undefined); };🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 49-49: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
53-62: Enhance file type validation and user feedbackThe current file type validation could be improved with more comprehensive checks and better user feedback.
<Upload accept=".pem,.crt,.key" fileList={fileList} onChange={onUploadChange} onRemove={onRemove} maxCount={1} + beforeUpload={(file) => { + const isValidType = ['.pem', '.crt', '.key'].some(ext => + file.name.toLowerCase().endsWith(ext) + ); + if (!isValidType) { + message.error('Please upload a valid SSL certificate file'); + } + return isValidType || Upload.LIST_IGNORE; + }} > <Button icon={<UploadOutlined />}>Click to upload SSL cert file</Button> </Upload>
68-69: Consider deriving SSL mode from Form.ItemInstead of maintaining a separate state for SSL mode, consider using Form's internal state management.
- const [sslMode, setSSLMode] = useState<string>(SSL_MODE.DISABLED); - const onSSLModeChange = (value: string) => setSSLMode(value) + const [form] = Form.useForm(); + const sslMode = Form.useWatch('sslMode', form) || SSL_MODE.DISABLED;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
wren-ui/src/apollo/server/types/sslMode.ts(1 hunks)wren-ui/src/components/pages/setup/dataSources/MySQLProperties.tsx(2 hunks)wren-ui/src/utils/enum/sslMode.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
wren-ui/src/components/pages/setup/dataSources/MySQLProperties.tsx
[error] 41-41: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 49-49: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
wren-ui/src/utils/enum/sslMode.ts (1)
1-5: Duplicate enum addressed in previous comment
| export enum SSLMode { | ||
| DISABLED = 'disabled', | ||
| ENABLED = 'enabled', | ||
| VERIFY_CA = 'verify_ca', | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating duplicate SSL mode enums
There appears to be duplicate SSL mode enums in both apollo/server/types/sslMode.ts and utils/enum/sslMode.ts. Consider consolidating these into a single shared enum to maintain a single source of truth.
I can help propose a solution to consolidate these enums if you'd like.
| const UploadSSL = (props) => { | ||
| const { onChange, value } = props; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add TypeScript interface for component props
The UploadSSL component lacks a proper TypeScript interface for its props. This could lead to type-related issues.
Add a props interface:
interface UploadSSLProps {
onChange?: (value: string | undefined) => void;
value?: string;
}
const UploadSSL = (props: UploadSSLProps) => {| <Form.Item label="SSL mode" name="sslMode" initialValue={SSL_MODE.DISABLED}> | ||
| <Select | ||
| style={{ width: 120 }} | ||
| onChange={onSSLModeChange} | ||
| disabled={isEditMode} | ||
| options={[ | ||
| { value: SSL_MODE.DISABLED, label: 'Disabled' }, | ||
| { value: SSL_MODE.ENABLED, label: 'Enabled' }, | ||
| { value: SSL_MODE.VERIFY_CA, label: 'Verify CA' }, | ||
| ]} | ||
| /> | ||
| </Form.Item> | ||
| { | ||
| sslMode === SSL_MODE.VERIFY_CA && | ||
| <Form.Item | ||
| label="SSL CA file" | ||
| name="sslCA" | ||
| required | ||
| rules={[ | ||
| { | ||
| required: true, | ||
| message: ERROR_TEXTS.CONNECTION.SSL_CERT.REQUIRED, | ||
| }, | ||
| ]} | ||
| > | ||
| <UploadSSL /> | ||
| </Form.Item> | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
SSL configuration edit mode behavior needs to be consistent
The verification confirms that while SSL mode selection is disabled in edit mode, the SSL certificate upload remains enabled, creating an inconsistent user experience. The suggested changes in the review comment would improve both consistency and user guidance:
- Disable both SSL mode selection and certificate upload in edit mode
- Add tooltips to explain the disabled state
- Keep the validation rules for when the form is in create mode
🔗 Analysis chain
Improve edit mode handling for SSL configuration
The current implementation disables SSL mode selection in edit mode but allows certificate changes. Consider making this behavior consistent.
Also, consider adding a tooltip to explain why SSL mode is disabled in edit mode.
<Form.Item label="SSL mode" name="sslMode" initialValue={SSL_MODE.DISABLED}>
<Select
style={{ width: 120 }}
- onChange={onSSLModeChange}
disabled={isEditMode}
+ tooltip={isEditMode ? "SSL mode cannot be changed after connection is created" : undefined}
options={[
{ value: SSL_MODE.DISABLED, label: 'Disabled' },
{ value: SSL_MODE.ENABLED, label: 'Enabled' },
{ value: SSL_MODE.VERIFY_CA, label: 'Verify CA' },
]}
/>
</Form.Item>
{
sslMode === SSL_MODE.VERIFY_CA &&
<Form.Item
label="SSL CA file"
name="sslCA"
required
+ tooltip="Upload a valid CA certificate file (.pem, .crt, or .key)"
rules={[
{
required: true,
message: ERROR_TEXTS.CONNECTION.SSL_CERT.REQUIRED,
},
]}
>
- <UploadSSL />
+ <UploadSSL disabled={isEditMode} />
</Form.Item>
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there are any existing implementations of edit mode handling for SSL
rg -A 5 "mode === FORM_MODE.EDIT.*ssl" .
Length of output: 42
Script:
#!/bin/bash
# Find form mode edit usages and SSL-related code
rg "FORM_MODE.EDIT" .
echo "---"
fd -e tsx -e ts "UploadSSL" .
echo "---"
rg "const.*SSL" .
Length of output: 1973
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
wren-ui/src/components/pages/setup/dataSources/MySQLProperties.tsx (4)
9-11: Add documentation for the Props interfaceConsider adding JSDoc comments to document the Props interface, especially explaining the significance of the
modeparameter and its impact on form behavior./** * Props for MySQLProperties component * @interface Props * @property {FORM_MODE} [mode] - Form mode that determines if fields are editable */ interface Props { mode?: FORM_MODE; }
22-34: Improve file reading implementationThe file reading implementation could be enhanced for better error handling and type safety.
- const readFileContent = (file: any, callback: (value: string) => void) => { + const readFileContent = (file: File, callback: (value: string) => void) => { const reader = new FileReader(); + reader.onerror = () => { + console.error('Error reading file:', reader.error); + }; reader.onloadend = (_e) => { const result = reader.result; - if (result) { + if (typeof result === 'string') { - const fileContent = String(result); - callback(fileContent); + callback(result); } }; reader.readAsText(file); };
36-50: Use optional chaining and improve type safetyAddress the static analysis suggestions and improve type safety.
- const onUploadChange = (info) => { + const onUploadChange = (info: { file: UploadFile; fileList: UploadFile[] }) => { const { file, fileList } = info; if (fileList.length) { const uploadFile = fileList[0]; readFileContent(file.originFileObj, (fileContent: string) => { - onChange && onChange(fileContent); + onChange?.(fileContent); }); setFileList([uploadFile]); } }; const onRemove = () => { setFileList([]); - onChange && onChange(undefined); + onChange?.(undefined); };🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 49-49: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
149-160: Enhance SSL mode selection UXAdd tooltips to explain different SSL modes and why they might be disabled in edit mode.
<Form.Item label="SSL mode" name="sslMode" initialValue={SSL_MODE.DISABLED}> <Select style={{ width: 120 }} onChange={onSSLModeChange} disabled={isEditMode} + tooltip={isEditMode ? "SSL mode cannot be changed after connection is created" : undefined} options={[ - { value: SSL_MODE.DISABLED, label: 'Disabled' }, - { value: SSL_MODE.ENABLED, label: 'Enabled' }, - { value: SSL_MODE.VERIFY_CA, label: 'Verify CA' }, + { + value: SSL_MODE.DISABLED, + label: 'Disabled', + title: 'No SSL encryption for connection' + }, + { + value: SSL_MODE.ENABLED, + label: 'Enabled', + title: 'Use SSL encryption without certificate verification' + }, + { + value: SSL_MODE.VERIFY_CA, + label: 'Verify CA', + title: 'Use SSL encryption with certificate verification' + }, ]} /> </Form.Item>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts(3 hunks)wren-ui/src/apollo/server/dataSource.ts(1 hunks)wren-ui/src/apollo/server/repositories/projectRepository.ts(2 hunks)wren-ui/src/apollo/server/types/index.ts(1 hunks)wren-ui/src/apollo/server/types/sslMode.ts(1 hunks)wren-ui/src/components/pages/setup/dataSources/MySQLProperties.tsx(2 hunks)wren-ui/src/utils/enum/index.ts(1 hunks)wren-ui/src/utils/enum/sslMode.ts(1 hunks)wren-ui/src/utils/error/dictionary.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- wren-ui/src/utils/enum/index.ts
- wren-ui/src/apollo/server/types/index.ts
- wren-ui/src/apollo/server/types/sslMode.ts
- wren-ui/src/utils/enum/sslMode.ts
- wren-ui/src/utils/error/dictionary.ts
- wren-ui/src/apollo/server/repositories/projectRepository.ts
- wren-ui/src/apollo/server/adaptors/tests/ibisAdaptor.test.ts
- wren-ui/src/apollo/server/dataSource.ts
🧰 Additional context used
🪛 Biome (1.9.4)
wren-ui/src/components/pages/setup/dataSources/MySQLProperties.tsx
[error] 41-41: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 49-49: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
wren-ui/src/components/pages/setup/dataSources/MySQLProperties.tsx (2)
13-14: Add TypeScript interface for UploadSSL propsThe component lacks proper type definitions for its props.
161-176: 🛠️ Refactor suggestionImprove SSL certificate upload validation and consistency
Add file type validation and maintain consistency with edit mode behavior.
{ sslMode === SSL_MODE.VERIFY_CA && <Form.Item label="SSL CA file" name="sslCA" required + tooltip="Upload a valid CA certificate file (.pem, .crt, or .key)" rules={[ { required: true, message: ERROR_TEXTS.CONNECTION.SSL_CERT.REQUIRED, }, + { + validator: async (_rule, value) => { + if (value && !value.match(/^-----BEGIN CERTIFICATE-----/)) { + throw new Error('Invalid certificate format'); + } + }, + }, ]} > - <UploadSSL /> + <UploadSSL disabled={isEditMode} /> </Form.Item> }Likely invalid or redundant comment.
Description
This PR introduces SSL connection handling for
MySQLdata source by adding aSelectmenu and anUploadcomponent for Certificate Authority (CA) Certificate in the setup form. Meanwhile, existing logic has also been modified to include these fields when passing connection info to the engine. Fix #886UI Screenshot
VERIFY_CA), the connection succeeds:Summary by CodeRabbit
New Features
Improvements
Documentation