Skip to content
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

Fix for error with params #2448

Closed
wants to merge 3 commits into from
Closed

Conversation

fgrande
Copy link
Contributor

@fgrande fgrande commented Jun 12, 2024

Description

This would fix Issue #2439
Looks like "params" property breaks the call, and this is filled with the params.

Copy link
Collaborator

@sanjai0py sanjai0py left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add more details on how this fix works?

@fgrande
Copy link
Contributor Author

fgrande commented Jun 13, 2024

Consider those calls

https://spring-boot-test-aplication.herokuapp.com/api/users/findById/:id (set 16 as :id)
https://spring-boot-test-aplication.herokuapp.com/api/users/findById/16

The difference between them are the "params" tag in the request.

Request with params :id :

mode: 'none',
method: 'GET',
url: 'https://spring-boot-test-aplication.herokuapp.com/api/users/findById/16',
headers: {},
params: [
{
name: 'id',
value: '16',
enabled: true,
type: 'path',
uid: '0Ndm3TH0VGhdOE0CgP4f6'
}
],
responseType: 'arraybuffer',
script: {},

Request without param :

mode: 'none',
method: 'GET',
url: 'https://spring-boot-test-aplication.herokuapp.com/api/users/findById/16',
headers: {},
params: [],
responseType: 'arraybuffer',
script: {},
vars: {},
assertions: [],

As you can see the difference lays in the params property. Since it is already decoded (the request.url is - at this point - already filled with the correct param) we don't need it anymore. And putting it, axios refuse the request because of an exception related to the actual url composed :

_header: 'GET /api/users/findById/16?0[name]=id&0[value]=16&0[enabled]=true&0[type]=path&0[uid]=w4AGcAZKgr0VVwXrnBKt7 HTTP/1.1\r\n' +
'Accept: application/json, text/plain, /\r\n' +
'request-start-time: 1718263487771\r\n' +
'User-Agent: axios/1.6.7\r\n' +
'Accept-Encoding: gzip, compress, deflate, br\r\n' +
'Host: spring-boot-test-aplication.herokuapp.com\r\n' +
'Connection: keep-alive\r\n' +
'\r\n',

So, removing the params property, works fine. Maybe is not the cleaner solution, but I can give the idea of what's happening.

Regards
FabioG

@fgrande
Copy link
Contributor Author

fgrande commented Jun 13, 2024

Adding test files (related to the issue)
BrunoTest.zip

@@ -499,6 +499,8 @@ const registerNetworkIpc = (mainWindow) => {
let response, responseTime;
try {
/** @type {import('axios').AxiosResponse} */
// Looks like the "params" create problems if filled
request.params = [];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After interpolation, we can remove the params values as they are already interpolated in the URL, making the approach simpler.

delete request.params;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sanjai0py Thanks a lot for the hint (I'm still working on my knowledge of Bruno), I've just updated my PR. Let me know if it's ok, hope to be (somehow) helpful

@helloanoop
Copy link
Contributor

Thanks @fgrande for investigating the issue and coming up with the fix.

I feel the code related to deleting the path params is better to be kept inside the configureRequest instead of having it in interpolate vars function. I have asked @sanjai0py to make this change right away in a separate PR.

Closing this PR. Thanks again for your work on this.

@helloanoop helloanoop closed this Jun 17, 2024
@fgrande fgrande deleted the fg/Issue2439 branch June 17, 2024 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants