Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

detect cycle in cli custom commands #765

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

detect cycle in cli custom commands #765

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cb85d03
detect cycle in cli custom commands
pkbhowmick Nov 5, 2024
91ad5e6
update command parsing
pkbhowmick Nov 5, 2024
9022b8c
Merge branch 'main' into pulak/DEV-2688
osterman Nov 8, 2024
3a9574b
Merge branch 'main' into pulak/DEV-2688
osterman Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 67 additions & 0 deletions cmd/cmd_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,69 @@ func WithStackValidation(check bool) AtmosValidateOption {
}
}

func detectCycle(commands []schema.Command) bool {
// Build a command graph
graph := make(map[string][]string)
for _, cmd := range commands {
for _, step := range cmd.Steps {
// Add an edge from command to each command it depends on
graph[cmd.Name] = append(graph[cmd.Name], parseCommandName(step))
}
}

// To track visited nodes and detect cycles
visited := make(map[string]bool)
recStack := make(map[string]bool)

// Run DFS for each command to detect cycles
for cmd := range graph {
if detectCycleUtil(cmd, graph, visited, recStack) {
return true // Cycle detected
}
}
return false // No cycle detected
}
Comment on lines +36 to +57
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider adding validation for command existence and depth limiting

The cycle detection implementation is solid, but could be enhanced in a few ways:

  1. Validate that all commands referenced in steps exist in the command list
  2. Add a maximum depth limit as mentioned in the requirements

Here's a suggested enhancement:

 func detectCycle(commands []schema.Command) bool {
+    // Create a map of valid commands for quick lookup
+    validCommands := make(map[string]bool)
+    for _, cmd := range commands {
+        validCommands[cmd.Name] = true
+    }
+
     // Build a command graph
     graph := make(map[string][]string)
     for _, cmd := range commands {
         for _, step := range cmd.Steps {
-            graph[cmd.Name] = append(graph[cmd.Name], parseCommandName(step))
+            cmdName := parseCommandName(step)
+            if cmdName != "" && !validCommands[cmdName] {
+                return true  // Invalid command reference creates a broken chain
+            }
+            graph[cmd.Name] = append(graph[cmd.Name], cmdName)
         }
     }

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like a good recommendation

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@osterman, here's an updated detectCycle function incorporating command existence validation and depth limiting:

func detectCycle(commands []schema.Command, maxDepth int) bool {
    // Create a map of valid commands for quick lookup
    validCommands := make(map[string]bool)
    for _, cmd := range commands {
        validCommands[cmd.Name] = true
    }

    // Build a command graph
    graph := make(map[string][]string)
    for _, cmd := range commands {
        for _, step := range cmd.Steps {
            cmdName := parseCommandName(step)
            if cmdName != "" {
                if !validCommands[cmdName] {
                    return true // Invalid command reference creates a broken chain
                }
                graph[cmd.Name] = append(graph[cmd.Name], cmdName)
            }
        }
    }

    // To track visited nodes and detect cycles
    visited := make(map[string]bool)
    recStack := make(map[string]bool)

    // Run DFS for each command to detect cycles with depth limit
    for cmd := range graph {
        if detectCycleUtil(cmd, graph, visited, recStack, 0, maxDepth) {
            return true // Cycle detected or depth limit exceeded
        }
    }
    return false // No cycle detected
}

func detectCycleUtil(command string, graph map[string][]string, visited, recStack map[string]bool, depth, maxDepth int) bool {
    if depth > maxDepth {
        return true // Depth limit exceeded
    }

    // If the current command is in the recursion stack, there's a cycle
    if recStack[command] {
        return true
    }

    // If already visited, no need to explore again
    if visited[command] {
        return false
    }

    // Mark as visited and add to recursion stack
    visited[command] = true
    recStack[command] = true

    // Recurse for all dependencies
    for _, dep := range graph[command] {
        if detectCycleUtil(dep, graph, visited, recStack, depth+1, maxDepth) {
            return true
        }
    }

    // Remove from recursion stack before backtracking
    recStack[command] = false
    return false
}

You'll need to define an appropriate maxDepth when calling detectCycle to enforce the recursion limit.


func detectCycleUtil(command string, graph map[string][]string, visited, recStack map[string]bool) bool {
// If the current command is in the recursion stack, there's a cycle
if recStack[command] {
return true
}

// If already visited, no need to explore again
if visited[command] {
return false
}

// Mark as visited and add to recursion stack
visited[command] = true
recStack[command] = true

// Recurse for all dependencies
for _, dep := range graph[command] {
if detectCycleUtil(dep, graph, visited, recStack) {
return true
}
}

// Remove from recursion stack before backtracking
recStack[command] = false
return false
}

// Helper function to parse command name from the step
func parseCommandName(step string) string {
// Split the step into parts
parts := strings.Split(step, " ")

// Check if the command starts with "atmos" and has additional parts
if len(parts) > 1 && parts[0] == "atmos" {
// Return everything after "atmos" as a single string
return strings.Join(parts[1:], " ")
}
return ""
}
Comment on lines +86 to +97
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance command parsing robustness

The current parsing logic might miss cycles in complex command formats and could mask issues by returning empty strings.

Consider this enhanced implementation:

 func parseCommandName(step string) string {
-    // Split the step into parts
     parts := strings.Split(step, " ")
 
-    // Check if the command starts with "atmos" and has additional parts
     if len(parts) > 1 && parts[0] == "atmos" {
-        // Return everything after "atmos" as a single string
-        return strings.Join(parts[1:], " ")
+        // Extract the actual command name, handling flags and arguments
+        cmdParts := []string{}
+        for _, part := range parts[1:] {
+            if strings.HasPrefix(part, "-") {
+                break  // Stop at first flag
+            }
+            cmdParts = append(cmdParts, part)
+        }
+        return strings.Join(cmdParts, " ")
     }
     return ""
 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Helper function to parse command name from the step
func parseCommandName(step string) string {
// Split the step into parts
parts := strings.Split(step, " ")
// Check if the command starts with "atmos" and has additional parts
if len(parts) > 1 && parts[0] == "atmos" {
// Return everything after "atmos" as a single string
return strings.Join(parts[1:], " ")
}
return ""
}
// Helper function to parse command name from the step
func parseCommandName(step string) string {
parts := strings.Split(step, " ")
if len(parts) > 1 && parts[0] == "atmos" {
// Extract the actual command name, handling flags and arguments
cmdParts := []string{}
for _, part := range parts[1:] {
if strings.HasPrefix(part, "-") {
break // Stop at first flag
}
cmdParts = append(cmdParts, part)
}
return strings.Join(cmdParts, " ")
}
return ""
}


// processCustomCommands processes and executes custom commands
func processCustomCommands(
cliConfig schema.CliConfiguration,
Expand All @@ -47,6 +110,10 @@ func processCustomCommands(
existingTopLevelCommands = getTopLevelCommands()
}

if detectCycle(commands) {
return errors.New("cycle detected in custom cli commands")
}
Comment on lines +113 to +115
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance error reporting and implement environment variable tracking

The current implementation could provide more helpful error messages and implement the environment variable tracking suggested in the PR comments.

Consider this enhancement:

+const AtmosCommandDepthEnv = "ATMOS_COMMAND_DEPTH"
+
+func getCommandDepth() int {
+    depth := os.Getenv(AtmosCommandDepthEnv)
+    if depth == "" {
+        return 0
+    }
+    val, _ := strconv.Atoi(depth)
+    return val
+}
+
 if detectCycle(commands) {
-    return errors.New("cycle detected in custom cli commands")
+    return fmt.Errorf("cycle detected in custom CLI commands - this could lead to infinite recursion")
+}
+
+// Implement depth tracking as suggested in PR comments
+depth := getCommandDepth()
+if depth > 10 {  // Configure max depth as needed
+    return fmt.Errorf("maximum command depth exceeded (%d) - possible recursive loop", depth)
 }
+os.Setenv(AtmosCommandDepthEnv, strconv.Itoa(depth+1))

Committable suggestion skipped: line range outside the PR's diff.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pkbhowmick something like this 👆

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pkbhowmick something like this 👆

got it. Thank you


for _, commandCfg := range commands {
// Clone the 'commandCfg' struct into a local variable because of the automatic closure in the `Run` function of the Cobra command.
// Cloning will make a closure over the local variable 'commandConfig' which is different in each iteration.
Expand Down