[feat] Decode postData params when mimeType is application/x-www-form-urlencoded #96
Conversation
EdvinasDaugirdas
requested review from
legander,
e-fisher,
w1kman and
allansson
There was a problem hiding this comment.
Sweet! It works nicely too 🙌 I left a few small comments, check em out and decide if you want to make a change or not.
Consider adding this change too to fix the BOM issue https://github.com/grafana/har-to-k6/pull/75/files#diff-6c8538269ea8beb3f861758572b0197f510b809890216c333e8a1a034b021b85
| return entry | ||
| } | ||
|
|
||
| if (entry.request.method !== 'POST' || !entry.request.postData) { |
There was a problem hiding this comment.
Other verbs are can have postData too not only POST, maybe we should just be checking entry.request.postData here.
There was a problem hiding this comment.
Yes, makes sense.
| } | ||
|
|
||
| if ( | ||
| entry.request.postData.mimeType !== 'application/x-www-form-urlencoded' || |
There was a problem hiding this comment.
it is not uncommon that mimeType here will include a chartset like application/x-www-form-urlencoded;charset=UTF-8 which would make this not match, look into using the helper function in aid.js called getContentTypeValue
There was a problem hiding this comment.
Good call, gonna update
| entries = getSortedEntries(entries, options) | ||
| entries = getDecodedPostDataParamEntries(entries) | ||
| return entries | ||
| } |
There was a problem hiding this comment.
I think we should try to avoid reassignment as much as possible and always return new objects. I know there are mutations and reassignment all over the place in the project but let's try to not add more of it. I think it will make the code easier to read, debug and produce less unexpected behaviour.
I wish we had a pipe operator here but i suppose this will also do
return getDecodedPostDataParamEntries(
getSortedEntries(entries, options)
)
// Alternatively
const sortedEntries = getSortedEntries(entries, options)
return getDecodedPostDataParamEntries(sortedEntries)It looks like getSortedEntries does a little more than sorting, perhaps a more fitting name would be setSleepTimings or createSleepStatements idk wdyt? 🤷♀️
There was a problem hiding this comment.
Yeaaaah, agree with reassignment 👍
The getSortedEntries is probably more fitting than setSleepTimings or createSleepStatements. It will always sort entries if there's a startedDateTime available, but only add sleep properties if { addSleep: true } option is passed.
There was a problem hiding this comment.
Fair enough 👍
It would have maybe made sense to split the two to separate the concerns then.
sortEntries
decodePostdataParams
if (options.addSleep) {
return withSleep(..)
}
return result
POST data with
application/x-www-form-urlencodedmight be encoded.We should attempt to decode it as otherwise we'll end up with "double encoding".
normalizefunction