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 query parameters for array values #606

Merged
merged 2 commits into from
Nov 21, 2024
Merged

Conversation

wallacecs007
Copy link
Contributor

We were creating a new snakeCaseKey=item for each item in the value array.

This was creating multiple instances of the snakeCaseKey query param if there is more than one item in an array.

If more than one value was passing more than one value in an array query parameter we were generating a URL that looks like this:
/v3/grants/9a8ce69e-61a5-42c5-bdda-1ab3c3745093/threads?limit=5&any_email=chase.w%40nylas.com&any_email=nick.b%40nylas.com

This change makes it so that the above URL instead produces the following request URL:
/v3/grants/9a8ce69e-61a5-42c5-bdda-1ab3c3745093/threads?limit=5&any_email=chase.w%40nylas.com,nick.b%40nylas.com

License

I confirm that this contribution is made under the terms of the MIT license and that I have the authority necessary to make this contribution on behalf of its copyright owner.

We were creating a new snakeCaseKey=item for each item in the value array.

This was creating multiple instances of the snakeCaseKey query param if there is more than one item in an array.
for (const item of value) {
url.searchParams.append(snakeCaseKey, item as string);
}
url.searchParams.append(snakeCaseKey, value.join(','));
Copy link
Collaborator

Choose a reason for hiding this comment

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

@wallacecs007 can you write a test? Also, will this apply only to the anyEmail query param or all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tjovicic I fixed the test that was failing due to this change and this will apply to all query parameters. I don't see any case in our API spec docs where we would want multiple of the same query parameter passed in to Nylas outside of Metadata.

Metadata has it's own case and will be unaffected.

Copy link
Contributor

Choose a reason for hiding this comment

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

@tjovicic This fix will apply to any query_params that are sent as an Array.

Copy link
Contributor

@SubashPradhan SubashPradhan left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

@wallacecs007 wallacecs007 merged commit 5cb01dd into main Nov 21, 2024
5 checks passed
@SubashPradhan SubashPradhan mentioned this pull request Nov 25, 2024
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.

3 participants