fix: skip null query parameters in Postman to Bruno conversion#7193
fix: skip null query parameters in Postman to Bruno conversion#7193sanish-bruno wants to merge 2 commits intousebruno:mainfrom
Conversation
Updated the importPostmanV2CollectionItem function to skip query parameters where both key and value are null. Added a test case to ensure that such parameters are not included in the converted Bruno collection, while preserving other valid parameters.
sanish-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
WalkthroughEnhances Postman-to-Bruno converter with null-safety guards for query parameters. Adds protective checks to skip entries where both key and value are null, and uses nullish coalescing to ensure defined string values in parameter assignments. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-converters/tests/postman/postman-to-bruno/postman-to-bruno.spec.js (1)
608-658: Good test — butoriginalRequest.url.querynull-filter path is uncovered.The test validates the fix for the main request query params (lines 535–547 in source), but the parallel null guard added to
originalRequest.url.query.forEach(lines 625–631) has no corresponding test. Per coding guidelines, both code paths should be covered.Consider adding a companion test case that includes an
i.response[].originalRequest.url.queryarray with a{ key: null, value: null }entry and asserts it's excluded frombrunoCollection.items[0].examples[0].request.params.📝 Suggested additional test case
+ it('should skip null query params in originalRequest (example response)', async () => { + const collection = { + info: { + _postman_id: 'test-null-orig-req', + name: 'collection', + schema: 'https://schema.getpostman.com/json/collection/v2.1.0/collection.json' + }, + item: [ + { + name: 'request', + request: { + method: 'GET', + header: [], + url: { raw: 'https://example.com/api' } + }, + response: [ + { + name: 'Example 1', + originalRequest: { + method: 'GET', + url: { + raw: 'https://example.com/api?q=hello', + query: [ + { key: 'q', value: 'hello' }, + { key: null, value: null }, + { key: null, value: 'x' } + ] + } + }, + status: 'OK', + code: 200, + body: '' + } + ] + } + ] + }; + + const brunoCollection = await postmanToBruno(collection); + const params = brunoCollection.items[0].examples[0].request.params; + + expect(params).toHaveLength(2); + expect(params[0]).toMatchObject({ name: 'q', value: 'hello', type: 'query' }); + expect(params[1]).toMatchObject({ name: '', value: 'x', type: 'query' }); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-converters/tests/postman/postman-to-bruno/postman-to-bruno.spec.js` around lines 608 - 658, Add a companion test that exercises the originalRequest.url.query null-filter path by creating a collection object where an item has a response array whose first element contains originalRequest.url.query including an entry { key: null, value: null }; call postmanToBruno and then assert that brunoCollection.items[0].examples[0].request.params does not include the fully-null entry (check length) and that the remaining params are normalized the same way as in the existing test (null key -> '', null value -> ''). This will cover the code in the originalRequest.url.query.forEach null-guard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/bruno-converters/tests/postman/postman-to-bruno/postman-to-bruno.spec.js`:
- Around line 608-658: Add a companion test that exercises the
originalRequest.url.query null-filter path by creating a collection object where
an item has a response array whose first element contains
originalRequest.url.query including an entry { key: null, value: null }; call
postmanToBruno and then assert that
brunoCollection.items[0].examples[0].request.params does not include the
fully-null entry (check length) and that the remaining params are normalized the
same way as in the existing test (null key -> '', null value -> ''). This will
cover the code in the originalRequest.url.query.forEach null-guard.
| value: param.value, | ||
| name: param.key ?? '', | ||
| value: param.value ?? '', | ||
| description: transformDescription(param.description), |
There was a problem hiding this comment.
Header params are pushed without any null guard. If Postman can produce null keys/values for headers too, the same crash could happen there. Worth investigating.
| if (param.key == null && param.value == null) { | ||
| return; | ||
| } | ||
| brunoRequestItem.request.params.push({ |
There was a problem hiding this comment.
Can we consider extracting a shared helper to normalizeQueryParams, since the null-guard + nullish checking logic is duplicated in two places.
There was a problem hiding this comment.
I feel like it may not be ideal, it is just a simple guard clause right? do you have any specific implementation in mind? Am i missing something here.
Updated the importPostmanV2CollectionItem function to skip headers, URL-encoded parameters, and form data where both key and value are null. Added corresponding test cases to ensure proper handling of these scenarios in the conversion process.
2 participants
Jira
Summary
nullkeys/values (e.g.,{"key": null, "value": null}), which causes a crash during import:TypeError: Cannot read properties of null (reading 'includes')null, and normalize individualnullkeys/values to empty strings — matching the existing guard used for path variablesTest plan
bruno-converterstests still pass (npm run test --workspace=packages/bruno-converters)Description
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
Bug Fixes
Tests