From 1f554c57346a2dc7dbf72f440e9d8d3d2710953e Mon Sep 17 00:00:00 2001 From: David Russell Date: Wed, 4 Dec 2024 17:34:50 +0000 Subject: [PATCH] migrate notifications to gql status objects --- .../Stream/CypherFrame/WarningsView.test.tsx | 97 ++++++++- .../Stream/CypherFrame/WarningsView.tsx | 67 +++--- .../__snapshots__/WarningsView.test.tsx.snap | 191 ++++++++++++++++-- .../Stream/CypherFrame/gqlStatusUtils.test.ts | 56 +++++ .../Stream/CypherFrame/gqlStatusUtils.ts | 45 +++++ .../Stream/CypherFrame/warningUtils.test.ts | 146 +++++++++++++ .../Stream/CypherFrame/warningUtilts.ts | 74 +++++++ src/shared/utils/strings.ts | 5 + 8 files changed, 623 insertions(+), 58 deletions(-) create mode 100644 src/browser/modules/Stream/CypherFrame/gqlStatusUtils.test.ts create mode 100644 src/browser/modules/Stream/CypherFrame/gqlStatusUtils.ts create mode 100644 src/browser/modules/Stream/CypherFrame/warningUtils.test.ts create mode 100644 src/browser/modules/Stream/CypherFrame/warningUtilts.ts create mode 100644 src/shared/utils/strings.ts diff --git a/src/browser/modules/Stream/CypherFrame/WarningsView.test.tsx b/src/browser/modules/Stream/CypherFrame/WarningsView.test.tsx index ffe5c42b5b7..ba3daa8590d 100644 --- a/src/browser/modules/Stream/CypherFrame/WarningsView.test.tsx +++ b/src/browser/modules/Stream/CypherFrame/WarningsView.test.tsx @@ -36,6 +36,7 @@ describe('WarningsViews', () => { // Then expect(container).toMatchSnapshot() }) + test('does displays a warning', () => { // Given const props = { @@ -43,13 +44,14 @@ describe('WarningsViews', () => { summary: { notifications: [ { - severity: 'WARNING xx0', + severity: 'WARNING', title: 'My xx1 warning', description: 'This is xx2 warning', position: { offset: 7, line: 1 - } + }, + code: 'xx3.Warning' } ], query: { @@ -65,29 +67,66 @@ describe('WarningsViews', () => { // Then expect(container).toMatchSnapshot() }) - test('does displays multiple warnings', () => { + + test('does display a warning for GQL status codes', () => { + // Given + const props = { + result: { + summary: { + server: { + protocolVersion: 5.6 + }, + gqlStatusObjects: [ + { + severity: 'WARNING', + gqlStatus: '03N90', + statusDescription: + "info: cartesian product. The disconnected pattern 'p = ()--(), q = ()--()' builds a cartesian product. A cartesian product may produce a large amount of data and slow down query processing.", + position: { + offset: 7, + line: 1 + } + } + ], + query: { + text: 'MATCH p=()--(), q=()--() RETURN p, q' + } + } + } + } + + // When + const { container } = render() + + // Then + expect(container).toMatchSnapshot() + }) + + test('does display multiple warnings', () => { // Given const props = { result: { summary: { notifications: [ { - severity: 'WARNING xx0', + severity: 'WARNING', title: 'My xx1 warning', description: 'This is xx2 warning', position: { offset: 7, line: 1 - } + }, + code: 'xx3.Warning' }, { - severity: 'WARNING yy0', + severity: 'WARNING', title: 'My yy1 warning', description: 'This is yy2 warning', position: { offset: 3, line: 1 - } + }, + code: 'yy3.Warning' } ], query: { @@ -103,5 +142,49 @@ describe('WarningsViews', () => { // Then expect(container).toMatchSnapshot() }) + + test('does display multiple warnings for GQL status codes', () => { + // Given + const props = { + result: { + summary: { + server: { + protocolVersion: 5.6 + }, + gqlStatusObjects: [ + { + severity: 'WARNING', + gqlStatus: '03N90', + statusDescription: + "info: cartesian product. The disconnected pattern 'p = ()--(), q = ()--()' builds a cartesian product. A cartesian product may produce a large amount of data and slow down query processing.", + position: { + offset: 7, + line: 1 + } + }, + { + severity: 'WARNING', + gqlStatus: '01N50', + statusDescription: + 'warn: label does not exist. The label `A` does not exist. Verify that the spelling is correct.', + position: { + offset: 3, + line: 1 + } + } + ], + query: { + text: 'MATCH p=()--(), q=()--() RETURN p, q' + } + } + } + } + + // When + const { container } = render() + + // Then + expect(container).toMatchSnapshot() + }) }) }) diff --git a/src/browser/modules/Stream/CypherFrame/WarningsView.tsx b/src/browser/modules/Stream/CypherFrame/WarningsView.tsx index 07871ee881b..861d1260654 100644 --- a/src/browser/modules/Stream/CypherFrame/WarningsView.tsx +++ b/src/browser/modules/Stream/CypherFrame/WarningsView.tsx @@ -34,8 +34,13 @@ import { StyledCypherInfoMessage } from '../styled' import { deepEquals } from 'neo4j-arc/common' +import { + formatNotificationsFromSummary, + FormattedNotification +} from './warningUtilts' +import { NotificationSeverityLevel } from 'neo4j-driver-core' -const getWarningComponent = (severity: any) => { +const getWarningComponent = (severity?: string | NotificationSeverityLevel) => { if (severity === 'ERROR') { return {severity} } else if (severity === 'WARNING') { @@ -56,43 +61,47 @@ export class WarningsView extends Component { render() { if (this.props.result === undefined) return null const { summary = {} } = this.props.result - const { notifications = [], query = {} } = summary + const { query = {} } = summary + const notifications = formatNotificationsFromSummary(summary) const { text: cypher = '' } = query if (!notifications || !cypher) { return null } const cypherLines = cypher.split('\n') - const notificationsList = notifications.map((notification: any) => { - // Detect generic warning without position information - const position = Object.keys(notification.position).length - ? notification.position - : { line: 1, offset: 0 } - return ( - - - {getWarningComponent(notification.severity)} - {notification.title} - - + const notificationsList = notifications.map( + (notification: FormattedNotification) => { + // Detect generic warning without position information + const { code, description, severity } = notification + const position = notification.position ?? { line: 1, offset: 0 } + const title = notification.title ?? '' + const line = position.line ?? 1 + const offset = position.offset ?? 0 + + return ( + - {notification.description} + {getWarningComponent(severity)} + {title} - - {cypherLines[position.line - 1]} - - {Array(position.offset + 1).join(' ')}^ - + {description} + + + {cypherLines[line - 1]} + + {Array(offset + 1).join(' ')}^ + + - - - Status code: {notification.code} - - - ) - }) + {code && ( + + Status code: {code} + + )} + + ) + } + ) return {notificationsList} } } diff --git a/src/browser/modules/Stream/CypherFrame/__snapshots__/WarningsView.test.tsx.snap b/src/browser/modules/Stream/CypherFrame/__snapshots__/WarningsView.test.tsx.snap index b89cff414d3..88c699fb17e 100644 --- a/src/browser/modules/Stream/CypherFrame/__snapshots__/WarningsView.test.tsx.snap +++ b/src/browser/modules/Stream/CypherFrame/__snapshots__/WarningsView.test.tsx.snap @@ -2,7 +2,7 @@ exports[`WarningsViews WarningsView displays nothing if no notifications 1`] = `
`; -exports[`WarningsViews WarningsView does displays a warning 1`] = ` +exports[`WarningsViews WarningsView does display a warning for GQL status codes 1`] = `
- WARNING xx0 + WARNING

- My xx1 warning + 03N90: Cartesian product

- This is xx2 warning + The disconnected pattern 'p = ()--(), q = ()--()' builds a cartesian product. A cartesian product may produce a large amount of data and slow down query processing.
- EXPLAIN MATCH xx3 + MATCH p=()--(), q=()--() RETURN p, q
@@ -47,21 +47,12 @@ exports[`WarningsViews WarningsView does displays a warning 1`] = `
-
- Status code: - -
`; -exports[`WarningsViews WarningsView does displays multiple warnings 1`] = ` +exports[`WarningsViews WarningsView does display multiple warnings 1`] = `
- WARNING xx0 + WARNING

+ > + xx3.Warning +

- WARNING yy0 + WARNING

+ > + yy3.Warning + +

+ + + +`; + +exports[`WarningsViews WarningsView does display multiple warnings for GQL status codes 1`] = ` +
+
+
+
+
+ WARNING +
+

+ 03N90: Cartesian product +

+
+
+
+ The disconnected pattern 'p = ()--(), q = ()--()' builds a cartesian product. A cartesian product may produce a large amount of data and slow down query processing. +
+
+ 
+            MATCH p=()--(), q=()--() RETURN p, q
+            

+                   
+            ^
+
+
+
+
+
+
+
+ WARNING +
+

+ 01N50: Label does not exist +

+
+
+
+ The label \`A\` does not exist. Verify that the spelling is correct. +
+
+ 
+            MATCH p=()--(), q=()--() RETURN p, q
+            

+               
+            ^
+
+
+
+
+
+
+`; + +exports[`WarningsViews WarningsView does displays a warning 1`] = ` +
+
+
+
+
+ WARNING +
+

+ My xx1 warning +

+
+
+
+ This is xx2 warning +
+
+ 
+            EXPLAIN MATCH xx3
+            

+                   
+            ^
+
+
+
+
+ Status code: + + xx3.Warning +
diff --git a/src/browser/modules/Stream/CypherFrame/gqlStatusUtils.test.ts b/src/browser/modules/Stream/CypherFrame/gqlStatusUtils.test.ts new file mode 100644 index 00000000000..ec3cfd4c1f0 --- /dev/null +++ b/src/browser/modules/Stream/CypherFrame/gqlStatusUtils.test.ts @@ -0,0 +1,56 @@ +import { + formatDescriptionFromGqlStatusDescription, + formatTitleFromGqlStatusDescription +} from './gqlStatusUtils' + +describe('gql status formatting', () => { + test('formats a title from a gql status description correctly', () => { + const gqlStatusDescription = + 'error: syntax error or access rule violation - invalid parameter. Invalid parameter $`param`.' + + const result = formatTitleFromGqlStatusDescription(gqlStatusDescription) + + expect(result).toEqual( + 'Syntax error or access rule violation - invalid parameter' + ) + }) + + test('formats a description from a gql status description correctly', () => { + const gqlStatusDescription = + "error: system configuration or operation exception - cyclic shortest path search disabled. Cannot find the shortest path when the start and end nodes are the same. To enable this behavior, set 'dbms.cypher.forbid_shortestpath_common_nodes' to false." + + const result = + formatDescriptionFromGqlStatusDescription(gqlStatusDescription) + + expect(result).toEqual( + "Cannot find the shortest path when the start and end nodes are the same. To enable this behavior, set 'dbms.cypher.forbid_shortestpath_common_nodes' to false." + ) + }) + + test('formats a description with no period correctly', () => { + const gqlStatusDescription = + 'error: system configuration or operation exception - cyclic shortest path search disabled. Cannot find the shortest path when the start and end nodes are the same' + const result = + formatDescriptionFromGqlStatusDescription(gqlStatusDescription) + + expect(result).toEqual( + 'Cannot find the shortest path when the start and end nodes are the same.' + ) + }) + + test('formats a title from a gql status description with no matches correctly', () => { + const gqlStatusDescription = + 'Unfortunately, no one can be told what the Matrix is. You have to see it for yourself' + const result = formatTitleFromGqlStatusDescription(gqlStatusDescription) + + expect(result).toEqual('') + }) + + test('formats a description from a gql status description with no matches correctly', () => { + const gqlStatusDescription = 'Believe the unbelievable' + const result = + formatDescriptionFromGqlStatusDescription(gqlStatusDescription) + + expect(result).toEqual('') + }) +}) diff --git a/src/browser/modules/Stream/CypherFrame/gqlStatusUtils.ts b/src/browser/modules/Stream/CypherFrame/gqlStatusUtils.ts new file mode 100644 index 00000000000..b46c7f054ea --- /dev/null +++ b/src/browser/modules/Stream/CypherFrame/gqlStatusUtils.ts @@ -0,0 +1,45 @@ +import { capitalize, isNonEmptyString } from 'shared/utils/strings' + +const gqlStatusIndexes = { + title: 1, + description: 2 +} + +const formatPropertyFromStatusDescripton = ( + index: number, + gqlStatusDescription?: string +): string | undefined => { + const matches = + gqlStatusDescription?.match( + /^(?:error|info|warn):\s(.+?)(?:\.(.+?))?\.?$/ + ) ?? [] + + return matches[index] === undefined + ? undefined + : capitalize(matches[index].trim()) +} + +export const formatTitleFromGqlStatusDescription = ( + gqlStatusDescription?: string +): string => { + return ( + formatPropertyFromStatusDescripton( + gqlStatusIndexes.title, + gqlStatusDescription + )?.trim() ?? '' + ) +} + +export const formatDescriptionFromGqlStatusDescription = ( + gqlStatusDescription?: string +): string => { + const description = + formatPropertyFromStatusDescripton( + gqlStatusIndexes.description, + gqlStatusDescription + )?.trim() ?? '' + + return isNonEmptyString(description) && !description.endsWith('.') + ? `${description}.` + : description +} diff --git a/src/browser/modules/Stream/CypherFrame/warningUtils.test.ts b/src/browser/modules/Stream/CypherFrame/warningUtils.test.ts new file mode 100644 index 00000000000..c5e13322f1a --- /dev/null +++ b/src/browser/modules/Stream/CypherFrame/warningUtils.test.ts @@ -0,0 +1,146 @@ +import type { GqlStatusObject } from 'neo4j-driver-core' +import { + notificationCategory, + notificationSeverityLevel +} from 'neo4j-driver-core' +import { formatNotificationsFromSummary } from './warningUtilts' + +describe('format rseult summary', () => { + test('formats result summary for notifications', () => { + const resultSummary = { + server: { + protocolVersion: 5.5 + }, + notifications: [ + { + code: 'Neo.ClientNotification.Statement.CartesianProduct', + title: + 'This query builds a cartesian product between disconnected patterns.', + description: + 'If a part of a query contains multiple disconnected patterns, this will build a cartesian product between all those parts. This may produce a large amount of data and slow down query processing. While occasionally intended, it may often be possible to reformulate the query that avoids the use of this cross product, perhaps by adding a relationship between the different parts or by using OPTIONAL MATCH (identifiers are: ())', + severity: 'INFORMATION', + position: { offset: 0, line: 1, column: 1 }, + severityLevel: notificationSeverityLevel.INFORMATION, + rawSeverityLevel: 'INFORMATION', + category: notificationCategory.PERFORMANCE, + rawCategory: 'PERFORMANCE' + }, + { + code: 'Neo.ClientNotification.Statement.UnknownLabelWarning', + title: 'The provided label is not in the database.', + description: + "One of the labels in your query is not available in the database, make sure you didn't misspell it or that the label is available when you run this statement in your application (the missing label name is: A)", + severity: 'WARNING', + position: { offset: 9, line: 1, column: 10 }, + severityLevel: notificationSeverityLevel.WARNING, + rawSeverityLevel: 'WARNING', + category: notificationCategory.UNRECOGNIZED, + rawCategory: 'UNRECOGNIZED' + } + ] + } + + const result = formatNotificationsFromSummary(resultSummary) + + expect(result).toEqual([ + { + code: 'Neo.ClientNotification.Statement.CartesianProduct', + description: + 'If a part of a query contains multiple disconnected patterns, this will build a cartesian product between all those parts. This may produce a large amount of data and slow down query processing. While occasionally intended, it may often be possible to reformulate the query that avoids the use of this cross product, perhaps by adding a relationship between the different parts or by using OPTIONAL MATCH (identifiers are: ())', + position: { + column: 1, + line: 1, + offset: 0 + }, + title: + 'This query builds a cartesian product between disconnected patterns.', + severity: 'INFORMATION' + }, + { + code: 'Neo.ClientNotification.Statement.UnknownLabelWarning', + description: + "One of the labels in your query is not available in the database, make sure you didn't misspell it or that the label is available when you run this statement in your application (the missing label name is: A)", + position: { + column: 10, + line: 1, + offset: 9 + }, + title: 'The provided label is not in the database.', + severity: 'WARNING' + } + ]) + }) + + test('formats result summary for gql status objects', () => { + const gqlStatusObjects: [GqlStatusObject, ...GqlStatusObject[]] = [ + { + gqlStatus: '03N90', + statusDescription: + "info: cartesian product. The disconnected pattern 'p = ()--(), q = ()--()' builds a cartesian product. A cartesian product may produce a large amount of data and slow down query processing.", + diagnosticRecord: { + OPERATION: '', + OPERATION_CODE: '0', + CURRENT_SCHEMA: '/', + classification: 'PERFORMANCE' + }, + position: { offset: 0, line: 1, column: 1 }, + severity: 'INFORMATION', + rawSeverity: 'INFORMATION', + classification: 'PERFORMANCE', + rawClassification: 'PERFORMANCE', + isNotification: true, + diagnosticRecordAsJsonString: '' + }, + { + gqlStatus: '01N50', + statusDescription: + 'warn: label does not exist. The label `A` does not exist. Verify that the spelling is correct.', + diagnosticRecord: { + OPERATION: '', + OPERATION_CODE: '0', + CURRENT_SCHEMA: '/' + }, + position: { offset: 9, line: 1, column: 10 }, + severity: 'WARNING', + rawSeverity: 'WARNING', + classification: 'UNRECOGNIZED', + rawClassification: 'UNRECOGNIZED', + isNotification: true, + diagnosticRecordAsJsonString: '' + } + ] + const resultSummary = { + server: { + protocolVersion: 5.7 + }, + gqlStatusObjects + } + + const result = formatNotificationsFromSummary(resultSummary) + + expect(result).toEqual([ + { + description: + "The disconnected pattern 'p = ()--(), q = ()--()' builds a cartesian product. A cartesian product may produce a large amount of data and slow down query processing.", + position: { + column: 1, + line: 1, + offset: 0 + }, + title: '03N90: Cartesian product', + severity: 'INFORMATION' + }, + { + description: + 'The label `A` does not exist. Verify that the spelling is correct.', + position: { + column: 10, + line: 1, + offset: 9 + }, + title: '01N50: Label does not exist', + severity: 'WARNING' + } + ]) + }) +}) diff --git a/src/browser/modules/Stream/CypherFrame/warningUtilts.ts b/src/browser/modules/Stream/CypherFrame/warningUtilts.ts new file mode 100644 index 00000000000..67a0e61077d --- /dev/null +++ b/src/browser/modules/Stream/CypherFrame/warningUtilts.ts @@ -0,0 +1,74 @@ +import type { Notification, NotificationSeverityLevel } from 'neo4j-driver' +import { + GqlStatusObject, + NotificationPosition, + ResultSummary +} from 'neo4j-driver-core' +import { + formatDescriptionFromGqlStatusDescription, + formatTitleFromGqlStatusDescription +} from './gqlStatusUtils' +import { isNonEmptyString } from 'shared/utils/strings' + +export type FormattedNotification = { + title?: string + description: string + position?: NotificationPosition + code?: string | null + severity?: NotificationSeverityLevel | string +} + +const mapGqlStatusObjectsToFormattedNotifications = ( + statusObjects: Omit[] +): FormattedNotification[] => { + return statusObjects.map(statusObject => { + const gqlStatusTitle = formatTitleFromGqlStatusDescription( + statusObject.statusDescription + ) + const { gqlStatus } = statusObject + const description = formatDescriptionFromGqlStatusDescription( + statusObject.statusDescription + ) + const title = isNonEmptyString(gqlStatusTitle) + ? gqlStatusTitle + : description + return { + title: isNonEmptyString(title) ? `${gqlStatus}: ${title}` : gqlStatus, + description, + position: statusObject.position, + severity: statusObject.severity + } + }) +} + +const mapNotificationsToFormattedNotifications = ( + notifications: Notification[] +): FormattedNotification[] => { + return notifications.map(notification => ({ + title: notification.title, + description: notification.description, + position: notification.position, + severity: notification.severity, + code: notification.code + })) +} + +export const formatNotificationsFromSummary = ( + resultSummary: Partial +): FormattedNotification[] => { + const { protocolVersion } = resultSummary.server ?? {} + const severityLevels = ['ERROR', 'WARNING', 'INFORMATION'] + if (protocolVersion === undefined || protocolVersion < 5.6) { + const filteredNotifications = + resultSummary.notifications?.filter(x => + severityLevels.includes(x.severity) + ) ?? [] + return mapNotificationsToFormattedNotifications(filteredNotifications) + } + + const filteredStatusObjects = + resultSummary.gqlStatusObjects?.filter(x => + severityLevels.includes(x.severity) + ) ?? [] + return mapGqlStatusObjectsToFormattedNotifications(filteredStatusObjects) +} diff --git a/src/shared/utils/strings.ts b/src/shared/utils/strings.ts new file mode 100644 index 00000000000..6277a553e2f --- /dev/null +++ b/src/shared/utils/strings.ts @@ -0,0 +1,5 @@ +export const capitalize = (s: string): string => + s.charAt(0).toUpperCase() + s.slice(1) + +export const isNonEmptyString = (s: unknown): s is string => + typeof s === 'string' && s !== ''