Skip to content
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

chore: bubble up more db error messages #21982

Merged
merged 2 commits into from
Nov 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions superset-frontend/src/components/DatabaseSelector/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ import Label from 'src/components/Label';
import { FormLabel } from 'src/components/Form';
import RefreshLabel from 'src/components/RefreshLabel';
import { useToasts } from 'src/components/MessageToasts/withToasts';
import {
getClientErrorMessage,
getClientErrorObject,
} from 'src/utils/getClientErrorObject';

const DatabaseSelectorWrapper = styled.div`
${({ theme }) => `
Expand Down Expand Up @@ -238,9 +242,16 @@ export default function DatabaseSelector({
setLoadingSchemas(false);
if (refresh > 0) addSuccessToast('List refreshed');
})
.catch(() => {
.catch(err => {
setLoadingSchemas(false);
handleError(t('There was an error loading the schemas'));
getClientErrorObject(err).then(clientError => {
handleError(
getClientErrorMessage(
t('There was an error loading the schemas'),
clientError,
),
);
});
});
}
}, [currentDb, onSchemasLoad, refresh]);
Expand Down
15 changes: 14 additions & 1 deletion superset-frontend/src/components/TableSelector/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,10 @@ import WarningIconWithTooltip from 'src/components/WarningIconWithTooltip';
import { useToasts } from 'src/components/MessageToasts/withToasts';
import { SchemaOption } from 'src/SqlLab/types';
import { useTables, Table } from 'src/hooks/apiResources';
import {
getClientErrorMessage,
getClientErrorObject,
} from 'src/utils/getClientErrorObject';

const REFRESH_WIDTH = 30;

Expand Down Expand Up @@ -187,7 +191,16 @@ const TableSelector: FunctionComponent<TableSelectorProps> = ({
addSuccessToast(t('List updated'));
}
},
onError: () => handleError(t('There was an error loading the tables')),
onError: (err: Response) => {
getClientErrorObject(err).then(clientError => {
handleError(
getClientErrorMessage(
t('There was an error loading the tables'),
clientError,
),
);
});
},
});

const tableOptions = useMemo<TableOption[]>(
Expand Down
15 changes: 9 additions & 6 deletions superset-frontend/src/explore/ExplorePage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -42,21 +42,24 @@ import { fallbackExploreInitialData } from './fixtures';
import { getItem, LocalStorageKeys } from '../utils/localStorageHelpers';
import { getFormDataWithDashboardContext } from './controlUtils/getFormDataWithDashboardContext';

const isResult = (rv: JsonObject): rv is ExploreResponsePayload =>
rv?.result?.form_data &&
rv?.result?.dataset &&
isDefined(rv?.result?.dataset?.id);
const isValidResult = (rv: JsonObject): boolean =>
rv?.result?.form_data && isDefined(rv?.result?.dataset?.id);

const fetchExploreData = async (exploreUrlParams: URLSearchParams) => {
try {
const rv = await makeApi<{}, ExploreResponsePayload>({
method: 'GET',
endpoint: 'api/v1/explore/',
})(exploreUrlParams);
if (isResult(rv)) {
if (isValidResult(rv)) {
return rv;
}
throw new Error(t('Failed to load chart data.'));
let message = t('Failed to load chart data');
const responseError = rv?.result?.message;
if (responseError) {
message = `${message}:\n${responseError}`;
}
throw new Error(message);
} catch (err) {
// todo: encapsulate the error handler
const clientError = await getClientErrorObject(err);
Expand Down
12 changes: 12 additions & 0 deletions superset-frontend/src/utils/getClientErrorObject.ts
Original file line number Diff line number Diff line change
Expand Up @@ -171,3 +171,15 @@ export function getClientErrorObject(
});
});
}

export function getClientErrorMessage(
message: string,
clientError?: ClientErrorObject,
) {
let finalMessage = message;
const errorMessage = clientError?.message || clientError?.error;
if (errorMessage) {
finalMessage = `${finalMessage}:\n${errorMessage}`;
}
return finalMessage;
}
9 changes: 8 additions & 1 deletion superset/databases/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@
from superset.databases.utils import get_table_metadata
from superset.db_engine_specs import get_available_engine_specs
from superset.errors import ErrorLevel, SupersetError, SupersetErrorType
from superset.exceptions import SupersetErrorsException
from superset.exceptions import SupersetErrorsException, SupersetException
from superset.extensions import security_manager
from superset.models.core import Database
from superset.superset_typing import FlaskResponse
Expand Down Expand Up @@ -293,6 +293,8 @@ def post(self) -> FlaskResponse:
exc_info=True,
)
return self.response_422(message=str(ex))
except SupersetException as ex:
return self.response(ex.status, message=ex.message)

@expose("/<int:pk>", methods=["PUT"])
@protect()
Expand Down Expand Up @@ -486,6 +488,8 @@ def schemas(self, pk: int, **kwargs: Any) -> FlaskResponse:
return self.response(
500, message="There was an error connecting to the database"
)
except SupersetException as ex:
return self.response(ex.status, message=ex.message)

@expose("/<int:pk>/table/<table_name>/<schema_name>/", methods=["GET"])
@protect()
Expand Down Expand Up @@ -544,6 +548,9 @@ def table_metadata(
except SQLAlchemyError as ex:
self.incr_stats("error", self.table_metadata.__name__)
return self.response_422(error_msg_from_exception(ex))
except SupersetException as ex:
return self.response(ex.status, message=ex.message)

self.incr_stats("success", self.table_metadata.__name__)
return self.response(200, **table_info)

Expand Down
16 changes: 12 additions & 4 deletions superset/db_engine_specs/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -639,7 +639,7 @@ def fetch_data(
return cursor.fetchmany(limit)
return cursor.fetchall()
except Exception as ex:
raise cls.get_dbapi_mapped_exception(ex)
raise cls.get_dbapi_mapped_exception(ex) from ex

@classmethod
def expand_data(
Expand Down Expand Up @@ -1025,7 +1025,11 @@ def get_table_names( # pylint: disable=unused-argument
:param schema: Schema to inspect. If omitted, uses default schema for database
:return: All tables in schema
"""
tables = inspector.get_table_names(schema)
try:
tables = inspector.get_table_names(schema)
except Exception as ex:
raise cls.get_dbapi_mapped_exception(ex) from ex

if schema and cls.try_remove_schema_from_table_name:
tables = [re.sub(f"^{schema}\\.", "", table) for table in tables]
return sorted(tables)
Expand All @@ -1045,7 +1049,11 @@ def get_view_names( # pylint: disable=unused-argument
:param schema: Schema name. If omitted, uses default schema for database
:return: All views in schema
"""
views = inspector.get_view_names(schema)
try:
views = inspector.get_view_names(schema)
except Exception as ex:
raise cls.get_dbapi_mapped_exception(ex) from ex

if schema and cls.try_remove_schema_from_table_name:
views = [re.sub(f"^{schema}\\.", "", view) for view in views]
return sorted(views)
Expand Down Expand Up @@ -1326,7 +1334,7 @@ def execute( # pylint: disable=unused-argument
try:
cursor.execute(query)
except Exception as ex:
raise cls.get_dbapi_mapped_exception(ex)
raise cls.get_dbapi_mapped_exception(ex) from ex

@classmethod
def make_label_compatible(cls, label: str) -> Union[str, quoted_name]:
Expand Down
4 changes: 2 additions & 2 deletions superset/db_engine_specs/bigquery.py
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
from superset.databases.schemas import encrypted_field_properties, EncryptedString
from superset.databases.utils import make_url_safe
from superset.db_engine_specs.base import BaseEngineSpec, BasicPropertiesType
from superset.db_engine_specs.exceptions import SupersetDBAPIDisconnectionError
from superset.db_engine_specs.exceptions import SupersetDBAPIConnectionError
from superset.errors import SupersetError, SupersetErrorType
from superset.sql_parse import Table
from superset.utils import core as utils
Expand Down Expand Up @@ -449,7 +449,7 @@ def get_dbapi_exception_mapping(cls) -> Dict[Type[Exception], Type[Exception]]:
# pylint: disable=import-outside-toplevel
from google.auth.exceptions import DefaultCredentialsError

return {DefaultCredentialsError: SupersetDBAPIDisconnectionError}
return {DefaultCredentialsError: SupersetDBAPIConnectionError}

@classmethod
def validate_parameters(
Expand Down
12 changes: 11 additions & 1 deletion superset/db_engine_specs/druid.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,14 @@
import json
import logging
from datetime import datetime
from typing import Any, Dict, List, Optional, TYPE_CHECKING
from typing import Any, Dict, List, Optional, Type, TYPE_CHECKING

from sqlalchemy import types
from sqlalchemy.engine.reflection import Inspector

from superset import is_feature_enabled
from superset.db_engine_specs.base import BaseEngineSpec
from superset.db_engine_specs.exceptions import SupersetDBAPIConnectionError
from superset.exceptions import SupersetException
from superset.utils import core as utils

Expand Down Expand Up @@ -136,3 +137,12 @@ def get_columns(
type_map["complex<hllsketch>"] = types.BLOB

return super().get_columns(inspector, table_name, schema)

@classmethod
def get_dbapi_exception_mapping(cls) -> Dict[Type[Exception], Type[Exception]]:
# pylint: disable=import-error,import-outside-toplevel
from requests import exceptions as requests_exceptions

return {
requests_exceptions.ConnectionError: SupersetDBAPIConnectionError,
}
2 changes: 1 addition & 1 deletion superset/db_engine_specs/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ class SupersetDBAPIDatabaseError(SupersetDBAPIError):
pass


class SupersetDBAPIDisconnectionError(SupersetDBAPIError):
class SupersetDBAPIConnectionError(SupersetDBAPIError):
pass


Expand Down
12 changes: 11 additions & 1 deletion superset/db_engine_specs/trino.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
from __future__ import annotations

import logging
from typing import Any, Dict, Optional, TYPE_CHECKING
from typing import Any, Dict, Optional, Type, TYPE_CHECKING

import simplejson as json
from flask import current_app
Expand All @@ -27,6 +27,7 @@
from superset.constants import USER_AGENT
from superset.databases.utils import make_url_safe
from superset.db_engine_specs.base import BaseEngineSpec
from superset.db_engine_specs.exceptions import SupersetDBAPIConnectionError
from superset.db_engine_specs.presto import PrestoBaseEngineSpec
from superset.models.sql_lab import Query
from superset.utils import core as utils
Expand Down Expand Up @@ -220,3 +221,12 @@ def update_params_from_encrypted_extra(
except json.JSONDecodeError as ex:
logger.error(ex, exc_info=True)
raise ex

@classmethod
def get_dbapi_exception_mapping(cls) -> Dict[Type[Exception], Type[Exception]]:
# pylint: disable=import-error,import-outside-toplevel
from requests import exceptions as requests_exceptions

return {
requests_exceptions.ConnectionError: SupersetDBAPIConnectionError,
}
12 changes: 8 additions & 4 deletions superset/explore/commands/get.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,17 +144,21 @@ def run(self) -> Optional[Dict[str, Any]]:
utils.merge_extra_filters(form_data)
utils.merge_request_params(form_data, request.args)

dummy_dataset_data: Dict[str, Any] = {
# TODO: this is a dummy placeholder - should be refactored to being just `None`
dataset_data: Dict[str, Any] = {
"type": self._dataset_type,
"name": dataset_name,
"columns": [],
"metrics": [],
"database": {"id": 0, "backend": ""},
}
try:
dataset_data = dataset.data if dataset else dummy_dataset_data
except (SupersetException, SQLAlchemyError):
dataset_data = dummy_dataset_data
if dataset:
dataset_data = dataset.data
except SupersetException as ex:
message = ex.message
except SQLAlchemyError:
message = "SQLAlchemy error"

metadata = None

Expand Down
15 changes: 8 additions & 7 deletions superset/models/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -563,9 +563,8 @@ def get_all_table_names_in_schema( # pylint: disable=unused-argument
database=self, inspector=self.inspector, schema=schema
)
return [(table, schema) for table in tables]
except Exception: # pylint: disable=broad-except
logger.warning("Get all table names in schema failed", exc_info=True)
return []
except Exception as ex: # pylint: disable=broad-except
raise self.db_engine_spec.get_dbapi_mapped_exception(ex)

@cache_util.memoized_func(
key="db:{self.id}:schema:{schema}:view_list",
Expand Down Expand Up @@ -594,9 +593,8 @@ def get_all_view_names_in_schema( # pylint: disable=unused-argument
database=self, inspector=self.inspector, schema=schema
)
return [(view, schema) for view in views]
except Exception: # pylint: disable=broad-except
logger.warning("Get all view names failed", exc_info=True)
return []
except Exception as ex: # pylint: disable=broad-except
raise self.db_engine_spec.get_dbapi_mapped_exception(ex)

@cache_util.memoized_func(
key="db:{self.id}:schema_list",
Expand All @@ -618,7 +616,10 @@ def get_all_schema_names( # pylint: disable=unused-argument
:param force: whether to force refresh the cache
:return: schema list
"""
return self.db_engine_spec.get_schema_names(self.inspector)
try:
return self.db_engine_spec.get_schema_names(self.inspector)
except Exception as ex:
raise self.db_engine_spec.get_dbapi_mapped_exception(ex) from ex

@property
def db_engine_spec(self) -> Type[db_engine_specs.BaseEngineSpec]:
Expand Down
Loading