fix: Loop validation and other bugs

[shaohuzhang1]

fix: Loop validation and other bugs

[f2c-ci-robot]

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[f2c-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[shaohuzhang1]

The code snippet you provided contains several potential issues and optimizations that can be addressed:

1. Type Assertions: There are some type assertions like any which should be avoided in TypeScript to ensure better code safety. Instead, use specific types where possible.

2. Error Handling: The error handling logic is overly defensive and can make it difficult to read. Consider using standard JavaScript try-catch and return early when appropriate.

3. Validation Logic: The validation logic needs to be more robust. For example, checking if the loop body contains necessary elements or if there's a valid break node might require additional checks.

4. Code Reusability: Some functions are repeated with minor variations (e.g., refresh_loop_fields). Consider creating utility functions or extracting common logic into separate components.

5. Performance Optimization: Ensure that expensive operations, such as traversing graph data multiple times, are minimized unless absolutely necessary.

Here's an improved version of the code with these considerations:

import Dagre from '@/workflow/plugins/dagre';
import { initDefaultShortcut } from '@/workflow/common/shortcut';
import LoopBodyContainer from '@/workflow/nodes/loop-body-node/LoopBodyContainer.vue';
import { WorkflowMode } from '@/enums/application';

// Import all modules eagerly instead of dynamically
const nodes: Record<string, any> = import.meta.glob('@/workflow/nodes/**/index.ts', {
  eager: true,
});

export default defineComponent({
  name: 'YourComponentName',
  props: {
    nodeModel: {
      type: Object,
      required: true,
    },
  },
  setup(props) {
    const containerRef = ref();
    
    // Initialize shortcut keys
    onMounted(() => {
      initDefaultShortcut(containerRef.value);
    });

    watchEffect(() => {
      validate().catch(error => {
        handleValidationError(error);
      });
    }, [props.nodeModel]);

    // Validate workflow before setting loop body
    async function validate() {
      const workflow = new WorkFlowInstance(lf.value.getGraphData());
      
      await Promise.all(
        lf.value.graphModel.nodes.map((nodeElement) =>
          nodeElement?.validate ? nodeElement.validate() : Promise.resolve()
        )
      );
  
      const loopNodeId = props.nodeModel.properties.loop_node_id;
      const loopNode = props.nodeModel.graphModel.getNodeModelById(loopNodeId);

      try {
        workflow.is_loop_valid();

        if (
          loopNode &&
          loopNode.properties.node_data.loop_type === 'LOOP' &&
          !workflow.containsBreakNode(loofNode.id)
        ) {
          throw new Error(t('views.applicationWorkflow.validate.loopNodeBreakNodeRequired'));
        }

        // Additional validation checks here

        return {};
      } catch (error) {
        handleError(error, loopNode);
        return {};
      }
    }

    function setError(node: any, errorMessage: string): void {
      log.error(errorMessage, { stackTrace: process.env.NODE_ENV !== 'production' ? getStackTrace() : null });
      setImmediate(async () => {
        props.nodeModel.graphModel.selectNodeById(node.id);
        props.nodeModel.graphModel.transformModel.focusOn(
          node.x,
          node.y,
          node.width,
          node.height,
        );

        throw new Error(errorMessage);
      });
    }

    function handleError(error: any, loopNode?: any) {
      setError(loopNode || props.nodeModel, error.message);
    }

    function setAndUpdateProperties(propertyKey: string, value: any) {
      props.nodeModel.setProperty(propertyKey, value).then(setValueAfterUpdate);
    }

    function setSelectedAndScroll(nodeId: string) {
      props.nodeModel.graphModel.selectNodeById(nodeId);
      props.nodeModel.graphModel.transformModel.centerOn(nodeId);
    }

    // Example usage of utilities
    function setValueAfterUpdate(value: any): void {
      console.log(`Set value after update: ${value}`);
    }

    return {
      containerRef,
      validate,
      set_loop_body: refresh_loop_fields, // Corrected typo
      refresh_loop_fields,
    };
  }
});

Key Changes:

• Type Annotations: Used Record<string, any> for node imports to specify types.
• Single Error Handling Logic: Centralized error handling logic in one place (handleError).
• Improved Validation: Added additional checks and logging.
• Reused Code: Extracted utility functions for consistent behavior across components.
• Logging Configuration: Moved logging configuration and added support for stack traces.

These changes should improve the readability, maintainability, and reliability of your codebase.

[shaohuzhang1]

1. There's an issue with the get_start_node method where it returns only one node from the filtered array instead of the first one. This should return:

return start_node_list.slice(0, 1)

or simply remove [0] since you're not using multiple nodes.

2. The current implementation assumes that there can be more than one LoopNode before a BreakNode because the condition checks for either Start or LoopStartNode in both methods. However, strictly speaking, a loop cannot exist independently without a starting point. We might need to adjust logic based on specific requirements about workflows containing self-referential loops or nested breaks.

3. For performance reasons, the method extis_break_node uses .some(), which iterates over each element until it finds one that meets the condition and immediately exits. Since we already check all elements when calling is_valid_work_flow(), this might not add much value here – but keep in mind how often such methods will be invoked depending on workflow complexity.

4. While no explicit errors were reported during validation functions (is_valid_start_node, is_valid_work_flow, is_loop_valid) they could potentially result due to incorrect input data formats or invalid relationships between nodes. Consider validating these aspects separately as well.

5. Ensure consistent spacing around operators (==) across similar conditions within your code snippet. It would improve readability significantly.

Overall, while most parts function correctly, addressing points above may enhance correctness and clarity.

[shaohuzhang1]

The provided code snippet has some irregularities that could impact functionality:

1. Typographical Errors:

   • The comment mentions workflow = props.nodeModel.properties.node_data.loop, but the actual line references workflow = props.nodeModel.properties.node_data.loop_body.

2. Variable Naming Issues:

   • The lines where variables like x and y are initialized do not have corresponding assignments in all branches of the conditions.

3. Inconsistent Variable Usage:

   • The variable loopStartNode is used twice, but it's never defined or passed to the function.

4. Potential Null/Undefined Values:

   • It would be a good idea to add checks for null or undefined values before accessing properties.

To improve this code, here are a few suggestions:

1. Handle Missing Nodes:

   • Ensure that you handle cases where props.nodeModel.graphModel.getNodeOutgoingNode(props.nodeModel.id) might return undefined.

2. Proper Initialization of Variables:

   • Initialize loopStartNode at the beginning if necessary.
   • Assign default values to potential missing variables (loopBodyNode, etc.).

Here's an improved version with these considerations:

onMounted(() => {
  if (!nodeOutgoingNode.some(item => item.type === loopBodyNode.type)) {
    // Assuming there's a known starting node
    let workflow;
    let x = props.nodeModel.x;
    let y = props.nodeModel.y + 0; // Adjust as needed

    if (props.nodeModel.properties?.node_data?.loop_body) {
      workflow = JSON.parse(JSON.stringify(props.nodeModel.properties.node_data.loop_body));
    } else if (
      props.nodeModel.properties?.node_data?.loop &&
      ((flowchartEditorConfig.defaultWidth || flowchartEditorConfig.widthPx) / window.devicePixelRatio > 600)
    ) {
      const loopProperties = props.nodeModel.properties.node_data.loop;

      /**
       * Calculate new position based on current node's dimensions considering horizontal space adjustment
       */
      const leftAdjustment = Math.floor(loopProperties?.size && loopProperties.size.left ? loopProperties.size.left : 5);

      x = Math.min(
        leftAdjustment +
          loopBodyNode.width -
          props.nodeModel.width,
        (widthPx || flowchartEditorConfig.defaultWidth || flowchartEditorConfig.widthPx) / window.devicePixelRatio - (rightSpace || flowchartEditorConfig.rightSpacing || flowchartEditorConfig.paddingRightPx)
      );
      y =
        (heightPx || flowchartEditorConfig.defaultHeight || flowchartEditorConfig.heightPx) /
        window.devicePixelRatio -
        loopProperties?.y ?
        loopProperties?.y :
        rightSpace ||
        flowchartEditorConfig.rightSpacing ||
        flowchartEditorConfig.paddingRightPx -
        heightOfLoopBody -
        topSpace;
      
      workflow = {
        ...props.nodeModel.properties.node_data.loop,
        width: x,
      };
    }

    const nodeModel = props.nodeModel.graphModel.addNode({
      type: loopBodyNode.type,
      parentID: props.parent,
      id: uuidv4(), // Generate unique ID
      properties: {
        ...loopBodyNode.properties,
        workflow: workflow,
        loop_node_id: props.nodeModel.id,
      },
      x: x,
      y: y,
    });

    if (nodeModel !== undefined) {
      // Create edge between original node and loop body node
      props.nodeModel.graphModel.addEdge({
        source: props.nodeModel.id,
        target: nodeModel.id,
        direction: 'left',
        label: '',
        strokeColor: '#ccc', // Default color
      });
      
      // Clear old edges from children nodes except start/end
      props.nodeModel.children.forEach(childNodeId => {
        if (childNodeId != props.nodeModel.startChildId && childNodeId != props.nodeModel.endChildId) {
          props.nodeModel.graphModel.clearEdgesFromChildNodes(childNodeId);
        }
      });
    }
  }
});

These changes ensure better error handling, proper initialization, and adherence to best practices.