From 5fcd6ab500194bbcae5e8cc46c88560d671909d6 Mon Sep 17 00:00:00 2001 From: miinhho Date: Mon, 5 Oct 2026 19:54:07 +0900 Subject: [PATCH] [ZEPPELIN-6676] Match notebook status, result codes and dataset types to server values Constrain paragraph snapshot and event states, result codes, and dataset types. Adapt Angular and React consumers, including SVG/NULL output and pending work invalidated by those output resets. Keep unrelated renderer behavior unchanged. Server evidence (paths relative to the repository root): - ParagraphItem.status, ParagraphStatus.status and terminal classification: zeppelin-interpreter/src/main/java/org/apache/zeppelin/scheduler/Job.java:56-80 (enumerated all seven Status values and isCompleted). - ParagraphResults.code and DatasetType: zeppelin-interpreter/src/main/java/org/apache/zeppelin/interpreter/InterpreterResult.java:40-61 (enumerated all four Code and eight Type values). --- .../components/renderers/ImageRenderer.tsx | 8 +- .../templates/SingleResultRenderer.spec.tsx | 37 +- .../src/templates/SingleResultRenderer.tsx | 9 +- .../interfaces/message-paragraph.interface.ts | 24 +- .../interfaces/notebook-finite-values.spec.ts | 45 +++ .../zeppelin-sdk/src/paragraph-state.spec.ts | 24 ++ .../zeppelin-sdk/src/paragraph-state.ts | 27 ++ .../projects/zeppelin-sdk/src/public-api.ts | 1 + .../paragraph-base/paragraph-base.spec.ts | 8 +- .../app/core/paragraph-base/paragraph-base.ts | 22 +- .../paragraph/paragraph.component.spec.ts | 4 +- .../share/result/result.component.html | 9 +- .../share/result/result.component.spec.ts | 337 ++++++++++++++++++ .../share/result/result.component.ts | 55 ++- .../classic-visualization.service.spec.ts | 101 ++++++ .../services/classic-visualization.service.ts | 33 +- 16 files changed, 704 insertions(+), 40 deletions(-) create mode 100644 zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/notebook-finite-values.spec.ts create mode 100644 zeppelin-web-angular/projects/zeppelin-sdk/src/paragraph-state.spec.ts create mode 100644 zeppelin-web-angular/projects/zeppelin-sdk/src/paragraph-state.ts create mode 100644 zeppelin-web-angular/src/app/pages/workspace/share/result/result.component.spec.ts create mode 100644 zeppelin-web-angular/src/app/services/classic-visualization.service.spec.ts diff --git a/zeppelin-web-angular/projects/zeppelin-react/src/components/renderers/ImageRenderer.tsx b/zeppelin-web-angular/projects/zeppelin-react/src/components/renderers/ImageRenderer.tsx index 1bcc157925e..1d24d1554a9 100644 --- a/zeppelin-web-angular/projects/zeppelin-react/src/components/renderers/ImageRenderer.tsx +++ b/zeppelin-web-angular/projects/zeppelin-react/src/components/renderers/ImageRenderer.tsx @@ -12,10 +12,14 @@ export interface ImageRendererProps { imageData: string; + format?: 'png' | 'svg'; } -export const ImageRenderer = ({ imageData }: ImageRendererProps) => { - const imgSrc = `data:image/png;base64,${imageData}`; +export const ImageRenderer = ({ imageData, format = 'png' }: ImageRendererProps) => { + const imgSrc = + format === 'svg' + ? `data:image/svg+xml;charset=utf-8,${encodeURIComponent(imageData)}` + : `data:image/png;base64,${imageData}`; return Result; }; diff --git a/zeppelin-web-angular/projects/zeppelin-react/src/templates/SingleResultRenderer.spec.tsx b/zeppelin-web-angular/projects/zeppelin-react/src/templates/SingleResultRenderer.spec.tsx index 3240b42f11a..ab9f4423c67 100644 --- a/zeppelin-web-angular/projects/zeppelin-react/src/templates/SingleResultRenderer.spec.tsx +++ b/zeppelin-web-angular/projects/zeppelin-react/src/templates/SingleResultRenderer.spec.tsx @@ -63,6 +63,40 @@ describe('SingleResultRenderer', () => { expect(screen.getByRole('img').getAttribute('src')).toBe('data:image/png;base64,QUJD'); }); + it('renders SVG as an encoded image without inserting its markup into the page', () => { + const svg = '서울 # &'; + const { container } = render(); + + expect(screen.getByRole('img').getAttribute('src')).toBe( + `data:image/svg+xml;charset=utf-8,${encodeURIComponent(svg)}` + ); + expect(container.querySelector('svg')).toBeNull(); + expect(container.querySelector('script')).toBeNull(); + }); + + it('updates the image format when an IMG result becomes SVG and back', () => { + const svg = ''; + const view = render(); + + view.rerender(); + expect(screen.getByRole('img').getAttribute('src')).toBe( + `data:image/svg+xml;charset=utf-8,${encodeURIComponent(svg)}` + ); + + view.rerender(); + expect(screen.getByRole('img').getAttribute('src')).toBe('data:image/png;base64,REVG'); + }); + + it('clears the previous image when the result becomes NULL', () => { + const view = render(')} />); + expect(screen.getByRole('img')).toBeTruthy(); + + view.rerender(); + + expect(screen.queryByRole('img')).toBeNull(); + expect(view.container.innerHTML).toBe(''); + }); + it('renders HTML as markup rather than as text', () => { render(markup output

')} />); @@ -76,8 +110,7 @@ describe('SingleResultRenderer', () => { expect(screen.getByText(/not supported in React environment/)).toBeTruthy(); }); - it('renders nothing for a type it has no renderer for', () => { - // NETWORK is declared by the SDK and reaches the default arm. + it('renders nothing for NETWORK, which has no renderer', () => { const { container } = render(); expect(container.innerHTML).toBe(''); diff --git a/zeppelin-web-angular/projects/zeppelin-react/src/templates/SingleResultRenderer.tsx b/zeppelin-web-angular/projects/zeppelin-react/src/templates/SingleResultRenderer.tsx index 5b4202ad1f7..10131bb5527 100644 --- a/zeppelin-web-angular/projects/zeppelin-react/src/templates/SingleResultRenderer.tsx +++ b/zeppelin-web-angular/projects/zeppelin-react/src/templates/SingleResultRenderer.tsx @@ -33,6 +33,8 @@ export const SingleResultRenderer = ({ result, index, config }: SingleResultRend return ; case DatasetType.IMG: return ; + case DatasetType.SVG: + return ; case DatasetType.ANGULAR: return ( ); - default: + case DatasetType.NULL: + case DatasetType.NETWORK: return null; + default: { + const _unhandled: never = result.type; + return null; + } } }; diff --git a/zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/message-paragraph.interface.ts b/zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/message-paragraph.interface.ts index 0906e196875..0025b1976ad 100644 --- a/zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/message-paragraph.interface.ts +++ b/zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/message-paragraph.interface.ts @@ -79,17 +79,33 @@ export interface ParagraphConfig { export interface ParagraphResults { [index: number]: Record; - code?: string; + code?: ParagraphResultCode; msg?: ParagraphIResultsMsgItem[]; } +export type ParagraphResultCode = 'SUCCESS' | 'INCOMPLETE' | 'ERROR' | 'KEEP_PREVIOUS_RESULT'; + +export const ParagraphStates = { + UNKNOWN: 'UNKNOWN', + READY: 'READY', + PENDING: 'PENDING', + RUNNING: 'RUNNING', + FINISHED: 'FINISHED', + ERROR: 'ERROR', + ABORT: 'ABORT' +} as const; + +export type ParagraphState = (typeof ParagraphStates)[keyof typeof ParagraphStates]; + export enum DatasetType { NETWORK = 'NETWORK', TABLE = 'TABLE', HTML = 'HTML', TEXT = 'TEXT', ANGULAR = 'ANGULAR', - IMG = 'IMG' + IMG = 'IMG', + SVG = 'SVG', + NULL = 'NULL' } export class ParagraphIResultsMsgItem { @@ -148,7 +164,7 @@ export interface ParagraphItem { dateFinished?: string; errorMessage?: string; runtimeInfos?: RuntimeInfos; - status: string; + status: ParagraphState; title?: string; focus?: boolean; // TODO(hsuanxyz): define proper type @@ -348,7 +364,7 @@ export interface Progress { export interface ParagraphStatus { id: string; - status: string; + status: ParagraphState; } interface GraphConfigSetting { diff --git a/zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/notebook-finite-values.spec.ts b/zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/notebook-finite-values.spec.ts new file mode 100644 index 00000000000..e79d68f81f3 --- /dev/null +++ b/zeppelin-web-angular/projects/zeppelin-sdk/src/interfaces/notebook-finite-values.spec.ts @@ -0,0 +1,45 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { assertType, expect, expectTypeOf, it } from 'vitest'; + +import { + DatasetType, + ParagraphItem, + ParagraphResultCode, + ParagraphState, + ParagraphStatus +} from './message-paragraph.interface'; + +it('matches the finite values in Job.Status and InterpreterResult', () => { + expectTypeOf().toEqualTypeOf< + 'UNKNOWN' | 'READY' | 'PENDING' | 'RUNNING' | 'FINISHED' | 'ERROR' | 'ABORT' + >(); + expectTypeOf().toEqualTypeOf(); + expectTypeOf['code']>().toEqualTypeOf(); + expectTypeOf().toEqualTypeOf<'SUCCESS' | 'INCOMPLETE' | 'ERROR' | 'KEEP_PREVIOUS_RESULT'>(); + assertType(DatasetType.SVG); + assertType(DatasetType.NULL); +}); + +it('declares all eight InterpreterResult.Type wire values', () => { + expect(Object.values(DatasetType).sort()).toEqual([ + 'ANGULAR', + 'HTML', + 'IMG', + 'NETWORK', + 'NULL', + 'SVG', + 'TABLE', + 'TEXT' + ]); +}); diff --git a/zeppelin-web-angular/projects/zeppelin-sdk/src/paragraph-state.spec.ts b/zeppelin-web-angular/projects/zeppelin-sdk/src/paragraph-state.spec.ts new file mode 100644 index 00000000000..7314cdc5acd --- /dev/null +++ b/zeppelin-web-angular/projects/zeppelin-sdk/src/paragraph-state.spec.ts @@ -0,0 +1,24 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { expect, it } from 'vitest'; + +import { ParagraphStates } from './interfaces/message-paragraph.interface'; +import { isTerminalParagraphState } from './paragraph-state'; + +it.each(Object.values(ParagraphStates))('classifies %s using Job.Status.isCompleted', status => { + expect(isTerminalParagraphState(status)).toBe(['FINISHED', 'ERROR', 'ABORT'].includes(status)); +}); + +it('does not treat a missing paragraph as a completed run', () => { + expect(isTerminalParagraphState(undefined)).toBe(false); +}); diff --git a/zeppelin-web-angular/projects/zeppelin-sdk/src/paragraph-state.ts b/zeppelin-web-angular/projects/zeppelin-sdk/src/paragraph-state.ts new file mode 100644 index 00000000000..0b0a03b858f --- /dev/null +++ b/zeppelin-web-angular/projects/zeppelin-sdk/src/paragraph-state.ts @@ -0,0 +1,27 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { ParagraphState } from './interfaces/message-paragraph.interface'; + +// Job.Status.isCompleted: FINISHED, ERROR and ABORT terminate a run. +const terminalStates: Record = { + UNKNOWN: false, + READY: false, + PENDING: false, + RUNNING: false, + FINISHED: true, + ERROR: true, + ABORT: true +}; + +export const isTerminalParagraphState = (status: ParagraphState | undefined): boolean => + status !== undefined && terminalStates[status]; diff --git a/zeppelin-web-angular/projects/zeppelin-sdk/src/public-api.ts b/zeppelin-web-angular/projects/zeppelin-sdk/src/public-api.ts index 5e6b79271ae..2db821a15ab 100644 --- a/zeppelin-web-angular/projects/zeppelin-sdk/src/public-api.ts +++ b/zeppelin-web-angular/projects/zeppelin-sdk/src/public-api.ts @@ -12,3 +12,4 @@ export * from './interfaces/public-api'; export * from './message'; +export * from './paragraph-state'; diff --git a/zeppelin-web-angular/src/app/core/paragraph-base/paragraph-base.spec.ts b/zeppelin-web-angular/src/app/core/paragraph-base/paragraph-base.spec.ts index 5f894dcf409..fbfc695f8a0 100644 --- a/zeppelin-web-angular/src/app/core/paragraph-base/paragraph-base.spec.ts +++ b/zeppelin-web-angular/src/app/core/paragraph-base/paragraph-base.spec.ts @@ -11,7 +11,7 @@ */ import { ChangeDetectorRef } from '@angular/core'; -import { DatasetType, Message, OP, ParagraphItem } from '@zeppelin/sdk'; +import { DatasetType, Message, OP, ParagraphItem, ParagraphState } from '@zeppelin/sdk'; import { EMPTY } from 'rxjs'; import { afterEach, describe, expect, it, vi } from 'vitest'; @@ -38,7 +38,11 @@ class TestParagraph extends ParagraphBase { } } -const paragraph = (id: string, status = 'RUNNING', dateStarted = '2026-01-01T00:00:00Z'): ParagraphItem => ({ +const paragraph = ( + id: string, + status: ParagraphState = 'RUNNING', + dateStarted = '2026-01-01T00:00:00Z' +): ParagraphItem => ({ id, status, dateStarted, diff --git a/zeppelin-web-angular/src/app/core/paragraph-base/paragraph-base.ts b/zeppelin-web-angular/src/app/core/paragraph-base/paragraph-base.ts index efdab3dc24b..27247815731 100644 --- a/zeppelin-web-angular/src/app/core/paragraph-base/paragraph-base.ts +++ b/zeppelin-web-angular/src/app/core/paragraph-base/paragraph-base.ts @@ -23,6 +23,8 @@ import { ParagraphConfigResults, ParagraphEditorSetting, ParagraphItem, + ParagraphStates, + isTerminalParagraphState, ParagraphIResultsMsgItem, WebSocketMessage } from '@zeppelin/sdk'; @@ -39,17 +41,7 @@ import { AngularContextManager } from './angular-context-manager'; import { NoteStatus } from './note-status'; import { ParagraphOutputState } from './paragraph-output-state'; -export const ParagraphStatus = { - READY: 'READY', - PENDING: 'PENDING', - RUNNING: 'RUNNING', - FINISHED: 'FINISHED', - ABORT: 'ABORT', - ERROR: 'ERROR' -}; - -const isTerminalParagraphStatus = (status?: string): boolean => - status === ParagraphStatus.FINISHED || status === ParagraphStatus.ABORT || status === ParagraphStatus.ERROR; +export const ParagraphStatus = ParagraphStates; export abstract class ParagraphBase extends MessageListenersManager { paragraph?: ParagraphItem; @@ -173,7 +165,7 @@ export abstract class ParagraphBase extends MessageListenersManager { this.outputState.reset(); } // Close the stream before publishing the terminal snapshot. - if (isTerminalParagraphStatus(newPara.status)) { + if (isTerminalParagraphState(newPara.status)) { this.outputState.finish(newPara.results?.msg); } if (this.isUpdateRequired(oldPara, newPara)) { @@ -247,7 +239,7 @@ export abstract class ParagraphBase extends MessageListenersManager { private initializeOutputState(): void { if (!this.outputState.isInitialized) { - this.outputState.reset(this.results, isTerminalParagraphStatus(this.paragraph?.status)); + this.outputState.reset(this.results, isTerminalParagraphState(this.paragraph?.status)); } } @@ -312,7 +304,7 @@ export abstract class ParagraphBase extends MessageListenersManager { newPara.jobName !== oldPara.jobName || newPara.title !== oldPara.title || isEmpty(newPara.results) !== isEmpty(oldPara.results) || - (isTerminalParagraphStatus(newPara.status) && !isEqual(newPara.results?.msg, oldPara.results?.msg)) || + (isTerminalParagraphState(newPara.status) && !isEqual(newPara.results?.msg, oldPara.results?.msg)) || newPara.errorMessage !== oldPara.errorMessage || !isEqual(newPara.settings, oldPara.settings) || !isEqual(newPara.config, oldPara.config) || @@ -481,7 +473,7 @@ export abstract class ParagraphBase extends MessageListenersManager { if (paragraph) { this.setResults(paragraph); } - const terminal = isTerminalParagraphStatus(paragraph?.status); + const terminal = isTerminalParagraphState(paragraph?.status); this.outputState.reset(this.results, terminal); this.cdr.markForCheck(); } diff --git a/zeppelin-web-angular/src/app/pages/workspace/published/paragraph/paragraph.component.spec.ts b/zeppelin-web-angular/src/app/pages/workspace/published/paragraph/paragraph.component.spec.ts index 731519047ea..30490196bb4 100644 --- a/zeppelin-web-angular/src/app/pages/workspace/published/paragraph/paragraph.component.spec.ts +++ b/zeppelin-web-angular/src/app/pages/workspace/published/paragraph/paragraph.component.spec.ts @@ -12,7 +12,7 @@ import { QueryList } from '@angular/core'; import { convertToParamMap } from '@angular/router'; -import { DatasetType, Note, ParagraphItem } from '@zeppelin/sdk'; +import { DatasetType, Note, ParagraphItem, ParagraphState } from '@zeppelin/sdk'; import { BehaviorSubject, EMPTY, of } from 'rxjs'; import { afterEach, describe, expect, it, vi } from 'vitest'; @@ -32,7 +32,7 @@ vi.mock('../../share/result/result.component', () => ({ NotebookParagraphResultC import { PublishedParagraphComponent } from './paragraph.component'; -const snapshot = (noteId: string, status: string, text: string): Note => ({ +const snapshot = (noteId: string, status: ParagraphState, text: string): Note => ({ note: { id: noteId, name: noteId, diff --git a/zeppelin-web-angular/src/app/pages/workspace/share/result/result.component.html b/zeppelin-web-angular/src/app/pages/workspace/share/result/result.component.html index 878bd2efdc4..0d0804e814d 100644 --- a/zeppelin-web-angular/src/app/pages/workspace/share/result/result.component.html +++ b/zeppelin-web-angular/src/app/pages/workspace/share/result/result.component.html @@ -69,7 +69,7 @@ } - @if (getCurrentVisualization()?.isClassic) { + @if (result.type !== datasetType.SVG && result.type !== datasetType.NULL && getCurrentVisualization()?.isClassic) {
} - @if (getCurrentVisualization()?.isClassic) { + @if (result.type !== datasetType.SVG && result.type !== datasetType.NULL && getCurrentVisualization()?.isClassic) {
} - @if (getCurrentVisualization()?.isClassic) { + @if (result.type !== datasetType.SVG && result.type !== datasetType.NULL && getCurrentVisualization()?.isClassic) {
} @switch (result?.type) { @@ -103,6 +103,9 @@ @case (datasetType.IMG) {
img
} + @case (datasetType.SVG) { +
svg
+ } } @if (frontEndError) {
diff --git a/zeppelin-web-angular/src/app/pages/workspace/share/result/result.component.spec.ts b/zeppelin-web-angular/src/app/pages/workspace/share/result/result.component.spec.ts new file mode 100644 index 00000000000..6ceacf50e94 --- /dev/null +++ b/zeppelin-web-angular/src/app/pages/workspace/share/result/result.component.spec.ts @@ -0,0 +1,337 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { CdkPortalOutlet, PortalModule } from '@angular/cdk/portal'; +import { CommonModule } from '@angular/common'; +import { + ChangeDetectorRef, + Injector, + NO_ERRORS_SCHEMA, + provideZoneChangeDetection, + ViewContainerRef +} from '@angular/core'; +import { TestBed } from '@angular/core/testing'; +import { DomSanitizer } from '@angular/platform-browser'; +import { DatasetType, GraphConfig } from '@zeppelin/sdk'; +import { HeliumClassicVisualization, HeliumClassicVisualizationConstructor } from '@zeppelin/interfaces'; +import { Visualization } from '@zeppelin/visualization'; +import { EMPTY, Subscription } from 'rxjs'; +import { describe, expect, it, vi } from 'vitest'; + +vi.mock('@zeppelin/services', () => ({ + ClassicVisualizationService: class {}, + HeliumService: class {}, + NgZService: class {}, + RuntimeCompilerService: class {} +})); +vi.mock('@zeppelin/visualizations', () => ({ + AreaChartVisualization: class {}, + BarChartVisualization: class {}, + LineChartVisualization: class {}, + PieChartVisualization: class {}, + ScatterChartVisualization: class {}, + TableVisualization: class {} +})); + +import { + ClassicVisualizationService, + DynamicTemplate, + HeliumService, + NgZService, + RuntimeCompilerService +} from '@zeppelin/services'; +import { NotebookParagraphResultComponent } from './result.component'; +import template from './result.component.html?raw'; + +const component = ( + compiler: Partial = {}, + classic: Partial = {} +) => + new NotebookParagraphResultComponent( + {} as Injector, + {} as ViewContainerRef, + { detectChanges: vi.fn(), markForCheck: vi.fn() } as unknown as ChangeDetectorRef, + compiler as RuntimeCompilerService, + {} as DomSanitizer, + {} as NgZService, + {} as HeliumService, + { destroyAllInstances: vi.fn(), ...classic } as unknown as ClassicVisualizationService + ); + +const pendingResult = () => { + let resolve!: (value: T) => void; + let reject!: (error: Error) => void; + const promise = new Promise((resolvePromise, rejectPromise) => { + resolve = resolvePromise; + reject = rejectPromise; + }); + return { promise, resolve, reject }; +}; + +const pendingCompilation = () => pendingResult(); + +const flushCompilation = async () => { + await Promise.resolve(); + await Promise.resolve(); +}; + +describe('finite dataset rendering', () => { + it('encodes SVG output as an image URL', () => { + const result = component(); + const svg = '한글 # &'; + result.result = { type: DatasetType.SVG, data: svg }; + result.angularComponent = {} as DynamicTemplate; + result.renderDefaultDisplay(); + expect(result.imgData).toBe(`data:image/svg+xml;charset=utf-8,${encodeURIComponent(svg)}`); + expect(result.angularComponent).toBeNull(); + }); + + it('clears a previous Angular display for NULL output', () => { + const result = component(); + result.result = { type: DatasetType.NULL, data: '' }; + result.angularComponent = {} as DynamicTemplate; + result.renderDefaultDisplay(); + expect(result.angularComponent).toBeNull(); + }); + + it.each([DatasetType.NULL, DatasetType.NETWORK])('leaves %s without a display renderer', type => { + const result = component(); + result.result = { type, data: '' }; + const renderHTML = vi.spyOn(result, 'renderHTML'); + const renderAngular = vi.spyOn(result, 'renderAngular'); + result.renderDefaultDisplay(); + expect(renderHTML).not.toHaveBeenCalled(); + expect(renderAngular).not.toHaveBeenCalled(); + expect(result.frontEndError).toBe(''); + }); +}); + +describe('SVG and NULL invalidate pending Angular output', () => { + it.each([DatasetType.SVG, DatasetType.NULL])('ignores stale compilation success after %s output', async type => { + const pending = pendingCompilation(); + const result = component({ createAndCompileTemplate: vi.fn().mockReturnValue(pending.promise) }); + result.result = { type: DatasetType.ANGULAR, data: '

previous

' }; + result.renderDefaultDisplay(); + result.result = { type, data: '' }; + result.renderDefaultDisplay(); + + pending.resolve({} as DynamicTemplate); + await flushCompilation(); + + expect(result.angularComponent).toBeNull(); + expect(result.frontEndError).toBe(''); + }); + + it.each([DatasetType.SVG, DatasetType.NULL])('ignores stale compilation errors after %s output', async type => { + const pending = pendingCompilation(); + const result = component({ createAndCompileTemplate: vi.fn().mockReturnValue(pending.promise) }); + result.result = { type: DatasetType.ANGULAR, data: '

previous

' }; + result.renderDefaultDisplay(); + result.result = { type, data: '' }; + result.renderDefaultDisplay(); + + pending.reject(new Error('obsolete compilation')); + await flushCompilation(); + + expect(result.angularComponent).toBeNull(); + expect(result.frontEndError).toBe(''); + }); + + it('reports errors from the current Angular compilation', async () => { + const pending = pendingCompilation(); + const result = component({ createAndCompileTemplate: vi.fn().mockReturnValue(pending.promise) }); + result.result = { type: DatasetType.ANGULAR, data: '

current

' }; + result.renderDefaultDisplay(); + + pending.reject(new Error('current compilation failed')); + await flushCompilation(); + + expect(result.angularComponent).toBeNull(); + expect(result.frontEndError).toBe('current compilation failed'); + }); +}); + +describe('result type transitions', () => { + it.each([DatasetType.SVG, DatasetType.NULL])('destroys an attached modern visualization for %s', type => { + const result = component(); + const destroy = vi.fn(); + const detach = vi.fn(); + const unsubscribe = vi.fn(); + const subscription = new Subscription(unsubscribe); + result.visualizations[0].instance = { destroy } as unknown as Visualization; + result.visualizations[0].changeSubscription = subscription; + result.portalOutlet = { hasAttached: () => true, detach } as unknown as CdkPortalOutlet; + result.result = { type, data: '' }; + + result.renderDefaultDisplay(); + + expect(destroy).toHaveBeenCalledOnce(); + expect(detach).toHaveBeenCalledOnce(); + expect(unsubscribe).toHaveBeenCalledOnce(); + expect(result.visualizations[0].instance).toBeUndefined(); + expect(result.visualizations[0].changeSubscription).toBeNull(); + }); + + it.each([DatasetType.SVG, DatasetType.NULL])('destroys a previous classic visualization for %s', type => { + const destroyInstance = vi.fn(); + const result = component({}, { destroyInstance }); + result.id = 'paragraph'; + result.visualizations.push({ + id: 'classic', + name: 'classic', + isClassic: true, + icon: {}, + Class: vi.fn() as unknown as HeliumClassicVisualizationConstructor, + instance: {} as HeliumClassicVisualization, + changeSubscription: null + }); + result.result = { type, data: '' }; + + result.renderDefaultDisplay(); + + expect(destroyInstance).toHaveBeenCalledExactlyOnceWith('pparagraph_classic'); + expect(result.visualizations.at(-1)?.instance).toBeUndefined(); + }); + + it('preserves an existing visualization during a TABLE refresh', () => { + const result = component(); + const destroy = vi.fn(); + const instance = { destroy } as unknown as Visualization; + result.visualizations[0].instance = instance; + result.result = { type: DatasetType.TABLE, data: 'column\nvalue' }; + const renderGraph = vi.spyOn(result, 'renderGraph').mockImplementation(() => {}); + + result.renderDefaultDisplay(); + + expect(renderGraph).toHaveBeenCalledOnce(); + expect(destroy).not.toHaveBeenCalled(); + expect(result.visualizations[0].instance).toBe(instance); + }); + + it.each([DatasetType.SVG, DatasetType.NULL])('cancels a pending classic visualization after %s', async type => { + const pending = pendingResult(); + const createClassicVisualization = vi.fn().mockReturnValue(pending.promise); + const result = component({}, { createClassicVisualization }); + result.id = 'paragraph'; + result.visualizations.push({ + id: 'classic', + name: 'classic', + isClassic: true, + icon: {}, + Class: vi.fn() as unknown as HeliumClassicVisualizationConstructor, + instance: undefined, + changeSubscription: null + }); + result.config = { graph: { ...new GraphConfig(), mode: 'classic' } }; + result.result = { type: DatasetType.TABLE, data: 'column\nvalue' }; + result.renderDefaultDisplay(); + const isCurrentRender = createClassicVisualization.mock.calls[0][5] as () => boolean; + expect(isCurrentRender()).toBe(true); + + result.result = { type, data: '' }; + result.renderDefaultDisplay(); + expect(isCurrentRender()).toBe(false); + pending.resolve(undefined); + await flushCompilation(); + + expect(result.visualizations.at(-1)?.instance).toBeUndefined(); + expect(result.frontEndError).toBe(''); + }); + + it('keeps the current classic instance when an obsolete render completes after returning to TABLE', async () => { + const previous = pendingResult(); + const current = pendingResult(); + const destroyInstance = vi.fn(); + const result = component( + {}, + { + createClassicVisualization: vi.fn().mockReturnValueOnce(previous.promise).mockReturnValueOnce(current.promise), + destroyInstance + } + ); + result.id = 'paragraph'; + result.visualizations.push({ + id: 'classic', + name: 'classic', + isClassic: true, + icon: {}, + Class: vi.fn() as unknown as HeliumClassicVisualizationConstructor, + instance: undefined, + changeSubscription: null + }); + result.config = { graph: { ...new GraphConfig(), mode: 'classic' } }; + result.result = { type: DatasetType.TABLE, data: 'column\nprevious' }; + result.renderDefaultDisplay(); + result.result = { type: DatasetType.NULL, data: '' }; + result.renderDefaultDisplay(); + result.result = { type: DatasetType.TABLE, data: 'column\ncurrent' }; + result.renderDefaultDisplay(); + const currentInstance = {} as HeliumClassicVisualization; + const previousInstance = {} as HeliumClassicVisualization; + + current.resolve(currentInstance); + await flushCompilation(); + previous.resolve(previousInstance); + await flushCompilation(); + + expect(result.visualizations.at(-1)?.instance).toBe(currentInstance); + expect(destroyInstance).toHaveBeenCalledExactlyOnceWith('pparagraph_classic', false, previousInstance); + }); + + it.each([DatasetType.SVG, DatasetType.NULL])( + 'removes classic visualization containers for %s in the template', + async type => { + await TestBed.configureTestingModule({ + declarations: [NotebookParagraphResultComponent], + imports: [CommonModule, PortalModule], + schemas: [NO_ERRORS_SCHEMA], + providers: [ + provideZoneChangeDetection(), + { provide: RuntimeCompilerService, useValue: {} }, + { provide: NgZService, useValue: { contextChanged: () => EMPTY } }, + { provide: HeliumService, useValue: { visualizationBundles: () => EMPTY } }, + { provide: ClassicVisualizationService, useValue: { destroyAllInstances: vi.fn() } } + ] + }) + .overrideComponent(NotebookParagraphResultComponent, { + set: { template, templateUrl: undefined, styles: [], styleUrls: [] } + }) + .compileComponents(); + const fixture = TestBed.createComponent(NotebookParagraphResultComponent); + const result = fixture.componentInstance; + result.published = true; + result.id = 'paragraph'; + result.visualizations.push({ + id: 'classic', + name: 'classic', + isClassic: true, + icon: {}, + Class: vi.fn() as unknown as HeliumClassicVisualizationConstructor, + instance: undefined, + changeSubscription: null + }); + result.config = { graph: { ...new GraphConfig(), mode: 'classic' } }; + result.result = { type: DatasetType.TABLE, data: 'column\nvalue' }; + vi.spyOn(result, 'renderGraph').mockImplementation(() => {}); + fixture.detectChanges(); + const element = fixture.nativeElement as HTMLElement; + const containers = '.classic-visualization-container, .transformation-setting, .visualization-setting'; + expect(element.querySelectorAll(containers)).toHaveLength(3); + + result.updateResult(result.config, { type, data: '' }); + fixture.detectChanges(); + + expect(element.querySelectorAll(containers)).toHaveLength(0); + expect(element.querySelectorAll('img')).toHaveLength(type === DatasetType.SVG ? 1 : 0); + } + ); +}); diff --git a/zeppelin-web-angular/src/app/pages/workspace/share/result/result.component.ts b/zeppelin-web-angular/src/app/pages/workspace/share/result/result.component.ts index 1b2ef5c9f38..ba73c32c260 100644 --- a/zeppelin-web-angular/src/app/pages/workspace/share/result/result.component.ts +++ b/zeppelin-web-angular/src/app/pages/workspace/share/result/result.component.ts @@ -109,6 +109,8 @@ export class NotebookParagraphResultComponent implements OnInit, AfterViewInit, @ViewChild(CdkPortalOutlet, { static: false }) portalOutlet!: CdkPortalOutlet; private destroy$ = new Subject(); + private outputResetVersion = 0; + private destroyed = false; datasetType = DatasetType; angularComponent: DynamicTemplate | null = null; innerHTML: string | SafeHtml = ''; @@ -332,6 +334,14 @@ export class NotebookParagraphResultComponent implements OnInit, AfterViewInit, renderDefaultDisplay() { this.frontEndError = ''; + if (this.result.type === DatasetType.SVG || this.result.type === DatasetType.NULL) { + this.outputResetVersion++; + this.angularComponent = null; + this.destroyVisualizations(); + if (this.portalOutlet?.hasAttached()) { + this.portalOutlet.detach(); + } + } switch (this.result.type) { case DatasetType.TABLE: this.renderGraph(); @@ -345,9 +355,21 @@ export class NotebookParagraphResultComponent implements OnInit, AfterViewInit, case DatasetType.IMG: this.renderImg(); break; + case DatasetType.SVG: + this.renderSvg(); + break; case DatasetType.ANGULAR: this.renderAngular(); break; + case DatasetType.NULL: + break; + case DatasetType.NETWORK: + // NULL has no display; NETWORK has no renderer in the Angular UI. + break; + default: { + const _unhandled: never = this.result.type; + break; + } } this.cdr.detectChanges(); } @@ -363,13 +385,20 @@ export class NotebookParagraphResultComponent implements OnInit, AfterViewInit, } renderAngular(): void { + const resetVersion = this.outputResetVersion; this.runtimeCompilerService .createAndCompileTemplate(this.id, this.result.data) .then(data => { + if (resetVersion !== this.outputResetVersion) { + return; + } this.angularComponent = data; this.cdr.markForCheck(); }) .catch(error => { + if (resetVersion !== this.outputResetVersion) { + return; + } this.angularComponent = null; this.frontEndError = error.message; this.cdr.markForCheck(); @@ -410,6 +439,10 @@ export class NotebookParagraphResultComponent implements OnInit, AfterViewInit, this.imgData = this.sanitizer.bypassSecurityTrustUrl(`data:image/png;base64,${this.result.data}`); } + renderSvg(): void { + this.imgData = `data:image/svg+xml;charset=utf-8,${encodeURIComponent(this.result.data)}`; + } + setGraphConfig() { const config = this.config; if (!config || !config.graph) { @@ -448,6 +481,11 @@ export class NotebookParagraphResultComponent implements OnInit, AfterViewInit, if (visualizationItem.isClassic) { // Classic visualization - delegate to ClassicVisualizationService const targetElementId = `p${this.id}_${config.graph.mode}`; + const resetVersion = this.outputResetVersion; + const isCurrentRender = () => + !this.destroyed && + resetVersion === this.outputResetVersion && + targetElementId === `p${this.id}_${this.config?.graph?.mode}`; const emitter = (c: GraphConfig) => { if (!this.config) { throw new Error('config is not defined'); @@ -457,8 +495,22 @@ export class NotebookParagraphResultComponent implements OnInit, AfterViewInit, }; this.classicVisualizationService - .createClassicVisualization(visualizationItem.Class, targetElementId, config.graph, this.tableData, emitter) + .createClassicVisualization( + visualizationItem.Class, + targetElementId, + config.graph, + this.tableData, + emitter, + isCurrentRender + ) .then(classicInstance => { + if (!classicInstance) { + return; + } + if (!isCurrentRender()) { + this.classicVisualizationService.destroyInstance(targetElementId, false, classicInstance); + return; + } visualizationItem.instance = classicInstance; this.cdr.markForCheck(); }) @@ -590,6 +642,7 @@ export class NotebookParagraphResultComponent implements OnInit, AfterViewInit, } ngOnDestroy(): void { + this.destroyed = true; this.destroyVisualizations(); this.classicVisualizationService.destroyAllInstances(true); this.destroy$.next(); diff --git a/zeppelin-web-angular/src/app/services/classic-visualization.service.spec.ts b/zeppelin-web-angular/src/app/services/classic-visualization.service.spec.ts new file mode 100644 index 00000000000..cd27b4ca55b --- /dev/null +++ b/zeppelin-web-angular/src/app/services/classic-visualization.service.spec.ts @@ -0,0 +1,101 @@ +/* + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { HttpClient } from '@angular/common/http'; +import { Injector } from '@angular/core'; +import { HeliumClassicVisualization, HeliumClassicVisualizationConstructor } from '@zeppelin/interfaces'; +import { GraphConfig } from '@zeppelin/sdk'; +import { TableData } from '@zeppelin/visualization'; +import { afterEach, describe, expect, it, vi } from 'vitest'; + +import { AngularDragDropService } from './angular-drag-drop.service'; +import { BootstrapCompatibilityService } from './bootstrap-compatibility.service'; +import { ClassicVisualizationService } from './classic-visualization.service'; +import { TableDataAdapterService } from './table-data-adapter.service'; + +const service = () => { + const injectBootstrapStyles = vi.fn(); + const instance = new ClassicVisualizationService( + {} as Injector, + {} as TableDataAdapterService, + {} as HttpClient, + {} as AngularDragDropService, + { injectBootstrapStyles } as unknown as BootstrapCompatibilityService + ); + return { instance, injectBootstrapStyles }; +}; + +afterEach(() => { + document.body.replaceChildren(); + vi.useRealTimers(); +}); + +describe('classic visualization cancellation', () => { + it('does not destroy a newer instance when an obsolete completion cleans up the same target', () => { + const { instance } = service(); + const destroy = vi.fn(); + const current = { destroy } as unknown as HeliumClassicVisualization; + const registry = instance as unknown as { + activeInstanceInfos: Map; + }; + registry.activeInstanceInfos.set('visualization', { instance: current }); + + instance.destroyInstance('visualization', false, {} as HeliumClassicVisualization); + + expect(destroy).not.toHaveBeenCalled(); + expect(registry.activeInstanceInfos.get('visualization')?.instance).toBe(current); + }); + it('cancels before allocation when its resolved target is removed before the await resumes', async () => { + const { instance, injectBootstrapStyles } = service(); + const target = document.createElement('div'); + target.id = 'visualization'; + document.body.appendChild(target); + let active = true; + const constructor = vi.fn() as unknown as HeliumClassicVisualizationConstructor; + const pending = instance.createClassicVisualization( + constructor, + target.id, + new GraphConfig(), + new TableData(), + vi.fn(), + () => active + ); + + target.remove(); + active = false; + + await expect(pending).resolves.toBeUndefined(); + expect(injectBootstrapStyles).not.toHaveBeenCalled(); + expect(constructor).not.toHaveBeenCalled(); + }); + + it('stops polling without an error when a pending target is cancelled', async () => { + vi.useFakeTimers(); + const { instance, injectBootstrapStyles } = service(); + let active = true; + const pending = instance.createClassicVisualization( + vi.fn() as unknown as HeliumClassicVisualizationConstructor, + 'missing-target', + new GraphConfig(), + new TableData(), + vi.fn(), + () => active + ); + + active = false; + await vi.advanceTimersByTimeAsync(100); + + await expect(pending).resolves.toBeUndefined(); + expect(vi.getTimerCount()).toBe(0); + expect(injectBootstrapStyles).not.toHaveBeenCalled(); + }); +}); diff --git a/zeppelin-web-angular/src/app/services/classic-visualization.service.ts b/zeppelin-web-angular/src/app/services/classic-visualization.service.ts index ca2c6866d1d..dd011532a3e 100644 --- a/zeppelin-web-angular/src/app/services/classic-visualization.service.ts +++ b/zeppelin-web-angular/src/app/services/classic-visualization.service.ts @@ -100,11 +100,20 @@ export class ClassicVisualizationService { }; } - private waitForElement(elementId: string, maxRetries = 50, interval = 100): Promise { + private waitForElement( + elementId: string, + isCurrentRender: () => boolean, + maxRetries = 50, + interval = 100 + ): Promise { return new Promise((resolve, reject) => { let retries = 0; const checkElement = () => { + if (!isCurrentRender()) { + resolve(undefined); + return; + } const element = document.getElementById(elementId); if (element) { resolve(element); @@ -160,14 +169,18 @@ export class ClassicVisualizationService { config: GraphConfig, tableData: TableData, // eslint-disable-next-line @typescript-eslint/no-explicit-any - emitter: (config: any) => void - ): Promise { + emitter: (config: any) => void, + isCurrentRender: () => boolean + ): Promise { + // Wait for DOM element to be available + const targetElement = await this.waitForElement(targetElementId, isCurrentRender); + if (!targetElement || !isCurrentRender()) { + return undefined; + } + // Inject Bootstrap compatibility styles before creating visualization this.bootstrapCompatibilityService.injectBootstrapStyles(); - // Wait for DOM element to be available - const targetElement = await this.waitForElement(targetElementId); - // Clean up any existing instance for this element this.destroyInstance(targetElementId); @@ -367,9 +380,13 @@ export class ClassicVisualizationService { } } - destroyInstance(targetElementId: string, forceCleanBootstrap = false): void { + destroyInstance( + targetElementId: string, + forceCleanBootstrap = false, + expectedInstance?: HeliumClassicVisualization + ): void { const instanceInfo = this.activeInstanceInfos.get(targetElementId); - if (!instanceInfo) { + if (!instanceInfo || (expectedInstance && instanceInfo.instance !== expectedInstance)) { return; }