Safer buildQueryString

[johnwiseheart]

Before this PR

buildQueryString assumed it would take an object, and then iterated through its keys. However, because in both of its uses it was being passed an any, it was not guaranteed that it is an object. This lead to conjure-client-factory passing params.queryArguments = undefined, which would cause an error.

After this PR

==COMMIT_MSG==
Check that queryArguments is non-null before iterating over keys
==COMMIT_MSG==

[changelog-app]

Generate changelog in packages/conjure-client/changelog/@unreleased

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

Check that queryArguments is non-null before iterating over keys

Check the box to generate changelog(s)

• Generate changelog entry

[ferozco]

Looks like the typings are overly lenient. In practice this doesn't regularly come up since conjure-typescript correctly generates a map.

Since it is technically legal to pass anything as a query param, it doesn't hurt to be defensive

[ferozco]

this should be sufficient since Object.keys() seems to coerce all other types correctly

[ferozco]

👍

[svc-autorelease]

The autorelease label is not supported for this repository type. Please merge this PR first and then use the Autorelease UI

[svc-autorelease]

The autorelease label is not supported for this repository type. Please merge this PR first and then use the Autorelease UI

[ferozco]

We shouldn't cut a major rev of this repo just to change a typing that only impacts tests. I would prefer to either add a defensive check to ensure the object is not null

[johnwiseheart]

Removed break

[ferozco]

great! thanks for the fix @johnwiseheart

[ferozco]

looks like changelog-app is confused...

[johnwiseheart]

Yeah I think theres something weird going on, don't think https://github.com/palantir/conjure-typescript-runtime/blob/develop/changelog/%40unreleased/pr-82.v2.yml should still be in @unreleased either