-
Notifications
You must be signed in to change notification settings - Fork 167
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?
fix: improve content-type detection in parseData handleNone function #154
Conversation
- Enhanced handleNone function to intelligently guess content-type from headers and data structure - Added support for detecting JSON, URL-encoded, multipart form data, and text content types - Checks Content-Type header (case-insensitive) and delegates to appropriate handlers - Implements heuristic content-type detection based on data format when headers are not explicit - Improves API reliability by automatically choosing correct parsing strategy - Addresses FIXME comment requesting content-type guessing functionality Resolves: SmythOS/sre#TODO-content-type-detection Signed-off-by: versona-tech <[email protected]>
PR_DESCRIPTION.md
Outdated
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.
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.
Did you perform some tests for this implementation ?
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.
Yes, I performed local testing and also added automated tests.
- 7 new tests were added in
parseData.test.ts
covering JSON, URL-encoded, text, heuristic detection, header priority, backward compatibility, and empty data handling. - All tests passed successfully (7/7).
- Build completed without errors.
Everything works as expected without regressions.
Hi @versona-tech thank you for your contribution, please check the comments on your PR. @forhad-hosain can you take some time to help in testing these changes and verify that they are not breaking for the UI ? |
- Add tests for explicit content-type headers (JSON, text) - Add tests for heuristic detection when no headers present - Add tests for backward compatibility with existing functionality - Mock dependencies to isolate parseData behavior - All 7 test cases passing successfully - Validates the content-type detection improvements in handleNone function Relates to SmythOS#154 Signed-off-by: versona-tech <[email protected]>
Thanks for the feedback! |
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @versona-tech
Thank you so much for your awesome contribution! 🎉 Your implementation looks great. There’s just a very small adjustment required to make it fully compatible with our UI.
Here’s an example of the config structure we receive from the UI:
config = {
"id": "<COMPONENT_ID>",
"name": "APICall",
"outputs": [...],
"inputs": [...],
"data": {
"method": "POST",
"url": "https://httpbin.org/post",
"headers": "{\n \"Content-Type\": \"multipart/form-data\"\n}",
"contentType": "none",
"body": "{\n \"file\": \"{{file}}\"\n}",
...
},
...
}
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:
You will receive config.data.headers
as a string.
You need to parse it before using it in your implementation.
Once updated, please also adjust the test cases to reflect this behavior.
Thanks again for your great work! 🙌
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 comment
The 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 binary
, we can simply call handleBinary()
.
But with contentType none
, users might specify types like image/png
, video/mp4
, etc, in the headers.
We could check for common file types and call handleBinary() if there’s a match. What do you think?
@forhad-hosain this might be helpful, I recently migrated many unit-tests from our old repo. |
Resolves: SmythOS/sre#TODO-content-type-detection
📝 Description
🚀 Improve Content-Type Detection in API Call Parser
📝 Description
This PR enhances the
handleNone
function inparseData.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:
Content-Type
headers (case-insensitive)🔧 Changes Made
Enhanced Content-Type Detection Logic:
content-type
andContent-Type
headersCode Quality Improvements:
🧪 Testing
📋 Checklist
git commit -s
)🎉 Impact
This improvement enhances the robustness of API calls within the SmythOS platform by:
Type:
fix
- Bug fix and improvementScope:
core/APICall
- Core API parsing functionalityBreaking Change: ❌ No
🔗 Related Issues
🔧 Type of Change
✅ Checklist