Fix API Key Using Property #350
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (3)
Deepgram/Models/Authenticate/v1/DeepgramWsClientOptions.cs (3)
Line range hint
100-150: Enhance validation and logging for better observability.While the validation logic is functional, consider these improvements:
- Move API key validation earlier in the constructor.
- Use more descriptive error messages.
- Adjust log levels for better production monitoring.
Consider this structure:
public DeepgramWsClientOptions(...) { Log.Verbose("DeepgramWsClientOptions", "ENTER"); + + // Validate and set API key first + if (!string.IsNullOrWhiteSpace(apiKey)) + { + ApiKey = apiKey; + Log.Information("DeepgramWsClientOptions", "Using provided API key"); + } + else + { + ApiKey = Environment.GetEnvironmentVariable(Defaults.DEEPGRAM_API_KEY) ?? string.Empty; + if (!string.IsNullOrWhiteSpace(ApiKey)) + { + Log.Information("DeepgramWsClientOptions", "Using API key from environment"); + } + } + + if (!OnPrem && string.IsNullOrWhiteSpace(ApiKey)) + { + throw new ArgumentException( + "Deepgram API Key is required. Provide it via constructor or DEEPGRAM_API_KEY environment variable." + ); + } // Rest of the initialization... }
Line range hint
151-200: Simplify URL handling logic using Uri class.The current implementation uses multiple regex operations for URL validation and transformation. Consider using .NET's
Uriclass for more robust URL handling.Here's a suggested implementation:
private string NormalizeBaseAddress(string address) { // Remove any existing protocols var cleanAddress = address.Replace("http://", "") .Replace("https://", "") .Replace("ws://", "") .Replace("wss://", ""); // Ensure API version if (!cleanAddress.Contains($"/v{APIVersion}")) { cleanAddress = $"{cleanAddress.TrimEnd('/')}/{APIVersion}"; } // Create WebSocket URL var wsUrl = $"wss://{cleanAddress.TrimStart('/')}"; return new Uri(wsUrl).AbsoluteUri.TrimEnd('/'); } // Usage in constructor: BaseAddress = NormalizeBaseAddress(baseAddress ?? Defaults.DEFAULT_URI); Log.Information("DeepgramWsClientOptions", $"Normalized BaseAddress: {BaseAddress}");This approach:
- Simplifies the logic by handling URL normalization in a single method
- Uses
Uriclass for validation and normalization- Reduces regex usage
- Makes the transformation steps more clear and maintainable
Line range hint
201-250: Enhance addon value parsing and validation.The current addon parsing logic silently ignores parsing failures and doesn't validate the parsed values.
Consider this improved implementation:
private decimal ParseAddonValue(string key, decimal defaultValue = 0) { if (!Addons.TryGetValue(key, out var addonValue)) { Log.Debug("DeepgramWsClientOptions", $"Addon {key} not found, using default: {defaultValue}"); return defaultValue; } if (!decimal.TryParse(addonValue, out var parsedValue)) { Log.Warning("DeepgramWsClientOptions", $"Failed to parse {key} value: {addonValue}, using default: {defaultValue}"); return defaultValue; } if (parsedValue < 0) { Log.Warning("DeepgramWsClientOptions", $"Invalid negative value for {key}: {parsedValue}, using default: {defaultValue}"); return defaultValue; } Log.Information("DeepgramWsClientOptions", $"Using {key}: {parsedValue}"); return parsedValue; } // Usage in constructor: AutoFlushReplyDelta = ParseAddonValue(Constants.AutoFlushReplyDelta); AutoFlushSpeakDelta = ParseAddonValue(Constants.AutoFlushSpeakDelta);This approach:
- Provides better error handling and logging
- Validates parsed values
- Uses a consistent pattern for all addon parsing
- Makes the code more maintainable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
Deepgram/Models/Authenticate/v1/DeepgramHttpClientOptions.cs(1 hunks)Deepgram/Models/Authenticate/v1/DeepgramWsClientOptions.cs(1 hunks)Deepgram/Models/Manage/v1/Config.cs(1 hunks)
🔇 Additional comments (1)
Deepgram/Models/Authenticate/v1/DeepgramHttpClientOptions.cs (1)
103-106: Verify API key usage across the codebase
Since this modifies core authentication behavior, we should verify:
- All client instantiations handle the API key correctly
- No breaking changes for existing usage patterns
- Consistent behavior between HTTP and WebSocket clients
✅ Verification successful
API key handling is consistent and properly implemented
The verification shows that:
- Both HTTP and WebSocket clients implement the same
IDeepgramClientOptionsinterface - API key initialization is consistent across both client types, using the same null-coalescing pattern (
apiKey ?? "") - The change in
DeepgramHttpClientOptionsmaintains consistency with the existing pattern in the codebase - Client instantiations in tests and examples follow the expected patterns
The modification to check for null/whitespace before setting the API key is a safe enhancement that maintains backward compatibility while adding an extra validation layer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for DeepgramHttpClientOptions instantiations
echo "Checking DeepgramHttpClientOptions usage patterns:"
rg -A 2 "new DeepgramHttpClientOptions"
# Compare with WebSocket client implementation
echo -e "\nChecking for consistency with WebSocket client:"
rg -A 10 "class DeepgramWsClientOptions"
# Look for direct ApiKey property access
echo -e "\nChecking direct ApiKey property usage:"
rg "\.ApiKey\s*[=]"
Length of output: 6206
Script:
#!/bin/bash
# Let's check the implementation of both client options classes
echo "Checking DeepgramHttpClientOptions implementation:"
rg -A 15 "class DeepgramHttpClientOptions"
echo -e "\nChecking DeepgramWsClientOptions implementation:"
rg -A 15 "class DeepgramWsClientOptions"
# Check for interface definition
echo -e "\nChecking IDeepgramClientOptions interface:"
rg -A 10 "interface IDeepgramClientOptions"
# Check for actual API key usage in client implementations
echo -e "\nChecking API key usage in client implementations:"
rg -A 3 "ApiKey\s*=\s*apiKey" --type cs
Length of output: 5122
lukeocodes
approved these changes
Nov 4, 2024
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Proposed changes
Addresses: #342, #346
Tested using microphone streaming example.
Types of changes
What types of changes does your code introduce to the community .NET SDK?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
NA
Summary by CodeRabbit
New Features
ApiKeyandBaseAddressin client options, improving visibility during initialization.ApiKeyin bothDeepgramHttpClientOptionsandDeepgramWsClientOptions.Bug Fixes
DetectTopicsproperty from theConfigrecord to address an identified error.Refactor
DeepgramHttpClientOptionsandDeepgramWsClientOptionsfor better error handling and configuration validation.