fix: websocket runnable

[dieriba]

Important

Refactor WebSocket URL handling by replacing get_url_from_runnable with get_url_from_runnable_value and updating argument processing.

• Refactor WebSocket URL Handling:
  • Replace get_url_from_runnable with get_url_from_runnable_value in listener.rs and handler.rs.
  • Modify get_url_from_runnable_value to accept args as Option<&Box<RawValue>>.
  • Update value_to_args_hashmap to handle Option<&Box<RawValue>>.
• Argument Handling:
  • Use to_raw_value for url_runnable_args in handler.rs and listener.rs.
  • Update raw_value_to_args_hashmap to convert args to HashMap.
• Miscellaneous:
  • Remove get_url_from_runnable method from listener.rs.
  • Minor logging adjustments in listener.rs.

^{This description was created by for d3884c7. You can customize this summary. It will automatically update as commits are pushed.}

[claude]

Claude finished @dieriba's task —— View job

---

Code Review for PR: fix: websocket runnable

Todo List:

• Read and analyze the modified files
• Provide comprehensive code review feedback
• Check for bugs, performance, and security issues
• Add reproduction instructions for testing

Review Summary

This PR refactors websocket runnable handling by consolidating duplicate logic and improving type safety. The changes primarily focus on removing code duplication and standardizing the handling of runnable arguments across different websocket trigger operations.

Code Quality & Best Practices ✅

Positive aspects:

• DRY Principle: Excellent removal of duplicate code by extracting get_url_from_runnable method into a shared get_url_from_runnable_value function
• Type Safety: Improved type consistency by using Box<RawValue> instead of serde_json::Value throughout
• Import Cleanup: Added necessary import (to_raw_value) and removed unused functions

Areas of concern:

1. Function Parameter Type Changes (mod.rs:64, 92)

// Before
args: Option<&serde_json::Value>,

// After  
args: Option<&Box<RawValue>>,

Issue: This is a breaking change in function signature. If these are public functions used by other modules, this could cause compilation failures elsewhere.
Recommendation: Ensure all callers have been updated, or consider providing a migration path.

2. Deserialization Method Change (mod.rs:68)

// Before
serde_json::from_value(args.clone())

// After
serde_json::from_str(args.get())

Issue: This changes the deserialization approach from value-based to string-based parsing. While more efficient (avoiding clone), it changes behavior.
Recommendation: Verify this doesn't break existing functionality, especially with complex nested arguments.

Performance Considerations ✅

Improvements:

• Removed unnecessary clone() operation in mod.rs:68, which improves memory efficiency
• Consolidated duplicate logic reduces code size and maintenance overhead
• More efficient string-based parsing with from_str() vs from_value()

Security Implications ✅

No major security concerns identified:

• Input validation remains intact through existing error handling
• No new attack vectors introduced
• Authentication and authorization patterns preserved

Potential Issues & Suggestions

1. Error Handling in handler.rs:240

config.url_runnable_args.as_ref().map(to_raw_value).as_ref()

Issue: The to_raw_value function could potentially fail, but the error is not handled here.
Suggestion: Consider proper error propagation:

config.url_runnable_args.as_ref()
    .map(to_raw_value)
    .transpose()?
    .as_ref()

2. Function Naming Consistency

The function raw_value_to_args_hashmap in listener.rs and value_to_args_hashmap in mod.rs serve similar purposes but have different names and slightly different implementations. Consider consolidating these for consistency.

3. Path Extraction Logic (listener.rs:158)

let path = url.splitn(2, ':').nth(1).unwrap();

Issue: Using unwrap() could panic if URL format is unexpected.
Suggestion: Add proper error handling:

let path = url.splitn(2, ':').nth(1)
    .ok_or_else(|| Error::BadConfig("Invalid runnable URL format".to_string()))?;

Minor Improvements

1. Code Readability: The extracted function makes the main logic cleaner and easier to follow
2. Maintainability: Centralized URL handling logic will be easier to maintain and debug
3. Testing: Having a single function for URL extraction will make unit testing more straightforward

Overall Assessment

This is a good refactoring PR that improves code organization and eliminates duplication. The changes are well-structured and follow good software engineering practices. The main concerns are around potential breaking changes and some missing error handling.

Recommendation: ✅ Approve with minor modifications

---

Testing Instructions

To verify these websocket runnable changes, navigate to the Windmill web interface and:

Create a websocket trigger that uses a runnable URL (either $script:path/to/script or $flow:path/to/flow format) with custom arguments in the url_runnable_args field. Test both script and flow runnables to ensure the consolidated URL resolution logic works correctly. Verify that the websocket connection establishes properly and that any initial messages or filters function as expected. The changes should be transparent to the user experience while providing better code organization internally.

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to d3884c7 in 1 minute and 29 seconds. Click for details.

• Reviewed 120 lines of code in 3 files
• Skipped 0 files when reviewing.
• Skipped posting 3 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. backend/windmill-api/src/triggers/websocket/handler.rs:240

• Draft comment:
  Mapping config.url_runnable_args with to_raw_value converts the Option<serde_json::Value> to the expected Option<&Box> for get_url_from_runnable_value. Confirm that input types are as intended.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

2. backend/windmill-api/src/triggers/websocket/listener.rs:156

• Draft comment:
  The inline block now retrieves authed and converts the SqlxJson-wrapped url_runnable_args using .map(|r| &r.0) before calling get_url_from_runnable_value. Ensure that the unwrap on splitting the URL (using splitn and nth(1)) is safe given expected URL formats.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

3. backend/windmill-api/src/triggers/websocket/mod.rs:63

• Draft comment:
  The updated value_to_args_hashmap now accepts Option<&Box> and uses serde_json::from_str(args.get()). This change ensures the raw JSON string is properly parsed. Verify that error handling and test cases cover any malformed JSON scenarios.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_aqdYkLxzMRsb3FuR

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[cloudflare-workers-and-pages]

Deploying windmill with    Cloudflare Pages

Latest commit:	d3884c7
Status:	✅  Deploy successful!
Preview URL:	https://08cde353.windmill.pages.dev
Branch Preview URL:	https://dieri-websocket-runnable.windmill.pages.dev

View logs