UN-2261 [FIX] Removed broken Oracle connector option from frontend #1663
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughImport reorganization in frontend components consolidates debounce imports. Backend introduces a centralized import helper Changes
Sequence DiagramsequenceDiagram
participant OldFlow as Old Import Flow
participant NewFlow as New Import Flow
rect rgb(245, 245, 245)
Note over OldFlow: Direct Import with Try/Except
OldFlow->>OldFlow: try: import RedisQueue<br/>except ImportError: skip
end
rect rgb(240, 250, 240)
Note over NewFlow: Centralized Import Helper
NewFlow->>NewFlow: call import_optional_connector<br/>(module_path, class_name)
NewFlow->>NewFlow: try: import and return class<br/>except: return None gracefully
NewFlow->>NewFlow: RedisQueue, OracleDB imported<br/>via helper
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/components/connectors/connector-list-modal/ConnectorListModal.jsx (1)
27-30: Oracle filtering logic is correct but duplicated; consider extracting to a shared utility.The filtering logic correctly excludes the "OracleDB" connector by matching the uppercase name against "ORACLE". However, this same logic is duplicated in
ListOfSources.jsx(lines 25-30).Since this is marked as temporary, extracting it to a shared utility function would improve maintainability:
// utils/connectorFilters.js export const shouldExcludeOracle = (item) => { return item?.name?.toUpperCase().includes("ORACLE"); };Then use in both
ConnectorListModal.jsxandListOfSources.jsx:- // Filter out Oracle connector temporarily - filtered = filtered.filter((connector) => { - return !connector?.name?.toUpperCase().includes("ORACLE"); - }); + // Filter out Oracle connector temporarily + filtered = filtered.filter((connector) => !shouldExcludeOracle(connector));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
frontend/src/components/connectors/connector-list-modal/ConnectorListModal.jsx(2 hunks)frontend/src/components/input-output/list-of-sources/ListOfSources.jsx(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
frontend/src/components/connectors/connector-list-modal/ConnectorListModal.jsx (1)
2-3: LGTM: Import reorganization and debounce addition.The import reorganization is clean, and the
debounceimport is correctly used on line 24 to wrap thefilterConnectorsfunction.frontend/src/components/input-output/list-of-sources/ListOfSources.jsx (2)
3-3: LGTM: Debounce import is correctly used.The
debounceimport is properly utilized on line 51 to wrap thehandleInputChangefunction.
25-30: Oracle filtering logic is correct with appropriate conditional.The filtering logic correctly excludes Oracle connectors temporarily, and the
isConnectorcondition appropriately limits this filter to connector-type sources only.Note: This logic is duplicated from
ConnectorListModal.jsx(lines 27-30), as noted in the previous file review. The suggestion to extract this into a shared utility function applies here as well.
kirtimanmishrazipstack
requested review from
chandrasekharan-zipstack,
jagadeeswaran-zipstack and
vishnuszipstack
and removed request for
chandrasekharan-zipstack and
vishnuszipstack
frontend/src/components/connectors/connector-list-modal/ConnectorListModal.jsx
Outdated
Show resolved
Hide resolved
…ve-oracle-db-support-temparary
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/connector_processor/connector_processor.py (1)
27-36: Consider restructuring the try-except and reviewing log verbosity.The static analysis tool suggests moving the success return (line 33) to an
elseblock, which would make the control flow more explicit:def import_optional_connector(module_path: str, class_name: str): """Import connector class with graceful error handling.""" try: module = __import__(module_path, fromlist=[class_name]) connector_class = getattr(module, class_name) - logger.info(f"Successfully imported {class_name}") - return connector_class except ImportError as e: logger.warning(f"Failed to import {class_name}: {e}") return None + else: + logger.info(f"Successfully imported {class_name}") + return connector_classAdditionally, consider whether INFO-level logging for every successful connector import is necessary, as this may create noise in production logs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (4)
.gitignore(1 hunks)backend/connector_processor/connector_processor.py(2 hunks)frontend/src/components/connectors/connector-list-modal/ConnectorListModal.jsx(1 hunks)frontend/src/components/input-output/list-of-sources/ListOfSources.jsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/components/input-output/list-of-sources/ListOfSources.jsx
🧰 Additional context used
🪛 Ruff (0.14.5)
backend/connector_processor/connector_processor.py
33-33: Consider moving this statement to an else block
(TRY300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
frontend/src/components/connectors/connector-list-modal/ConnectorListModal.jsx (1)
2-3: These import changes appear unrelated to the PR objective.The changes in this file only reorder and consolidate imports without any functional modifications to connector filtering. The PR objective states "Removed broken Oracle connector option from frontend," but no Oracle-specific logic is present here. The actual Oracle removal is implemented in the backend file (
connector_processor.py), which aligns with the previous reviewer's suggestion to handle this on the backend.backend/connector_processor/connector_processor.py (1)
94-98: Implementation correctly addresses the PR objective.The approach of adding OracleDB to the
unsupported_connectorslist successfully prevents it from being exposed to the frontend while gracefully handling import failures. This aligns with the previous reviewer's suggestion to handle Oracle exclusion in the backend.Based on past review comments.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
backend/connector_processor/connector_processor.py (1)
27-36: Add return type hint and catchAttributeError.The function should include a return type hint for better type safety. Additionally, catching only
ImportErrorwon't handle the case where the module imports successfully but the specified class doesn't exist, which would raiseAttributeError.Apply this diff:
-def import_optional_connector(module_path: str, class_name: str): +def import_optional_connector(module_path: str, class_name: str) -> type | None: """Import connector class with graceful error handling.""" try: module = __import__(module_path, fromlist=[class_name]) connector_class = getattr(module, class_name) logger.info(f"Successfully imported {class_name}") return connector_class - except ImportError as e: + except (ImportError, AttributeError) as e: logger.warning(f"Failed to import {class_name}: {e}") return None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
backend/connector_processor/connector_processor.py(2 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
backend/connector_processor/connector_processor.py
[warning] 41-41: Complete the task associated to this "TODO" comment.
🪛 Ruff (0.14.5)
backend/connector_processor/connector_processor.py
33-33: Consider moving this statement to an else block
(TRY300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
backend/connector_processor/connector_processor.py (2)
39-44: LGTM! TODO comment appropriately addresses past feedback.The TODO comment clearly explains why Oracle is temporarily excluded and references the relevant issue (UN-2261). This addresses the previous reviewer's request for an explanatory comment.
Note: The SonarCloud warning about completing the TODO is expected and serves as a tracking mechanism for when Oracle wallet support is implemented.
Based on past review comments.
97-99: LGTM! Correct handling of optional connectors.The use of
filter(None, [RedisQueue, OracleDB])correctly removes anyNonevalues before callingget_id(), ensuring that connectors which failed to import don't cause runtime errors. This effectively excludes Oracle from the supported connectors list as intended.
…om:Zipstack/unstract into UN-2261-remove-oracle-db-support-temparary
kirtimanmishrazipstack
requested review from
chandrasekharan-zipstack and
ritwik-g
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
backend/connector_processor/connector_processor.py(2 hunks)
🧰 Additional context used
🪛 GitHub Check: SonarCloud Code Analysis
backend/connector_processor/connector_processor.py
[warning] 41-41: Complete the task associated to this "TODO" comment.
🪛 Ruff (0.14.5)
backend/connector_processor/connector_processor.py
33-33: Consider moving this statement to an else block
(TRY300)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
backend/connector_processor/connector_processor.py (2)
98-98: LGTM! Defensive handling of optional connectors.The use of
filter(None, [RedisQueue, OracleDB])properly handles the case where either connector fails to import and returnsNone. This ensures only successfully imported connectors are added to the unsupported list.
40-44: None handling is properly implemented viafilter(None, ...).The only usage of
RedisQueueandOracleDBwithinconnector_processor.pyoccurs at line 98, wherefilter(None, [RedisQueue, OracleDB])explicitly filters out anyNonevalues before calling.get_id()on each connector. This is correct and safe. Additionally, these variables are not exported or referenced by other modules, so there are no external None handling concerns.
| def import_optional_connector(module_path: str, class_name: str): | ||
| """Import connector class with graceful error handling.""" | ||
| try: | ||
| module = __import__(module_path, fromlist=[class_name]) | ||
| connector_class = getattr(module, class_name) | ||
| logger.debug(f"Successfully imported {class_name}") | ||
| return connector_class | ||
| except ImportError as e: | ||
| logger.debug(f"Failed to import {class_name}: {e}") | ||
| return None |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Prefer importlib.import_module and improve error handling.
The current implementation has a few areas for improvement:
__import__is lower-level and less maintainable thanimportlib.import_module- Should catch
AttributeErrorin case the class doesn't exist in the module - Following the Ruff suggestion, using
try/except/elseis clearer than logging before return
Apply this diff:
+import importlib
+
def import_optional_connector(module_path: str, class_name: str):
"""Import connector class with graceful error handling."""
try:
- module = __import__(module_path, fromlist=[class_name])
+ module = importlib.import_module(module_path)
connector_class = getattr(module, class_name)
+ except (ImportError, AttributeError) as e:
- logger.debug(f"Successfully imported {class_name}")
- return connector_class
- except ImportError as e:
logger.debug(f"Failed to import {class_name}: {e}")
return None
+ else:
+ logger.debug(f"Successfully imported {class_name}")
+ return connector_class🧰 Tools
🪛 Ruff (0.14.5)
33-33: Consider moving this statement to an else block
(TRY300)
🤖 Prompt for AI Agents
In backend/connector_processor/connector_processor.py around lines 27 to 36,
replace the low-level __import__ usage and current error handling with
importlib.import_module and a clearer try/except/else structure: import the
module via importlib.import_module(module_path) inside a try block, catch
ImportError and AttributeError separately, log appropriate debug messages
including the exception details, and in the else block retrieve the class via
getattr(module, class_name) and return it; return None only from the except
blocks so successful imports are handled in the else branch.
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
What
Why
How
Can this PR break any existing features. If yes, please list possible items. If no, please explain why. (PS: Admins do not merge the PR without this section filled)
Database Migrations
Env Config
Relevant Docs
Related Issues or PRs
Dependencies Versions
Notes on Testing
Screenshots
Checklist
I have read and understood the Contribution Guidelines.