fix: only add query params to GET requests #21
Conversation
| @@ -189,7 +189,7 @@ private Uri BuildUri(KnockRequest request) | |||
| builder.Append(ApiBaseURL); | |||
| builder.Append(request.Path); | |||
|
|
|||
| if (request.Method != HttpMethod.Post && options != null) | |||
| if (request.Method == HttpMethod.Get && options != null) | |||
There was a problem hiding this comment.
Just to confirm: are we 100% about this change? What about DELETE requests? It's generally expected that deletes use query params > request bodies iirc
There was a problem hiding this comment.
Yeah I was on the fence with how to treat this here given future state of generated SDKs from a spec.
I double-checked all of our documented endpoints, and we currently only use query params on GET-method endpoints except for two PUT endpoints (for Slack and Teams, to revoke access to a given access_token), both of which are not currently in this SDK.
I started out thinking we should add cases for those requests but then realized the methods didn't exist in the SDK so I opted for the simpler solution. Right now, CreateHttpRequestMessage puts any non-GET request's Options in the request body, so current state of the SDK will duplicate Options in body + query params for any request that isn't a POST or GET.
Description
This SDK currently checks the HTTP method of a given request to see whether it should add the request
Optionsas query parameters (as opposed to in the body of the request). Unfortunately, it's currently targeted at any endpoint that does not have aPOSTmethod rather than to only the relevantGETrequests. This means we have somePUTrequests (like updating user preferences) which are passing query parameters unnecessarily.