-
Notifications
You must be signed in to change notification settings - Fork 168
fix: improve content-type detection in parseData handleNone function #154
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.
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
base: main
Are you sure you want to change the base?
Changes from 2 commits
bc4e30e
c960dc1
93ef98e
4b838ec
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
# π Improve Content-Type Detection in API Call Parser | ||
|
||
## π Description | ||
|
||
This PR enhances the `handleNone` function in `parseData.ts` to intelligently detect and handle different content types when none is explicitly specified. The improvement addresses a long-standing FIXME comment and significantly improves API reliability. | ||
|
||
## π― What This Fixes | ||
|
||
**Before**: The `handleNone` function would simply return raw data without attempting to determine the appropriate content type, leading to potential parsing issues. | ||
|
||
**After**: The function now: | ||
1. β Checks for explicit `Content-Type` headers (case-insensitive) | ||
2. β Intelligently delegates to appropriate handlers based on detected content type | ||
3. β Implements heuristic detection for JSON, URL-encoded, text, and other formats | ||
4. β Maintains backward compatibility with fallback behavior | ||
|
||
## π§ Changes Made | ||
|
||
### Enhanced Content-Type Detection Logic: | ||
- **Header Analysis**: Checks `content-type` and `Content-Type` headers | ||
- **JSON Detection**: Identifies JSON structures using bracket/brace patterns + validation | ||
- **URL-Encoded Detection**: Recognizes form-encoded data patterns | ||
- **Text Detection**: Falls back to text handling for string data | ||
- **Object Handling**: Automatically stringifies objects for JSON processing | ||
|
||
### Code Quality Improvements: | ||
- β Resolves FIXME comment: "try to guess the content type from headers content-type and data" | ||
- β Maintains existing API compatibility | ||
- β Comprehensive error handling with graceful fallbacks | ||
- β Clean, readable implementation with clear logic flow | ||
|
||
## π§ͺ Testing | ||
|
||
- β Project builds successfully without errors | ||
- β All existing functionality preserved | ||
- β CLI tools continue to work as expected | ||
- β No breaking changes introduced | ||
|
||
## π Checklist | ||
|
||
- [x] Code follows project style guidelines | ||
- [x] Self-review completed | ||
- [x] Functionality tested locally | ||
- [x] No breaking changes | ||
- [x] Commit message follows conventional format | ||
- [x] DCO sign-off included (`git commit -s`) | ||
- [x] FIXME comment addressed and resolved | ||
|
||
## π Impact | ||
|
||
This improvement enhances the robustness of API calls within the SmythOS platform by: | ||
- **Reducing parsing errors** through intelligent content-type detection | ||
- **Improving developer experience** with more predictable behavior | ||
- **Maintaining compatibility** while adding new capabilities | ||
- **Following best practices** for content-type handling | ||
|
||
--- | ||
|
||
**Type:** `fix` - Bug fix and improvement | ||
**Scope:** `core/APICall` - Core API parsing functionality | ||
**Breaking Change:** β No | ||
|
||
Thank you for reviewing this contribution! π |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you perform some tests for this implementation ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I performed local testing and also added automated tests.
Everything works as expected without regressions. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -151,8 +151,54 @@ async function handleBinary(body: any, input: any, config, agent: Agent) { | |
} | ||
|
||
async function handleNone(body: any, input: any, config, agent: Agent) { | ||
//FIXME: try to guess the content type from headers content-type and data | ||
// Try to guess the content type from headers content-type and data | ||
const configHeaders = config?.headers || {}; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hi @versona-tech Hereβs an example of the config structure we receive from the UI:
As you can see, both headers and body are always expected to be strings. So, could you please make sure to handle them accordingly? This means: Once updated, please also adjust the test cases to reflect this behavior. Thanks again for your great work! π |
||
const contentTypeHeader = configHeaders['content-type'] || configHeaders['Content-Type']; | ||
|
||
// If content-type is explicitly set in headers, delegate to appropriate handler | ||
if (contentTypeHeader) { | ||
if (contentTypeHeader.includes('application/json')) { | ||
return await handleJson(body, input, config, agent); | ||
} else if (contentTypeHeader.includes('application/x-www-form-urlencoded')) { | ||
return await handleUrlEncoded(body, input, config, agent); | ||
} else if (contentTypeHeader.includes('multipart/form-data')) { | ||
return await handleMultipartFormData(body, input, config, agent); | ||
} else if (contentTypeHeader.includes('text/')) { | ||
return handleText(body, input, config, agent); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @versona-tech Handling binary data is a bit tricky. By setting contentType to But with contentType |
||
} | ||
|
||
// Attempt to guess content type from data structure | ||
if (typeof body === 'string') { | ||
const trimmedBody = body.trim(); | ||
|
||
// Check if it looks like JSON | ||
if ((trimmedBody.startsWith('{') && trimmedBody.endsWith('}')) || | ||
(trimmedBody.startsWith('[') && trimmedBody.endsWith(']'))) { | ||
try { | ||
JSON.parse(trimmedBody); | ||
return await handleJson(body, input, config, agent); | ||
} catch { | ||
// Not valid JSON, continue with default handling | ||
} | ||
} | ||
|
||
// Check if it looks like URL-encoded data | ||
if (trimmedBody.includes('=') && !trimmedBody.includes(' ') && | ||
!trimmedBody.includes('\n') && !trimmedBody.includes('<')) { | ||
return await handleUrlEncoded(body, input, config, agent); | ||
} | ||
|
||
// Default to text handling for strings | ||
return handleText(body, input, config, agent); | ||
} | ||
|
||
// For objects, try JSON handling | ||
if (typeof body === 'object' && body !== null) { | ||
return await handleJson(JSON.stringify(body), input, config, agent); | ||
} | ||
|
||
// Fallback to original behavior | ||
return { data: typeof body === 'string' ? body : JSON.stringify(body), headers: {} }; | ||
} | ||
function handleText(body: any, input: any, config: any, agent: Agent) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The description should be part of the PR, not as a .md file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out.
Iβve moved the description into the PR itself and removed the extra
PR_DESCRIPTION.md
file.