-
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 all 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 |
---|---|---|
|
@@ -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) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,165 @@ | ||
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; | ||
|
||
// Mock dependencies to avoid complex setup | ||
vi.mock('../../../../src/helpers/TemplateString.helper', () => ({ | ||
TemplateString: (body: any) => ({ | ||
parseComponentTemplateVarsAsync: () => ({ | ||
asyncResult: Promise.resolve(body) | ||
}), | ||
parseTeamKeysAsync: () => ({ | ||
asyncResult: Promise.resolve(body) | ||
}), | ||
parse: () => ({ | ||
parse: () => ({ | ||
clean: () => ({ | ||
result: body | ||
}) | ||
}) | ||
}) | ||
}) | ||
})); | ||
|
||
// Mock other dependencies | ||
vi.mock('../../../../src/helpers/JsonContent.helper', () => ({ | ||
JSONContent: (data: any) => ({ | ||
tryParse: () => { | ||
try { | ||
return typeof data === 'string' ? JSON.parse(data) : data; | ||
} catch { | ||
return null; | ||
} | ||
} | ||
}) | ||
})); | ||
|
||
import { parseData } from '../../../../src/Components/APICall/parseData'; | ||
import { REQUEST_CONTENT_TYPES } from '../../../../src/constants'; | ||
|
||
// Simple mock agent | ||
const mockAgent = { | ||
teamId: 'test-team', | ||
id: 'test-agent' | ||
} as any; | ||
|
||
describe('APICall parseData Component', () => { | ||
beforeEach(() => { | ||
vi.clearAllMocks(); | ||
}); | ||
|
||
afterEach(() => { | ||
vi.restoreAllMocks(); | ||
}); | ||
|
||
describe('handleNone function - content-type detection', () => { | ||
it('should detect JSON content-type from headers', async () => { | ||
const body = '{"test": "value"}'; | ||
const config = { | ||
data: { | ||
contentType: REQUEST_CONTENT_TYPES.none, | ||
body | ||
}, | ||
headers: { 'content-type': 'application/json' } | ||
}; | ||
|
||
const result = await parseData({}, config, mockAgent); | ||
|
||
// Should delegate to JSON handler and parse the JSON | ||
expect(result.data).toEqual({ test: 'value' }); | ||
}); | ||
|
||
it('should detect JSON content-type from headers (case-insensitive)', async () => { | ||
const body = '{"test": "value"}'; | ||
const config = { | ||
data: { | ||
contentType: REQUEST_CONTENT_TYPES.none, | ||
body | ||
}, | ||
headers: { 'Content-Type': 'application/json; charset=utf-8' } | ||
}; | ||
|
||
const result = await parseData({}, config, mockAgent); | ||
|
||
// Should delegate to JSON handler | ||
expect(result.data).toEqual({ test: 'value' }); | ||
}); | ||
|
||
it('should use heuristic detection for JSON when no explicit headers', async () => { | ||
const body = '{"valid": "json", "number": 42}'; | ||
const config = { | ||
data: { | ||
contentType: REQUEST_CONTENT_TYPES.none, | ||
body | ||
}, | ||
headers: {} | ||
}; | ||
|
||
const result = await parseData({}, config, mockAgent); | ||
|
||
// Should detect JSON and parse it | ||
expect(result.data).toEqual({ valid: 'json', number: 42 }); | ||
}); | ||
|
||
it('should handle text data as fallback', async () => { | ||
const body = 'plain text content'; | ||
const config = { | ||
data: { | ||
contentType: REQUEST_CONTENT_TYPES.none, | ||
body | ||
}, | ||
headers: {} | ||
}; | ||
|
||
const result = await parseData({}, config, mockAgent); | ||
|
||
// Should return as-is for plain text | ||
expect(result.data).toBe(body); | ||
}); | ||
|
||
it('should prioritize explicit headers over heuristic detection', async () => { | ||
// Data looks like JSON but header says it's text | ||
const body = '{"looks": "like json"}'; | ||
const config = { | ||
data: { | ||
contentType: REQUEST_CONTENT_TYPES.none, | ||
body | ||
}, | ||
headers: { 'content-type': 'text/plain' } | ||
}; | ||
|
||
const result = await parseData({}, config, mockAgent); | ||
|
||
// Should respect header and treat as text, not parse as JSON | ||
expect(result.data).toBe(body); | ||
}); | ||
}); | ||
|
||
describe('integration with existing parseData functionality', () => { | ||
it('should not interfere with explicit JSON content type', async () => { | ||
const body = '{"test": "value"}'; | ||
const config = { | ||
data: { | ||
contentType: REQUEST_CONTENT_TYPES.json, | ||
body | ||
}, | ||
headers: {} | ||
}; | ||
|
||
const result = await parseData({}, config, mockAgent); | ||
expect(result.data).toEqual({ test: 'value' }); | ||
}); | ||
|
||
it('should not interfere with explicit text content type', async () => { | ||
const body = 'test data'; | ||
const config = { | ||
data: { | ||
contentType: REQUEST_CONTENT_TYPES.text, | ||
body | ||
}, | ||
headers: {} | ||
}; | ||
|
||
const result = await parseData({}, config, mockAgent); | ||
expect(result.data).toBe(body); | ||
}); | ||
}); | ||
}); |
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.
parseData.test.ts
covering JSON, URL-encoded, text, heuristic detection, header priority, backward compatibility, and empty data handling.Everything works as expected without regressions.