fix: Set up a complete knowledge base workflow in the loop body, debug prompts that the knowledge base write node cannot be used as the end node

[shaohuzhang1]

fix: Set up a complete knowledge base workflow in the loop body, debug prompts that the knowledge base write node cannot be used as the end node

[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]

After reviewing the provided code, here are some observations and suggested improvements:

1. Duplicated Logic: The is_valid_work_flow method checks both data source nodes and loop start nodes depending on whether the workflow mode is knowledge. This logic can be refactored into a single function to improve maintainability.

2. Code Duplication: There are multiple occurrences of similar logic to filter nodes. Refactoring these blocks can reduce redundancy.

3. Edge Cases Missing:

   • Ensure that all types of nodes (DataSource, LoopStartNode) are correctly handled in validation methods.
   • Handle the case where a node has no edges after validation.

4. Logging and Error Handling: Consider adding more comprehensive logging and better error messages to help with debugging and user experience.

5. Performance Optimization: If there's a lot of processing happening during certain functions, consider optimizing loops or using data structures that allow faster lookups.

Here's an optimized version of the relevant parts:

class WorkFlowInstance {
  constructor(nodes, edge) {}

  validate(workflowModel) {
    // Common setup for validating workflows
    this.workflowModel = workflowModel;
    this.nodes = nodes;
    this.edges = edge;

    try {
      this.is_valid_start_node();
      this.ensureOnlyDataSourcesExist(this.workflowModel);
      this.is_valid_nodes(this.workflowModel);
      this.is_valid_edges(workflowModel);

      // Add any additional validations specific to the workflow model
    } catch (error) {
      console.error(error.message);
    }
  }

  ensureOnlyDataSourcesExist(workflowModel) {
    if (workflowModel !== WorkflowMode.Knowledge || workflowModel !== WorkflowMode.Customize) {
      return true; // No need to verify data sources for other modes
    }

    const nonDataSourceNodes = this.nonDataSourceNodes.filter((node) => !end_nodes_dict[workflowModel].includes(node.properties.kind));
    if (nonDataSourceNodes.length > 0) {
      throw new Error(`${t('workflow.validate.missingDataSource')}`);
    }

    return true;
  }

  isValidNode(properties, type) {
    // Check node properties based on its type
    // You might want to use a switch statement instead
    if (properties.status === 500) {
      throw new Error(`The node "${properties.stepName}" is unavailable.`);
    }
    if ([WorkflowType.Condition].includes(type)) {
      // Handle conditions separately
      const branches = properties.node_data.branch;
      branches.forEach((branchType) => {
        if (!this.edges.some(edge => edge.sourceAnchorId.endsWith(branchType + '_right'))) {
          throw new Error(`Need to connect "${properties.stepName} (${type}) with: ${branchType}`);
        }
      });
    } else if (![...end_nodes_dict[this.workflowModel]].includes(type)) {
      if (type === WorkflowType.LoopNode && properties.node_data.loop_body) {
        const bodyEdges = new KnowledgeWorkFlowInstance(properties.node_data.loop_body, WorkflowMode.KnowledgeLoop).edges;
        if (!bodyEdges.every(edge => edge.targetNodeProperties.kind !== WorkflowKind.LoopingEnd)) {
          throw new Error(`Cannot finish Loop Node "${properties.stepName}".`);
        }
      } else {
        throw new Error(`Cannot end Workflow at node "${properties.stepName}" (${type}). End nodes should be: ${Object.keys(end_nodes_dict[this.workflowModel]).join(', ')}`);
      }
    }
    if (properties.status !== 200) {
      throw new Error(`The node "${properties.stepName}" is not available.`);
    }
  }

  private nonDataSourceNodes(): { id: string, properties }: any[] {
    return this.nodes.filter(node => ![WorkflowType.DataSource, WorkflowType.LoopStartNode].includes(node.type));
  }

  private getNodesByType(nodes, type): { id: string, properties }: any[] {
    return nodes.filter(node => node.type === type);
  }
}

This version consolidates and simplifies the logic while maintaining readability and robustness. Make sure to adjust the error messages and exception handling according to your application's needs.

[shaohuzhang1]

There are no major structural issues or obvious bugs in your provided code snippet. However, here are a few areas where improvements can be made for clarity and maintainability:

1. Variable Naming: The variable knowledge_work_flow_instance could be more descriptive, maybe something like instanceForKnowledgeLoop. This will make it easier to understand its purpose.

2. Conditional Logic Formatting: Consider using template literals for constructing the message in the console log at the end of the validate function (line 44). It would look cleaner and more readable.

Here's the updated code snippet with these suggestions:

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 { WorkFlowInstance, KnowledgeWorkFlowInstance } from '@/workflow/common/validate';
import { t } from '@/locales';
import { disconnectByFlow } from '@/workflow/common/teleport';
const loop_workflow_mode = inject('loopWorkflowMode') || WorkflowMode.ApplicationLoop;
const props = defineProps<{ nodeModel: any }>();
const containerRef = ref();
const LoopBodyContainerRef = ref<InstanceType<typeof LoopBodyContainer>>();
const validate = () => {
  const workflow =
    loop_workflow_mode === WorkflowMode.ApplicationLoop
      ? new WorkFlowInstance(lf.value.getGraphData(), WorkflowMode.ApplicationLoop)
      : new KnowledgeWorkFlowInstance(lf.value.getGraphData(), WorkflowMode.KnowledgeLoop);
  return Promise.all(lf.value.graphModel.nodes.map((element: any) => element?.validate?.()))
    .then(() => {
      const loop_node_id = props.nodeModel.properties.loop_node_id;
      
      // Use template literal for more organized string
      console.log(t("Validate completed successfully.", { node_id: loop_node_id }));
      
      // Rest of the validation logic may follow...
    })
    .catch((error) => {
      console.error(t("Validation failed:", { error_message: error.message }));
    })
    .finally(() => {});
};

These changes improve readability and maintainability within your codebase.