Copilot commented on code in PR #3772:
URL: https://github.com/apache/texera/pull/3772#discussion_r2381238224
##########
core/amber/src/main/scala/edu/uci/ics/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResource.scala:
##########
@@ -102,6 +103,140 @@ object WorkflowExecutionsResource {
}
}
+ private case class RestrictedDataset(ownerEmail: String, datasetName:
String) {
+ def cacheKey: (String, String) = (ownerEmail.toLowerCase,
datasetName.toLowerCase)
+ def label: String = s"$datasetName ($ownerEmail)"
Review Comment:
[nitpick] The `RestrictedDataset` case class uses `toLowerCase` for cache
keys but the original case is preserved in labels. This could lead to
inconsistencies. Consider documenting this behavior or using a consistent
casing strategy throughout.
```suggestion
/**
* Represents a dataset with owner and name, normalized to lower case for
consistency.
* Both the cache key and label use lowercased values to avoid casing
inconsistencies.
*/
private case class RestrictedDataset(ownerEmail: String, datasetName:
String) {
val normalizedOwnerEmail: String = ownerEmail.toLowerCase
val normalizedDatasetName: String = datasetName.toLowerCase
def cacheKey: (String, String) = (normalizedOwnerEmail,
normalizedDatasetName)
def label: String = s"$normalizedDatasetName ($normalizedOwnerEmail)"
```
##########
core/gui/src/app/workspace/service/workflow-result-export/workflow-result-export.service.ts:
##########
@@ -60,34 +69,217 @@ export class WorkflowResultExportService {
this.workflowActionService.getJointGraphWrapper().getJointOperatorHighlightStream(),
this.workflowActionService.getJointGraphWrapper().getJointOperatorUnhighlightStream()
).subscribe(() => {
- // check if there are any results to export on highlighted operators
(either paginated or snapshot)
- this.hasResultToExportOnHighlightedOperators =
-
isNotInExecution(this.executeWorkflowService.getExecutionState().state) &&
- this.workflowActionService
- .getJointGraphWrapper()
- .getCurrentHighlightedOperatorIDs()
- .filter(
- operatorId =>
- this.workflowResultService.hasAnyResult(operatorId) ||
-
this.workflowResultService.getResultService(operatorId)?.getCurrentResultSnapshot()
!== undefined
- ).length > 0;
-
- // check if there are any results to export on all operators (either
paginated or snapshot)
- let staticHasResultToExportOnAllOperators =
-
isNotInExecution(this.executeWorkflowService.getExecutionState().state) &&
- this.workflowActionService
- .getTexeraGraph()
- .getAllOperators()
- .map(operator => operator.operatorID)
- .filter(
- operatorId =>
- this.workflowResultService.hasAnyResult(operatorId) ||
-
this.workflowResultService.getResultService(operatorId)?.getCurrentResultSnapshot()
!== undefined
- ).length > 0;
-
- // Notify subscribers of changes
-
this.hasResultToExportOnAllOperators.next(staticHasResultToExportOnAllOperators);
+ this.updateExportAvailabilityFlags();
+ });
+ }
+
+ private registerRestrictionRecomputeTriggers(): void {
+ const texeraGraph = this.workflowActionService.getTexeraGraph();
+ merge(
+ texeraGraph.getOperatorAddStream(),
+ texeraGraph.getOperatorDeleteStream(),
+ texeraGraph.getOperatorPropertyChangeStream(),
+ texeraGraph.getLinkAddStream(),
+ texeraGraph.getLinkDeleteStream(),
+ texeraGraph.getDisabledOperatorsChangedStream()
+ ).subscribe(() => {
+ this.runRestrictionAnalysis();
+ });
+ }
+
+ public refreshDatasetMetadata(): Observable<void> {
+ this.datasetListLoaded = false;
+ return this.datasetService.retrieveAccessibleDatasets().pipe(
+ take(1),
+ tap(datasets => {
+ this.datasetDownloadableMap.clear();
+ this.datasetLabelMap.clear();
+ datasets.forEach(dataset => {
+ const key = this.buildDatasetKey(dataset.ownerEmail,
dataset.dataset.name);
+ const isDownloadable = dataset.dataset.isDownloadable ||
dataset.isOwner;
+ this.datasetDownloadableMap.set(key, isDownloadable);
+ this.datasetLabelMap.set(key, `${dataset.dataset.name}
(${dataset.ownerEmail})`);
+ });
+ this.datasetListLoaded = true;
+ this.runRestrictionAnalysis();
+ }),
+ map(() => undefined),
+ catchError(() => {
+ this.datasetDownloadableMap.clear();
+ this.datasetLabelMap.clear();
+ this.datasetListLoaded = true;
+ this.runRestrictionAnalysis();
+ return of(undefined);
+ })
+ );
+ }
+
+ private buildDatasetKey(ownerEmail: string, datasetName: string): string {
+ return `${ownerEmail.toLowerCase()}::${datasetName.toLowerCase()}`;
+ }
+
+ private extractDatasetInfo(fileName: unknown): { key: string; label: string
} | null {
+ if (typeof fileName !== "string") {
+ return null;
+ }
+ const trimmed = fileName.trim();
+ if (!trimmed.startsWith("/")) {
+ return null;
+ }
+ try {
+ const { ownerEmail, datasetName } = parseFilePathToDatasetFile(trimmed);
+ if (!ownerEmail || !datasetName) {
+ return null;
+ }
+ const key = this.buildDatasetKey(ownerEmail, datasetName);
+ if (!this.datasetDownloadableMap.has(key)) {
+ return null;
+ }
+ const label = this.datasetLabelMap.get(key) ?? `${datasetName}
(${ownerEmail})`;
+ return { key, label };
+ } catch {
+ return null;
+ }
+ }
+
+ private runRestrictionAnalysis(): void {
+ if (!this.datasetListLoaded) {
+ this.restrictedOperatorMap.clear();
+ this.updateExportAvailabilityFlags();
+ return;
+ }
+
+ const texeraGraph = this.workflowActionService.getTexeraGraph();
+ const allOperators = texeraGraph.getAllOperators();
+ const operatorById = new Map(allOperators.map(op => [op.operatorID, op] as
const));
+ const enabledOperators = allOperators.filter(operator =>
!operator.isDisabled);
+ const datasetSources: Array<{ operatorId: string; label: string }> = [];
+
+ enabledOperators.forEach(operator => {
+ const datasetInfo =
this.extractDatasetInfo(operator.operatorProperties?.fileName);
+ if (!datasetInfo) {
+ return;
+ }
+ const isDownloadable = this.datasetDownloadableMap.get(datasetInfo.key);
+ if (isDownloadable === false) {
+ datasetSources.push({ operatorId: operator.operatorID, label:
datasetInfo.label });
+ }
+ });
+
+ const restrictions = new Map<string, Set<string>>();
+
+ if (datasetSources.length === 0) {
+ this.restrictedOperatorMap = restrictions;
+ this.updateExportAvailabilityFlags();
+ return;
+ }
+
+ const adjacency = new Map<string, string[]>();
+ texeraGraph.getAllLinks().forEach(link => {
+ const sourceId = link.source.operatorID;
+ const targetId = link.target.operatorID;
+ const sourceOperator = operatorById.get(sourceId);
+ const targetOperator = operatorById.get(targetId);
+ if (!sourceOperator || !targetOperator) {
+ return;
+ }
+ if (sourceOperator.isDisabled || targetOperator.isDisabled) {
+ return;
+ }
+ const neighbors = adjacency.get(sourceId);
+ if (neighbors) {
+ neighbors.push(targetId);
+ } else {
+ adjacency.set(sourceId, [targetId]);
+ }
+ });
+
+ const queue: Array<{ operatorId: string; datasets: Set<string> }> = [];
+ datasetSources.forEach(source => {
+ queue.push({ operatorId: source.operatorId, datasets: new
Set([source.label]) });
+ });
+
+ while (queue.length > 0) {
+ const current = queue.shift()!;
+ const existing = restrictions.get(current.operatorId) ?? new
Set<string>();
+ let updated = false;
+ current.datasets.forEach(label => {
+ if (!existing.has(label)) {
+ existing.add(label);
+ updated = true;
+ }
+ });
+ if (updated || !restrictions.has(current.operatorId)) {
+ restrictions.set(current.operatorId, existing);
+ const neighbors = adjacency.get(current.operatorId) ?? [];
+ neighbors.forEach(nextOperatorId => {
+ queue.push({ operatorId: nextOperatorId, datasets: new Set(existing)
});
+ });
+ }
+ }
+
+ this.restrictedOperatorMap = restrictions;
+ this.updateExportAvailabilityFlags();
+ }
+
+ private updateExportAvailabilityFlags(): void {
+ const executionIdle =
isNotInExecution(this.executeWorkflowService.getExecutionState().state);
+
+ const highlightedOperators =
this.workflowActionService.getJointGraphWrapper().getCurrentHighlightedOperatorIDs();
+
+ const highlightedHasResult = highlightedOperators.some(
+ operatorId =>
+ this.workflowResultService.hasAnyResult(operatorId) ||
+
this.workflowResultService.getResultService(operatorId)?.getCurrentResultSnapshot()
!== undefined
+ );
+
+ this.hasResultToExportOnHighlightedOperators = executionIdle &&
highlightedHasResult;
+
+ const allOperatorIds = this.workflowActionService
+ .getTexeraGraph()
+ .getAllOperators()
+ .map(operator => operator.operatorID);
+
+ const hasAnyResult =
+ executionIdle &&
+ allOperatorIds.some(
+ operatorId =>
+ this.workflowResultService.hasAnyResult(operatorId) ||
+
this.workflowResultService.getResultService(operatorId)?.getCurrentResultSnapshot()
!== undefined
+ );
+
+ this.hasResultToExportOnAllOperators.next(hasAnyResult);
+ }
+
+ private isOperatorEligibleForExport(operatorId: string): boolean {
+ if (this.restrictedOperatorMap.has(operatorId)) {
+ return false;
+ }
+ return (
+ this.workflowResultService.hasAnyResult(operatorId) ||
+
this.workflowResultService.getResultService(operatorId)?.getCurrentResultSnapshot()
!== undefined
+ );
+ }
Review Comment:
The method `isOperatorEligibleForExport` is defined but never used in the
codebase. Consider removing it or using it in place of the inline logic in
other methods to reduce code duplication.
##########
core/amber/src/main/scala/edu/uci/ics/texera/web/resource/dashboard/user/workflow/WorkflowExecutionsResource.scala:
##########
@@ -102,6 +103,140 @@ object WorkflowExecutionsResource {
}
}
+ private case class RestrictedDataset(ownerEmail: String, datasetName:
String) {
+ def cacheKey: (String, String) = (ownerEmail.toLowerCase,
datasetName.toLowerCase)
+ def label: String = s"$datasetName ($ownerEmail)"
+ }
+
+ private def parseDatasetPath(path: String): Option[RestrictedDataset] = {
+ if (path == null) {
+ return None
+ }
+ val trimmed = path.trim
+ if (!trimmed.startsWith("/")) {
+ return None
+ }
+ val segments = trimmed.split("/").filter(_.nonEmpty)
+ if (segments.length < 4) {
+ return None
+ }
+ val ownerEmail = segments(0)
+ val datasetName = segments(1)
+ Some(RestrictedDataset(ownerEmail, datasetName))
+ }
+
+ private def lookupDatasetDownloadable(
+ dataset: RestrictedDataset,
+ cache: mutable.Map[(String, String), Option[Boolean]]
+ ): Option[Boolean] = {
+ cache.getOrElseUpdate(
+ dataset.cacheKey, {
+ val record = context
+ .select(DATASET.IS_DOWNLOADABLE)
+ .from(DATASET)
+ .join(USER)
+ .on(DATASET.OWNER_UID.eq(USER.UID))
+ .where(
+ USER.EMAIL
+ .equalIgnoreCase(dataset.ownerEmail)
+ .and(DATASET.NAME.equalIgnoreCase(dataset.datasetName))
+ )
+ .fetchOne()
+ if (record == null) {
+ None
+ } else {
+ Option(record.value1())
+ }
+ }
+ )
+ }
+
+ private def computeDatasetRestrictionMap(
+ wid: Int,
+ currentUser: UserPojo
+ ): Map[String, Set[RestrictedDataset]] = {
+ val workflowRecord = context
+ .select(WORKFLOW.CONTENT)
+ .from(WORKFLOW)
+ .where(WORKFLOW.WID.eq(wid))
+ .fetchOne()
+
+ if (workflowRecord == null) {
+ return Map.empty
+ }
+
+ val content = workflowRecord.value1()
+ if (content == null || content.isEmpty) {
+ return Map.empty
+ }
+
+ val rootNode =
+ try {
+ objectMapper.readTree(content)
+ } catch {
+ case _: Exception => return Map.empty
+ }
Review Comment:
The catch block catches all exceptions with a wildcard pattern, which can
hide important parsing errors. Consider catching more specific exceptions like
`JsonProcessingException` to avoid masking unexpected errors.
##########
core/gui/src/app/workspace/service/workflow-result-export/workflow-result-export.service.ts:
##########
@@ -60,34 +69,217 @@ export class WorkflowResultExportService {
this.workflowActionService.getJointGraphWrapper().getJointOperatorHighlightStream(),
this.workflowActionService.getJointGraphWrapper().getJointOperatorUnhighlightStream()
).subscribe(() => {
- // check if there are any results to export on highlighted operators
(either paginated or snapshot)
- this.hasResultToExportOnHighlightedOperators =
-
isNotInExecution(this.executeWorkflowService.getExecutionState().state) &&
- this.workflowActionService
- .getJointGraphWrapper()
- .getCurrentHighlightedOperatorIDs()
- .filter(
- operatorId =>
- this.workflowResultService.hasAnyResult(operatorId) ||
-
this.workflowResultService.getResultService(operatorId)?.getCurrentResultSnapshot()
!== undefined
- ).length > 0;
-
- // check if there are any results to export on all operators (either
paginated or snapshot)
- let staticHasResultToExportOnAllOperators =
-
isNotInExecution(this.executeWorkflowService.getExecutionState().state) &&
- this.workflowActionService
- .getTexeraGraph()
- .getAllOperators()
- .map(operator => operator.operatorID)
- .filter(
- operatorId =>
- this.workflowResultService.hasAnyResult(operatorId) ||
-
this.workflowResultService.getResultService(operatorId)?.getCurrentResultSnapshot()
!== undefined
- ).length > 0;
-
- // Notify subscribers of changes
-
this.hasResultToExportOnAllOperators.next(staticHasResultToExportOnAllOperators);
+ this.updateExportAvailabilityFlags();
+ });
+ }
+
+ private registerRestrictionRecomputeTriggers(): void {
+ const texeraGraph = this.workflowActionService.getTexeraGraph();
+ merge(
+ texeraGraph.getOperatorAddStream(),
+ texeraGraph.getOperatorDeleteStream(),
+ texeraGraph.getOperatorPropertyChangeStream(),
+ texeraGraph.getLinkAddStream(),
+ texeraGraph.getLinkDeleteStream(),
+ texeraGraph.getDisabledOperatorsChangedStream()
+ ).subscribe(() => {
+ this.runRestrictionAnalysis();
+ });
+ }
+
+ public refreshDatasetMetadata(): Observable<void> {
+ this.datasetListLoaded = false;
+ return this.datasetService.retrieveAccessibleDatasets().pipe(
+ take(1),
+ tap(datasets => {
+ this.datasetDownloadableMap.clear();
+ this.datasetLabelMap.clear();
+ datasets.forEach(dataset => {
+ const key = this.buildDatasetKey(dataset.ownerEmail,
dataset.dataset.name);
+ const isDownloadable = dataset.dataset.isDownloadable ||
dataset.isOwner;
+ this.datasetDownloadableMap.set(key, isDownloadable);
+ this.datasetLabelMap.set(key, `${dataset.dataset.name}
(${dataset.ownerEmail})`);
+ });
+ this.datasetListLoaded = true;
+ this.runRestrictionAnalysis();
+ }),
+ map(() => undefined),
+ catchError(() => {
+ this.datasetDownloadableMap.clear();
+ this.datasetLabelMap.clear();
+ this.datasetListLoaded = true;
+ this.runRestrictionAnalysis();
+ return of(undefined);
+ })
+ );
+ }
+
+ private buildDatasetKey(ownerEmail: string, datasetName: string): string {
+ return `${ownerEmail.toLowerCase()}::${datasetName.toLowerCase()}`;
Review Comment:
[nitpick] The delimiter `::` used for building dataset keys could conflict
with actual email addresses or dataset names containing this sequence. Consider
using a more unique delimiter or a different key generation strategy.
```suggestion
// Use JSON.stringify to avoid delimiter collision
return JSON.stringify({
ownerEmail: ownerEmail.toLowerCase(),
datasetName: datasetName.toLowerCase(),
});
```
--
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]
