diff --git a/superset-frontend/packages/superset-ui-core/src/query/getClientErrorObject.ts b/superset-frontend/packages/superset-ui-core/src/query/getClientErrorObject.ts index 8b4b62cf76c8e..dd26aec6192d7 100644 --- a/superset-frontend/packages/superset-ui-core/src/query/getClientErrorObject.ts +++ b/superset-frontend/packages/superset-ui-core/src/query/getClientErrorObject.ts @@ -23,10 +23,12 @@ import { t, SupersetError, ErrorTypeEnum, + isProbablyHTML, + isJsonString, } from '@superset-ui/core'; -// The response always contains an error attribute, can contain anything from the -// SupersetClientResponse object, and can contain a spread JSON blob +// The response always contains an error attribute, can contain anything from +// the SupersetClientResponse object, and can contain a spread JSON blob export type ClientErrorObject = { error: string; errors?: SupersetError[]; @@ -51,8 +53,74 @@ type ErrorType = type ErrorTextSource = 'dashboard' | 'chart' | 'query' | 'dataset' | 'database'; -export function parseErrorJson(responseObject: JsonObject): ClientErrorObject { - let error = { ...responseObject }; +const ERROR_CODE_LOOKUP = { + 400: 'Bad request', + 401: 'Unauthorized', + 402: 'Payment required', + 403: 'Forbidden', + 404: 'Not found', + 405: 'Method not allowed', + 406: 'Not acceptable', + 407: 'Proxy authentication required', + 408: 'Request timeout', + 409: 'Conflict', + 410: 'Gone', + 411: 'Length required', + 412: 'Precondition failed', + 413: 'Payload too large', + 414: 'URI too long', + 415: 'Unsupported media type', + 416: 'Range not satisfiable', + 417: 'Expectation failed', + 418: "I'm a teapot", + 500: 'Server error', + 501: 'Not implemented', + 502: 'Bad gateway', + 503: 'Service unavailable', + 504: 'Gateway timeout', + 505: 'HTTP version not supported', + 506: 'Variant also negotiates', + 507: 'Insufficient storage', + 508: 'Loop detected', + 510: 'Not extended', + 511: 'Network authentication required', + 599: 'Network error', +}; + +export function checkForHtml(str: string): boolean { + return !isJsonString(str) && isProbablyHTML(str); +} + +export function parseStringResponse(str: string): string { + if (checkForHtml(str)) { + for (const [code, message] of Object.entries(ERROR_CODE_LOOKUP)) { + const regex = new RegExp(`${code}|${message}`, 'i'); + if (regex.test(str)) { + return t(message); + } + } + return t('Unknown error'); + } + return str; +} + +export function getErrorFromStatusCode(status: number): string | null { + return ERROR_CODE_LOOKUP[status] || null; +} + +export function retrieveErrorMessage( + str: string, + errorObject: JsonObject, +): string { + const statusError = + 'status' in errorObject ? getErrorFromStatusCode(errorObject.status) : null; + + // Prefer status code message over the response or HTML text + return statusError || parseStringResponse(str); +} + +export function parseErrorJson(responseJson: JsonObject): ClientErrorObject { + let error = { ...responseJson }; // Backwards compatibility for old error renderers with the new error object if (error.errors && error.errors.length > 0) { error.error = error.description = error.errors[0].message; @@ -67,7 +135,11 @@ export function parseErrorJson(responseObject: JsonObject): ClientErrorObject { t('Invalid input'); } if (typeof error.message === 'string') { - error.error = error.message; + if (checkForHtml(error.message)) { + error.error = retrieveErrorMessage(error.message, error); + } else { + error.error = error.message; + } } } if (error.stack) { @@ -95,11 +167,12 @@ export function getClientErrorObject( | { response: Response } | string, ): Promise { - // takes a SupersetClientResponse as input, attempts to read response as Json if possible, - // and returns a Promise that resolves to a plain object with error key and text value. + // takes a SupersetClientResponse as input, attempts to read response as Json + // if possible, and returns a Promise that resolves to a plain object with + // error key and text value. return new Promise(resolve => { if (typeof response === 'string') { - resolve({ error: response }); + resolve({ error: parseStringResponse(response) }); return; } @@ -149,20 +222,32 @@ export function getClientErrorObject( const responseObject = response instanceof Response ? response : response.response; + if (responseObject && !responseObject.bodyUsed) { - // attempt to read the body as json, and fallback to text. we must clone the - // response in order to fallback to .text() because Response is single-read + // attempt to read the body as json, and fallback to text. we must clone + // the response in order to fallback to .text() because Response is + // single-read responseObject .clone() .json() .then(errorJson => { - const error = { ...responseObject, ...errorJson }; + // Destructuring instead of spreading to avoid loss of sibling properties to the body + const { url, status, statusText, redirected, type } = responseObject; + const responseSummary = { url, status, statusText, redirected, type }; + const error = { + ...errorJson, + ...responseSummary, + }; resolve(parseErrorJson(error)); }) .catch(() => { // fall back to reading as text responseObject.text().then((errorText: any) => { - resolve({ ...responseObject, error: errorText }); + resolve({ + // Destructuring not necessary here + ...responseObject, + error: retrieveErrorMessage(errorText, responseObject), + }); }); }); return; @@ -177,7 +262,7 @@ export function getClientErrorObject( } resolve({ ...responseObject, - error, + error: parseStringResponse(error), }); }); } diff --git a/superset-frontend/packages/superset-ui-core/src/utils/html.test.tsx b/superset-frontend/packages/superset-ui-core/src/utils/html.test.tsx index 9b950e4246e92..f3aee1c9e7bef 100644 --- a/superset-frontend/packages/superset-ui-core/src/utils/html.test.tsx +++ b/superset-frontend/packages/superset-ui-core/src/utils/html.test.tsx @@ -23,6 +23,8 @@ import { sanitizeHtmlIfNeeded, safeHtmlSpan, removeHTMLTags, + isJsonString, + getParagraphContents, } from './html'; describe('sanitizeHtml', () => { @@ -114,3 +116,77 @@ describe('removeHTMLTags', () => { expect(output).toBe('Unclosed tag'); }); }); + +describe('isJsonString', () => { + test('valid JSON object', () => { + const jsonString = '{"name": "John", "age": 30, "city": "New York"}'; + expect(isJsonString(jsonString)).toBe(true); + }); + + test('valid JSON array', () => { + const jsonString = '[1, 2, 3, 4, 5]'; + expect(isJsonString(jsonString)).toBe(true); + }); + + test('valid JSON string', () => { + const jsonString = '"Hello, world!"'; + expect(isJsonString(jsonString)).toBe(true); + }); + + test('invalid JSON with syntax error', () => { + const jsonString = '{"name": "John", "age": 30, "city": "New York"'; + expect(isJsonString(jsonString)).toBe(false); + }); + + test('empty string', () => { + const jsonString = ''; + expect(isJsonString(jsonString)).toBe(false); + }); + + test('non-JSON string', () => { + const jsonString = '

Hello, World!

'; + expect(isJsonString(jsonString)).toBe(false); + }); + + test('non-JSON formatted number', () => { + const jsonString = '12345abc'; + expect(isJsonString(jsonString)).toBe(false); + }); +}); + +describe('getParagraphContents', () => { + test('should return an object with keys for each paragraph tag', () => { + const htmlString = + '

First paragraph.

Second paragraph.

'; + const result = getParagraphContents(htmlString); + expect(result).toEqual({ + p1: 'First paragraph.', + p2: 'Second paragraph.', + }); + }); + + test('should return null if the string is not HTML', () => { + const nonHtmlString = 'Just a plain text string.'; + expect(getParagraphContents(nonHtmlString)).toBeNull(); + }); + + test('should return null if there are no

tags in the HTML string', () => { + const htmlStringWithoutP = '

No paragraph here.
'; + expect(getParagraphContents(htmlStringWithoutP)).toBeNull(); + }); + + test('should return an object with empty string for empty

tag', () => { + const htmlStringWithEmptyP = '

'; + const result = getParagraphContents(htmlStringWithEmptyP); + expect(result).toEqual({ p1: '' }); + }); + + test('should handle HTML strings with nested

tags correctly', () => { + const htmlStringWithNestedP = + '

First paragraph with nested content.

'; + const result = getParagraphContents(htmlStringWithNestedP); + expect(result).toEqual({ + p1: 'First paragraph with nested content.', + }); + }); +}); diff --git a/superset-frontend/packages/superset-ui-core/src/utils/html.tsx b/superset-frontend/packages/superset-ui-core/src/utils/html.tsx index 95695ed5877f5..f0c59695c3baf 100644 --- a/superset-frontend/packages/superset-ui-core/src/utils/html.tsx +++ b/superset-frontend/packages/superset-ui-core/src/utils/html.tsx @@ -45,10 +45,26 @@ export function sanitizeHtml(htmlString: string) { return xssFilter.process(htmlString); } +export function hasHtmlTagPattern(str: string): boolean { + const htmlTagPattern = + /<(html|head|body|div|span|a|p|h[1-6]|title|meta|link|script|style)/i; + + return htmlTagPattern.test(str); +} + export function isProbablyHTML(text: string) { - return Array.from( - new DOMParser().parseFromString(text, 'text/html').body.childNodes, - ).some(({ nodeType }) => nodeType === 1); + const cleanedStr = text.trim().toLowerCase(); + + if ( + cleanedStr.startsWith('') && + hasHtmlTagPattern(cleanedStr) + ) { + return true; + } + + const parser = new DOMParser(); + const doc = parser.parseFromString(cleanedStr, 'text/html'); + return Array.from(doc.body.childNodes).some(({ nodeType }) => nodeType === 1); } export function sanitizeHtmlIfNeeded(htmlString: string) { @@ -71,3 +87,36 @@ export function safeHtmlSpan(possiblyHtmlString: string) { export function removeHTMLTags(str: string): string { return str.replace(/<[^>]*>/g, ''); } + +export function isJsonString(str: string): boolean { + try { + JSON.parse(str); + return true; + } catch (e) { + return false; + } +} + +export function getParagraphContents( + str: string, +): { [key: string]: string } | null { + if (!isProbablyHTML(str)) { + return null; + } + + const parser = new DOMParser(); + const doc = parser.parseFromString(str, 'text/html'); + const pTags = doc.querySelectorAll('p'); + + if (pTags.length === 0) { + return null; + } + + const paragraphContents: { [key: string]: string } = {}; + + pTags.forEach((pTag, index) => { + paragraphContents[`p${index + 1}`] = pTag.textContent || ''; + }); + + return paragraphContents; +} diff --git a/superset-frontend/packages/superset-ui-core/test/query/getClientErrorObject.test.ts b/superset-frontend/packages/superset-ui-core/test/query/getClientErrorObject.test.ts index c37e6fbe93c0b..1abbf5c3065f9 100644 --- a/superset-frontend/packages/superset-ui-core/test/query/getClientErrorObject.test.ts +++ b/superset-frontend/packages/superset-ui-core/test/query/getClientErrorObject.test.ts @@ -25,19 +25,59 @@ import { ErrorTypeEnum, } from '@superset-ui/core'; -it('Returns a Promise', () => { +test('Returns a Promise', () => { const response = getClientErrorObject('error'); expect(response instanceof Promise).toBe(true); }); -it('Returns a Promise that resolves to an object with an error key', async () => { +test('Returns a Promise that resolves to an object with an error key', async () => { const error = 'error'; const errorObj = await getClientErrorObject(error); expect(errorObj).toMatchObject({ error }); }); -it('Handles Response that can be parsed as json', async () => { +test('should handle HTML response with "500" or "server error"', async () => { + const htmlString500 = '
500: Internal Server Error
'; + const clientErrorObject500 = await getClientErrorObject(htmlString500); + expect(clientErrorObject500).toEqual({ error: 'Server error' }); + + const htmlStringServerError = '
Server error message
'; + const clientErrorObjectServerError = await getClientErrorObject( + htmlStringServerError, + ); + expect(clientErrorObjectServerError).toEqual({ + error: 'Server error', + }); +}); + +test('should handle HTML response with "404" or "not found"', async () => { + const htmlString404 = '
404: Page not found
'; + const clientErrorObject404 = await getClientErrorObject(htmlString404); + expect(clientErrorObject404).toEqual({ error: 'Not found' }); + + const htmlStringNotFoundError = '
Not found message
'; + const clientErrorObjectNotFoundError = await getClientErrorObject( + htmlStringNotFoundError, + ); + expect(clientErrorObjectNotFoundError).toEqual({ + error: 'Not found', + }); +}); + +test('should handle HTML response without common error code', async () => { + const htmlString = '
Foo bar Lorem Ipsum
'; + const clientErrorObject = await getClientErrorObject(htmlString); + expect(clientErrorObject).toEqual({ error: 'Unknown error' }); + + const htmlString2 = '

An error occurred

'; + const clientErrorObject2 = await getClientErrorObject(htmlString2); + expect(clientErrorObject2).toEqual({ + error: 'Unknown error', + }); +}); + +test('Handles Response that can be parsed as json', async () => { const jsonError = { something: 'something', error: 'Error message' }; const jsonErrorString = JSON.stringify(jsonError); @@ -45,7 +85,7 @@ it('Handles Response that can be parsed as json', async () => { expect(errorObj).toMatchObject(jsonError); }); -it('Handles backwards compatibility between old error messages and the new SIP-40 errors format', async () => { +test('Handles backwards compatibility between old error messages and the new SIP-40 errors format', async () => { const jsonError = { errors: [ { @@ -63,14 +103,21 @@ it('Handles backwards compatibility between old error messages and the new SIP-4 expect(errorObj.link).toEqual(jsonError.errors[0].extra.link); }); -it('Handles Response that can be parsed as text', async () => { +test('Handles Response that can be parsed as text', async () => { + const textError = 'Hello I am a text error'; + + const errorObj = await getClientErrorObject(new Response(textError)); + expect(errorObj).toMatchObject({ error: textError }); +}); + +test('Handles Response that contains raw html be parsed as text', async () => { const textError = 'Hello I am a text error'; const errorObj = await getClientErrorObject(new Response(textError)); expect(errorObj).toMatchObject({ error: textError }); }); -it('Handles TypeError Response', async () => { +test('Handles TypeError Response', async () => { const error = new TypeError('Failed to fetch'); // @ts-ignore @@ -78,7 +125,7 @@ it('Handles TypeError Response', async () => { expect(errorObj).toMatchObject({ error: 'Network error' }); }); -it('Handles timeout error', async () => { +test('Handles timeout error', async () => { const errorObj = await getClientErrorObject({ timeout: 1000, statusText: 'timeout', @@ -110,14 +157,30 @@ it('Handles timeout error', async () => { }); }); -it('Handles plain text as input', async () => { +test('Handles plain text as input', async () => { const error = 'error'; const errorObj = await getClientErrorObject(error); expect(errorObj).toMatchObject({ error }); }); -it('Handles error with status text and message', async () => { +test('Handles error with status code', async () => { + const status500 = new Response(null, { status: 500 }); + const status404 = new Response(null, { status: 404 }); + const status502 = new Response(null, { status: 502 }); + + expect(await getClientErrorObject(status500)).toMatchObject({ + error: 'Server error', + }); + expect(await getClientErrorObject(status404)).toMatchObject({ + error: 'Not found', + }); + expect(await getClientErrorObject(status502)).toMatchObject({ + error: 'Bad gateway', + }); +}); + +test('Handles error with status text and message', async () => { const statusText = 'status'; const message = 'message'; @@ -135,7 +198,7 @@ it('Handles error with status text and message', async () => { }); }); -it('getClientErrorMessage', () => { +test('getClientErrorMessage', () => { expect(getClientErrorMessage('error')).toEqual('error'); expect( getClientErrorMessage('error', { @@ -150,7 +213,7 @@ it('getClientErrorMessage', () => { ).toEqual('error:\nclient error'); }); -it('parseErrorJson with message', () => { +test('parseErrorJson with message', () => { expect(parseErrorJson({ message: 'error message' })).toEqual({ message: 'error message', error: 'error message', @@ -181,7 +244,49 @@ it('parseErrorJson with message', () => { }); }); -it('parseErrorJson with stacktrace', () => { +test('parseErrorJson with HTML message', () => { + expect( + parseErrorJson({ + message: '
error message
', + }), + ).toEqual({ + message: '
error message
', + error: 'Unknown error', + }); + expect( + parseErrorJson({ + message: '
Server error
', + }), + ).toEqual({ + message: '
Server error
', + error: 'Server error', + }); +}); + +test('parseErrorJson with HTML message and status code', () => { + expect( + parseErrorJson({ + status: 502, + message: '
error message
', + }), + ).toEqual({ + status: 502, + message: '
error message
', + error: 'Bad gateway', + }); + expect( + parseErrorJson({ + status: 999, + message: '
Server error
', + }), + ).toEqual({ + status: 999, + message: '
Server error
', + error: 'Server error', + }); +}); + +test('parseErrorJson with stacktrace', () => { expect( parseErrorJson({ error: 'error message', stack: 'stacktrace' }), ).toEqual({ @@ -204,7 +309,7 @@ it('parseErrorJson with stacktrace', () => { }); }); -it('parseErrorJson with CSRF', () => { +test('parseErrorJson with CSRF', () => { expect( parseErrorJson({ responseText: 'CSRF', @@ -215,7 +320,7 @@ it('parseErrorJson with CSRF', () => { }); }); -it('getErrorText', async () => { +test('getErrorText', async () => { expect(await getErrorText('error', 'dashboard')).toEqual( 'Sorry, there was an error saving this dashboard: error', );