UN-2081 [FIX] Surface underlying library errors for database destination connectors in the UI #1734
Conversation
kirtimanmishrazipstack
requested review from
chandrasekharan-zipstack and
muhammad-ali-e
and removed request for
chandrasekharan-zipstack
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughIntroduced a private helper to compose exception details and set the composed message on Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
kirtimanmishrazipstack
requested review from
chandrasekharan-zipstack,
harini-venkataraman,
jaseemjaskp and
muhammad-ali-e
and removed request for
muhammad-ali-e
![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 (2)
unstract/connectors/tests/databases/test_bigquery_db.py (1)
15-182: Consider extracting common test setup to reduce duplication.The four test methods follow an identical pattern for setup (BigQuery instance creation, mocking engine/query job, patching get_information_schema). While the tests are comprehensive and correctly validate the error handling, extracting common setup logic to a helper method or
setUpmethod would improve maintainability.♻️ Example refactor to reduce duplication
def _setup_bigquery_with_mock_error(self, mock_error): """Helper to create BigQuery instance and mock engine with error.""" bigquery = BigQuery( { "json_credentials": ( '{"type":"service_account","project_id":"test_project"}' ) } ) mock_engine = MagicMock() mock_query_job = MagicMock() mock_engine.query.return_value = mock_query_job mock_query_job.result.side_effect = mock_error return bigquery, mock_engine def _execute_with_expected_exception(self, bigquery, mock_engine, exception_class): """Helper to execute query with mocked schema and return raised exception.""" with patch.object(bigquery, "get_information_schema", return_value={}): with self.assertRaises(exception_class) as context: bigquery.execute_query( engine=mock_engine, sql_query="INSERT INTO test.dataset.table VALUES (@col)", table_name="test.dataset.table", sql_values={"col": "value"}, sql_keys=["col"], ) return context.exceptionThen each test can be simplified to focus on the specific error scenario and assertions.
unstract/connectors/src/unstract/connectors/databases/exceptions.py (1)
9-21: Message composition logic is correct and well-designed.The refactored base class cleanly handles message composition by:
- Accepting both
detail(library error) anddefault_detail(user-friendly context)- Conditionally including "Details:" section only when detail is non-empty
- Using newline for better readability in multi-line error messages
The implementation correctly surfaces underlying library errors while maintaining user-friendly context, which aligns with the PR objective.
Minor naming suggestion: Consider renaming the
self.detailattribute toself.messageorself.composed_messageto clarify it contains the final composed message rather than just the raw detail parameter. This would reduce potential confusion for future maintainers.
📜 Review details
Configuration used: Organization 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)
unstract/connectors/src/unstract/connectors/databases/exceptions.pyunstract/connectors/tests/databases/test_bigquery_db.py
🧰 Additional context used
🧬 Code graph analysis (2)
unstract/connectors/tests/databases/test_bigquery_db.py (2)
unstract/connectors/src/unstract/connectors/databases/bigquery/bigquery.py (1)
BigQuery(27-395)unstract/connectors/src/unstract/connectors/databases/exceptions.py (2)
BigQueryForbiddenException(74-80)BigQueryNotFoundException(83-89)
unstract/connectors/src/unstract/connectors/databases/exceptions.py (1)
unstract/connectors/src/unstract/connectors/exceptions.py (1)
user_message(14-15)
⏰ 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 (1)
unstract/connectors/src/unstract/connectors/databases/exceptions.py (1)
23-112: Consistent refactoring across all exception subclasses.All exception subclasses correctly follow the new pattern:
- Compute their specific
default_detailmessage- Pass both
detailanddefault_detailto the base class- No longer perform local message composition
This ensures consistent error message formatting across all database connector exceptions.
unstract/connectors/src/unstract/connectors/databases/exceptions.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unstract/connectors/tests/databases/test_bigquery_db.py (1)
25-52: Consider using a list forsql_valuesto match the actual API.The helper method is well-structured, but line 48 passes
sql_valuesas a dict{"col": "value"}. Looking at the BigQueryexecute_queryimplementation,sql_valuesshould be an iterable of values (not a dict) since it's zipped withsql_keys:for key, value in zip(sql_keys, sql_values, strict=False).While this doesn't break the tests (the mock error is raised before parameter processing), it's better practice to use test data that matches the actual API.
♻️ Suggested fix
with self.assertRaises(expected_exception) as context: self.bigquery.execute_query( engine=mock_engine, sql_query="INSERT INTO test.dataset.table VALUES (@col)", table_name="test.dataset.table", - sql_values={"col": "value"}, + sql_values=["value"], sql_keys=["col"], )
📜 Review details
Configuration used: Organization 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)
unstract/connectors/src/unstract/connectors/databases/exceptions.pyunstract/connectors/tests/databases/test_bigquery_db.py
🧰 Additional context used
🧬 Code graph analysis (1)
unstract/connectors/tests/databases/test_bigquery_db.py (2)
unstract/connectors/src/unstract/connectors/databases/bigquery/bigquery.py (1)
BigQuery(27-395)unstract/connectors/src/unstract/connectors/databases/exceptions.py (2)
BigQueryForbiddenException(73-80)BigQueryNotFoundException(83-90)
⏰ 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 (7)
unstract/connectors/tests/databases/test_bigquery_db.py (4)
1-10: LGTM!The imports are appropriate for testing BigQuery error handling with mocked Google API exceptions.
15-23: LGTM!The setUp method properly initializes the BigQuery instance with test credentials, reducing duplication across test methods.
54-115: LGTM!The test methods comprehensively cover different error scenarios (billing, permissions, and resource not found). The assertions properly verify that both default messages and actual error details are included in the exception.
116-132: LGTM!Good edge case coverage! This test ensures that when the error detail is empty, the formatted message doesn't include an empty "Details:" section, maintaining clean error messages for users.
unstract/connectors/src/unstract/connectors/databases/exceptions.py (3)
9-13: LGTM!Adding
self.detail = user_messageat line 13 properly exposes the formatted error message as an attribute, enabling the UI to surface detailed error information to users. This directly addresses the PR objective.
22-23: LGTM!All exception classes have been consistently updated to use the
_format_exception_detailhelper. The pattern of calling the helper to createfinal_detailand passing it to the base class ensures uniform error message formatting across all database connector exceptions.Also applies to: 29-30, 39-40, 49-50, 58-59, 69-70, 79-80, 89-90, 107-108, 114-115
118-131: LGTM!The
_format_exception_detailhelper function correctly centralizes error message formatting. The truthiness check onactual_detailensures that empty details don't result in a dangling "Details:" label, which is properly validated by the unit tests.
Test ResultsSummary
Runner Tests - Full Report
SDK1 Tests - Full Report
|
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unstract/connectors/src/unstract/connectors/databases/exceptions.py (1)
118-129: Consider moving this helper function earlier in the file.While functionally correct, defining
_format_exception_detailafter all the classes that use it is unconventional. Moving it to the top of the file (e.g., right after the imports, around line 5) would improve readability by introducing the helper before its first usage.♻️ Suggested placement
Move the function definition to appear after the imports (around line 5):
from typing import Any from unstract.connectors.exceptions import ConnectorBaseException def _format_exception_detail(base_error: str, library_error: Any) -> str: """Format exception detail by combining base error with library-specific details. Args: base_error: The base error message describing the error type library_error: The actual error detail from the database library Returns: Formatted error message combining both if library_error exists, otherwise just the base_error """ return f"{base_error}\nDetails: {library_error}" if library_error else base_error class UnstractDBConnectorException(ConnectorBaseException): # ... rest of the classes
📜 Review details
Configuration used: Organization 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)
unstract/connectors/src/unstract/connectors/databases/exceptions.py
⏰ 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)
unstract/connectors/src/unstract/connectors/databases/exceptions.py (2)
9-13: LGTM! Good addition of thedetailattribute.The base class correctly exposes the formatted error message via the
detailattribute, making it accessible to callers who need to display library-specific error information.
22-23: Excellent consistency across all exception subclasses.All exception classes now uniformly use the
_format_exception_detailhelper to compose error messages, ensuring library errors are consistently surfaced to users.Also applies to: 29-30, 39-40, 49-50, 58-59, 69-70, 79-80, 89-90, 107-108, 114-115
kirtimanmishrazipstack
deleted the
UN-2081-Improve-error-message-for-destination-connector
branch
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.