"Fix: Avoid double-wrapping JSON in DoStream" #287
Conversation
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Parser
participant Buffer
Client->>Parser: Attempt to unmarshal input text
alt Valid JSON with "text" field exists
Parser-->>Client: isValidJSON = true
Client->>Client: Log "Input is already JSON"
Client->>Buffer: Write original text
alt Write operation fails
Buffer-->>Client: Return error
else Write operation succeeds
Buffer-->>Client: Return success
end
else
Parser-->>Client: isValidJSON = false
Client->>Client: Encode text as textSource struct
Client->>Buffer: Write encoded text
Buffer-->>Client: Return result or error
end
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 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: 1
🧹 Nitpick comments (6)
pkg/client/analyze/v1/client.go (6)
149-154: Consider additional logging for JSON parsing failuresWhen JSON unmarshaling fails, this is valuable debugging information that is currently not logged.
if err := json.Unmarshal([]byte(text), &tmp); err == nil { // It's valid JSON, now check if it already contains a "text" field if val, exists := tmp["Text"]; exists && val != nil { isValidJSON = true } + } else { + klog.V(6).Infof("Input is not valid JSON. Err: %v\n", err) }
157-163: Fix incorrect package name in log messageThe error log message references
speak.DoText()but this is in theanalyzepackage.if isValidJSON { klog.V(6).Infof("Is Already JSON \n") _,err = buf.WriteString(text) if err != nil { klog.V(1).Infof("JSON WriteString failed. Err: %v\n", err) - klog.V(6).Infof("speak.DoText() LEAVE\n") + klog.V(6).Infof("analyze.DoText() LEAVE\n") return err }
157-157: Enhance log message for better debuggingThe current log message could be more descriptive to aid in debugging.
- klog.V(6).Infof("Is Already JSON \n") + klog.V(6).Infof("Input already contains valid JSON with 'text' field, skipping re-encoding\n")
151-153: Validate the type of the Text fieldThe code checks if the "Text" field exists and is not nil, but doesn't verify that it's a string, which could lead to issues later.
- if val, exists := tmp["Text"]; exists && val != nil { + if val, exists := tmp["text"]; exists && val != nil { + // Verify that the value is a string + if _, ok := val.(string); ok { isValidJSON = true + } }
158-158: Fix missing space after commaThere's a formatting issue with a missing space after the comma in the function call.
- _,err = buf.WriteString(text) + _, err = buf.WriteString(text)
163-164: Remove unnecessary trailing whitespaceThere's unnecessary trailing whitespace at the end of line 163.
return err } - + } else {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
pkg/client/analyze/v1/client.go(1 hunks)
🔇 Additional comments (1)
pkg/client/analyze/v1/client.go (1)
146-172: Feature implementation looks good, with suggested fixesThe overall approach to prevent double-wrapping JSON by checking if the input is already in JSON format is sound and addresses the PR objectives. With the suggested fixes above, this will effectively solve the issue of double-wrapping JSON in DoStream.
Proposed changes
Stream is now checked if it already a JSON and avoiding Double-Wrapping the JSON.
Types of changes
Checklist
Further comments
Considered creating another Function DoJSON but it is not needed as the change is very subtle.
Summary by CodeRabbit