fix: honor request options

[leonsomed]

Description

This PR enforces honoring on queryParams, pathParams, and header like it was supported in version < 2.1.1.

Motivation and Context

Previous to version 2.1.1 queryParams, pathParams, and header were honored to remain as the options that were explicitly passed to the request method. However, version 2.1.1 introduces a change where it will initialize queryParams, pathParams, and header to an empty object when they are not passed to the request method.

The behavior described above causes an issue with requests that contain queryParams in the path. For example: /users?lucky=true, this path contains an embeded queryParam but it will be stripped away before the execution because it is not included in the queryParams object. Prior to version 2.1.1 this was not a problem because an undefined queryParams will be ignored, however after version 2.1.1 an undefined queryParams will be initialized to an empty object and it will cause the removal of the embedded lucky query param.

The reason this is important is because there are APIs that respond with link resources with embedded queryParams.

How Has This Been Tested?

Previously this call will strip away the lucky query param
client.request({ method: 'GET', path: '/users?lucky=true' });

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[tuckbick]

Good catch! I prefer making the change here like you've done as opposed to checking if the options are empty before operating on them: https://github.com/ExpediaGroup/service-client/blob/master/lib/http/request.js#L187-L203

I ran a couple benchmarks for the different ways to go about this change:

• empty options
• populated options

For the sake of being more clear (and given the test results above), can you implement the more explicit code seen in the 3rd case test case in the links above?

We'll get this added to the 2.x version too once this is merged in.

[leonsomed]

The only affected option is queryParams both pathParams and header do not get affected.
After further analysis I realized queryParams were not being stripped but instead they were being obscured by an additional query string, ie:

// this requests the following path, notice the double start query string character: ?

client.request({ method: 'GET', path: '/users?lucky=true', queryParams: {} });
// /users?lucky=true?

client.request({ method: 'GET', path: '/users?lucky=true', queryParams: { test: true } });
// /users?lucky=true?test=true

So the new approach ensures not breaking the existing query string in path, if it exists, ie:

client.request({ method: 'GET', path: '/users?lucky=true', queryParams: {} });
// /users?lucky=true&

client.request({ method: 'GET', path: '/users?lucky=true', queryParams: { test: true } });
// /users?lucky=true&test=true

[tuckbick]

I think we also need to account for if path already ends in a &. We wouldn't want to have ?foo=bar&&fizz=buzz.

Some options to address this:

• strip any trailing & before processing anything
• don't add a & if one already exists
• parse the path's query string into an object, merge options.queryParams into the object, then stringify and re-attach to the path

[leonsomed]

i took the third approach

[tuckbick]

Thanks! This is great! I'll get this merged and released in v2 and v3 asap.

[tuckbick]

@leonsomed Released in v2.1.2 and v3.0.1.