Fix: Handle trailing slashes in server URLs across all libraries #1969
Conversation
- Add consistent trailing slash normalization in JavaScript, Python, Ruby, and Rust
- JavaScript: Implement regex-based URL and path normalization
- Python: Use rstrip('/') for clean URL handling
- Ruby: Apply regex substitution for multiple slash removal
- Rust: Loop-based trailing slash elimination
- Add comprehensive test suites for all libraries
- Ensure backward compatibility and consistent behavior
|
@tasn / @svix-mman Can you please review this PR? |
| // Ensure path always starts with a single slash | ||
| const normalizedPath = this.path.replace(/^\/+/, "/"); |
There was a problem hiding this comment.
Why do this, and only for JavaScript? This is an internal API, and I'm pretty sure it's only ever called with paths that already begin with (one) slash, I think?
| const baseUrl: string = ( | ||
| options.serverUrl ?? | ||
| regionalUrl ?? | ||
| "https://api.svix.com" | ||
| ).replace(/\/+$/, ""); |
There was a problem hiding this comment.
I think it's cleaner to only run the replace if the user specified a server URL. The fallback values all don't end in / and I wouldn't want us to become sloppy with things like that internally.
Same for all the other SDKs.
python/tests/test_trailing_slash.py
Outdated
There was a problem hiding this comment.
Can you add a final newline to the end of this file?
| @@ -0,0 +1,80 @@ | |||
| RSpec.describe "Trailing Slash Handling" do | |||
There was a problem hiding this comment.
I'm surprised you copy-pasted bits from the client initialization here rather than actually calling the client initialization. This way the test could easily get out of sync with the actual code. This being Ruby, should it not be simple to check the internal state of the client after construction, like you did with Python? (I haven't seriously used Ruby, but I know it allows all sorts of monkey patching)
There was a problem hiding this comment.
If you make this a unit test rather than an integration test, you can make assertions about the internal state of the Svix client. The way it works is you create an inline module within src/api/client.rs like
#[cfg(test)]
mod tests {
// write tests here
}| result.pop(); | ||
| } | ||
|
|
||
| assert_eq!(result, expected, "Failed for input: {}", input); |
There was a problem hiding this comment.
| assert_eq!(result, expected, "Failed for input: {}", input); | |
| assert_eq!(result, expected, "Failed for input: {input}"); |
| } | ||
|
|
||
| assert_eq!(result, expected, "Failed for input: {}", input); | ||
| println!("✅ {} → {}", input, result); |
There was a problem hiding this comment.
stdout is only printed if a test fails, so this seems kind of pointless.
PR Summary
Summary
• Implements consistent trailing slash handling across JavaScript, Python, Ruby, and Rust libraries
• Adds robust URL normalization to prevent multiple slashes in API requests
• Maintains backward compatibility while fixing edge cases with multiple trailing slashes
• Ensures uniform behavior across all SDK languages for better developer experience
Implementation details
Closes #1154
Test Results
1) JavaScript
2) Rust
3) Ruby