kunwp1 commented on code in PR #8539:
URL: https://github.com/apache/texera/pull/8539#discussion_r4055115618
##########
frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.ts:
##########
@@ -197,12 +197,18 @@ export class WorkflowResultExportService {
rowIndex: number,
columnIndex: number,
filename: string,
- exportAll: boolean = false, // if the user click export button on the top
bar (a.k.a menu),
- // we should export all operators, otherwise, only highlighted ones
- // which means export button is selected from context-menu
- destination: "dataset" | "local" = "dataset", // default to dataset
- unit: DashboardWorkflowComputingUnit | null // computing unit for cluster
setting
+ destination: "dataset" | "local",
+ unit: DashboardWorkflowComputingUnit | null, // computing unit for cluster
setting
+ // The operators this export covers. The caller resolves them: the dialog
already works out
+ // its own scope in order to report what a blocking dataset blocks, so it
says so here rather
+ // than leaving the scope to be worked out a second time, separately, from
a flag and the
+ // canvas -- two answers to one question that agree only for as long as
nobody edits one.
+ operatorIds: readonly string[]
): void {
+ // Copied now, not read later: the restriction analysis below is
asynchronous, and the canvas
+ // selection a caller may have handed us is the live array, so the scope
would otherwise be
+ // whatever is selected when the analysis answers rather than what was
asked for.
+ const scope = [...operatorIds];
this.computeRestrictionAnalysis()
Review Comment:
**Carry the same idea one step further: the dialog already holds the
analysis.**
`ResultExportationComponent.ngOnInit` subscribes to
`computeRestrictionAnalysis()` and stores the answer in `this.downloadability`.
Clicking export then calls this line, which issues `GET
.../{wid}/downloadability` a **second** time and blocks the export on it. The
dialog is the only caller of `exportWorkflowExecutionResult`, and
`performExport` already takes a `WorkflowResultDownloadability` parameter — so
the dialog can hand over the analysis exactly the way this PR now has it hand
over `operatorIds`.
That single change pays for itself three times:
1. One HTTP round trip disappears from the click path.
2. The dialog's banner and the service's error/warning notification stop
being computed from two separate responses that can disagree.
3. **It removes the "Export unavailable" flash** I raised on
`result-exportation.component.ts` — if the dialog can only export once it holds
`downloadability`, the undefined window stops being a state the export path has
to survive.
Your own comment on the signature above argues for exactly this: "two
answers to one question that agree only for as long as nobody edits one." That
reasoning applies to the analysis as much as to the scope.
The duplicate request is pre-existing — but this diff is precisely where it
becomes easy to delete.
##########
frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.ts:
##########
@@ -197,12 +197,18 @@ export class WorkflowResultExportService {
rowIndex: number,
Review Comment:
**Two cheap wins in this file, both pointed at by the change you already
made.**
**1. `getWorkflow().wid` serializes the whole workflow to read one integer**
(line 264 in `performExport`, and line 132 in `computeRestrictionAnalysis`).
`getWorkflow()` spreads the metadata over `getWorkflowContent()`, which calls
`getAllOperators()` twice, plus `getAllLinks()`, `getAllCommentBoxes()`, and a
per-operator position lookup — then everything but `.wid` is thrown away. `wid:
number | undefined` is declared directly on `WorkflowMetadata`, so
`getWorkflowMetadata().wid` is the same value at zero graph cost.
**2. `updateExportAvailabilityFlags` (lines 174-177) still spells it the
long way.** This PR replaced `.getAllOperators().map(op => op.operatorID)` with
`getAllOperatorIDs()` over in the dialog — the identical expression survives
twenty lines above the method you rewrote. It also matters more here than
there: this method is subscribed to the highlight **and** unhighlight streams,
and `unhighlightAll` emits one event per operator, so clearing an N-operator
selection costs N full `toJSON()` materializations of every operator. The
dialog's call site was a cold path; this one runs per interaction.
##########
frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.ts:
##########
@@ -197,12 +197,18 @@ export class WorkflowResultExportService {
rowIndex: number,
columnIndex: number,
filename: string,
- exportAll: boolean = false, // if the user click export button on the top
bar (a.k.a menu),
- // we should export all operators, otherwise, only highlighted ones
- // which means export button is selected from context-menu
- destination: "dataset" | "local" = "dataset", // default to dataset
- unit: DashboardWorkflowComputingUnit | null // computing unit for cluster
setting
+ destination: "dataset" | "local",
+ unit: DashboardWorkflowComputingUnit | null, // computing unit for cluster
setting
+ // The operators this export covers. The caller resolves them: the dialog
already works out
+ // its own scope in order to report what a blocking dataset blocks, so it
says so here rather
+ // than leaving the scope to be worked out a second time, separately, from
a flag and the
+ // canvas -- two answers to one question that agree only for as long as
nobody edits one.
+ operatorIds: readonly string[]
): void {
+ // Copied now, not read later: the restriction analysis below is
asynchronous, and the canvas
+ // selection a caller may have handed us is the live array, so the scope
would otherwise be
+ // whatever is selected when the analysis answers rather than what was
asked for.
+ const scope = [...operatorIds];
Review Comment:
**This copy is correct and needed — but the contract it is working around is
broken one level down.**
`JointGraphWrapper.getCurrentHighlightedOperatorIDs()` carries this
docstring:
> "The returned array is not the original one so that other
services/components can't modify it directly."
Its body is `return this.currentHighlightedOperators;` — the original, which
`highlightElement` and `unhighlightElement` mutate in place. Your copy is the
right call given that, but it only protects this one consumer. The dialog's own
`exportableOperatorIds`, `blockedOperatorIds` and `blockingDatasetLabels`
getters still read the live array across the async analysis, and roughly thirty
other call sites of the accessor are equally exposed.
Honouring the docstring at the source — `return
[...this.currentHighlightedOperators];` — fixes all of them and makes this
local copy unnecessary. That is a separate one-line PR against its own issue,
not something to fold in here; worth filing while it is in view.
##########
frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.html:
##########
@@ -193,7 +193,11 @@ <h5 class="rightAlign"><span
[innerHTML]="compare(column.header, 'other')"></spa
<ng-container *ngSwitchDefault>{{ column.getCell(row)
}}</ng-container>
</ng-container>
</span>
+ <!-- Not rendered when result export is switched off for the
deployment: every action
+ behind it returns without sending a request, so the button
would be there and do
+ nothing. The top menu and the context menu already honour the
same switch. -->
<button
Review Comment:
**Gating on the config switch alone leaves this button live during
execution, when the other two entry points are not.**
The sibling entry points both fold in execution state as well as the switch:
| Entry point | Gate |
| --- | --- |
| Top menu | `!exportExecutionResultEnabled \|\| !hasResultToExport`, where
the flag includes `isNotInExecution(...)` |
| Context menu | `hasResultToExportOnHighlightedOperators &&
exportExecutionResultEnabled && !hasHighlightedLinks()` |
| This button (new) | `exportExecutionResultEnabled` only |
So mid-execution the menu export is greyed out and this button is not. The
PR adds a third export scope — a named operator — without a matching
availability concept, which is a special case layered on shared infrastructure.
The deeper change is one `hasResultToExport(operatorIds: readonly string[])`
on `WorkflowResultExportService`; the two existing `BehaviorSubject` flags
become thin callers of it, and this button gets the same answer as the others
for free.
Separately, the expression itself should be a field: this sits inside the
row × column `*ngFor`, so it is a service getter call per cell per
change-detection cycle for a value fixed for the frame's lifetime. `ngOnInit`
already does exactly this one line away — `this.columnLimit =
this.guiConfigService.env.limitColumns;`. Hoisting it also removes the need to
widen `guiConfigService` to `public` at all.
##########
frontend/src/app/workspace/service/workflow-result-export/workflow-result-export.service.spec.ts:
##########
@@ -234,7 +234,7 @@ describe("WorkflowResultExportService", () => {
const download = stubDownloadService();
texeraGraphSpy.getAllOperators.mockReturnValue([{ operatorID: "op1" }]
as any);
Review Comment:
**Removing `exportAll` orphaned fourteen of these stubs — this diff edited
the line under each one and left them.**
`performExport` no longer reads the graph at all: the scope arrives as a
parameter, and the only `workflowActionService` call left in it is
`getWorkflow().wid`. The constructor path is already covered by the
`beforeEach` stub, and the highlight streams are `of()`, so
`texeraGraphSpy.getAllOperators.mockReturnValue(...)` cannot influence any of
these tests.
Same dead line at 247, 262, 358, 373, 402, 417, 444, 459, 470, 480, 493,
506, 518. Deleting them is the point of the refactor made visible — they
currently imply the export path depends on graph contents, which is exactly the
coupling this PR removed.
One deliberate exception: keep the stub at line 288. There it is
load-bearing, because the test proves the service ignores the graph, and the
comment says so.
##########
frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.spec.ts:
##########
@@ -92,6 +94,13 @@ describe("ResultTableFrameComponent", () => {
const queryParams = (pageIndex: number): NzTableQueryParams => ({ pageIndex,
pageSize: 5, sort: [], filter: [] });
+ // `commonTestProviders` supplies MockGuiConfigService, which is what the
component receives,
Review Comment:
**This comment is right, and it means the inline stub above it is dead.**
Lines 123-130 provide `{ provide: GuiConfigService, useValue: { env: {
limitColumns: GUI_CONFIG_LIMIT } } }`, and line 131 then spreads
`...commonTestProviders`. The last provider for a token wins, so
`MockGuiConfigService` is what the component gets — precisely as your comment
states, and as `setExport` requires (the inline literal has no `setConfig`, so
it would throw).
The consequence is worth a look: `it("should set columnLimit from
gui-config")` passes only because `MockGuiConfigService`'s default
`limitColumns` also happens to be `15`, the same number as `GUI_CONFIG_LIMIT`.
It reads as if it were asserting against the inline stub; it is not.
Delete lines 123-130 and drive that test through `setConfig({ limitColumns:
... })`, the way your new `setExport` already does.
##########
frontend/src/app/workspace/component/result-exportation/result-exportation.component.spec.ts:
##########
@@ -826,15 +869,125 @@ describe("ResultExportationComponent (context-menu
source with default modal dat
expect(component.blockedOperatorIds).toEqual([]);
});
- it("exports highlighted operators only (exportAll === false) for a
context-menu trigger", () => {
+ it("exports to the destination it was given for a context-menu trigger", ()
=> {
const exportService = TestBed.inject(WorkflowResultExportService)
.exportWorkflowExecutionResult as unknown as ReturnType<typeof vi.fn>;
component.onClickExportResult("local");
expect(exportService).toHaveBeenCalledTimes(1);
const args = exportService.mock.calls[0];
- expect(args[6]).toBe(false); // exportAll is false because sourceTriggered
!== "menu"
- expect(args[7]).toBe("local");
+ expect(args[6]).toBe("local"); // destination
+ });
+
+ // The context menu names no operators of its own, so the dialog resolves
the selection and
+ // hands that over. It does not pass nothing and leave the export to read
the canvas a second
+ // time, which would let what is exported differ from what the dialog
reported on.
+ it("hands over the selection it resolved, not an empty scope", () => {
+ const exportService = TestBed.inject(WorkflowResultExportService)
+ .exportWorkflowExecutionResult as unknown as ReturnType<typeof vi.fn>;
+
+ component.onClickExportResult("local");
+
+ expect(exportService.mock.calls[0][8]).toEqual(["hl-1", "hl-2"]);
+ });
+});
+
+// A result cell opens this dialog naming the one operator whose results it
shows. Both the
+// dialog's own checks and the export it triggers have to use that operator:
the cell is also
+// mounted on the Form View, where nothing is selected until the user clicks a
step, so reading
+// the selection there answered "no operators" and the export sent nothing.
+describe("ResultExportationComponent (a caller that names its operators)", ()
=> {
Review Comment:
**This is the third byte-identical copy of the same ~55-line provider array,
and one of its three tests is already covered.**
The blocks at lines 66, 820 and 900 differ only in `NZ_MODAL_DATA`, the two
graph return values, and `determineOutputTypes`. The PR pays for that
immediately: adding `getAllOperatorIDs` required the same edit in all three
(lines 93, 825, 940), and `setAllOperators(ids)` is separately declared
byte-identical at lines 217, 275 and 417 — all three also edited.
Of this block's three tests, `it("hands the named operator to the export")`
at line 987 asserts the same getter and the same argument index as `it("is the
operators the caller named, whatever the canvas has selected")` at line 176,
which reaches it in three lines with no new TestBed. Only `it("reads an absent
source trigger as an empty string")` genuinely needs its own `NZ_MODAL_DATA`.
A file-scope `configureExportDialog(modalData, overrides?)` that all three
describes call would make the next constructor dependency one edit instead of
three.
##########
frontend/src/app/workspace/component/result-panel/result-table-frame/result-table-frame.component.ts:
##########
@@ -464,17 +464,27 @@ export class ResultTableFrameComponent implements OnInit,
OnChanges {
}
downloadData(data: any, rowIndex: number, columnIndex: number, columnName:
string): void {
+ // A cell belongs to the operator whose results this frame is showing.
Without one there is
+ // nothing to scope an export to, and the dialog would open only to export
nothing.
+ if (!this.operatorId) {
Review Comment:
**This is the sixth copy of a guard that exists only because the input is
declared optional.**
`@Input() operatorId?: string` (line 90) forces `if (!this.operatorId)` at
lines 171, 192, 329, 394, 421 and now 469, plus two positive forms at 138 and
215. Both mounts always supply a `string`: `result-panel.component.ts` passes
it via `componentInputs: { operatorId }` from `displayResult(operatorId:
string)`, and `workflow-form.component.html` binds `[operatorId]="id"`. The
frame is inert without one anyway — `ngOnChanges`, `ngOnInit` and every stream
handler bail.
`@Input({ required: true }) operatorId!: string` deletes all eight checks
and makes "a cell with no operator" stop being a state anyone has to remember.
Your new test at line 670 pins a state no caller can produce.
The guard does earn its place today for one narrow reason — without it,
`[this.operatorId]` types as `(string | undefined)[]`. Making the input
required solves that at the source instead.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
