dapr / dotnet-sdk

Dapr SDK for .NET
Apache License 2.0
1.12k stars 340 forks source link

Added overload to support SDK supplying query string on invoked URL #1310

Closed WhitWaldo closed 5 months ago

WhitWaldo commented 5 months ago

Description

When a method is invoked via the Dapr service invocation building block, a query string can be passed through on the invocation URL without issue. However, when using the .NET SDK, this behavior isn't exposed anywhere. Existing overloads allow the app ID to be specified, the method named, the payload to be provided, but that's it - it's up to the developer to realize that a query string is supported, then augment the RequestUri property of the returned HttpRequestMessage to append their query string values, and proceed with the invocation.

This PR provides overloads to the invocation methods to allow an IReadOnlyCollection<KeyValuePair<string,string>> to be provided as the collection of key/value pairs to build/supplement the query string with. This uses a static extension method I've contributed (and added tests for) that appends the encoded key/value pairs provided in the collection to build a standards-matching query string so developers don't have to maintain the same on their own.

Why an IReadOnlyCollection instead of a Dictionary<string,string>? Because identical keys can exist more than once in a query string with different values, so this needs to be more of a collection and less of a mapping. And because at the end of the day, this is being written to a string-based URI, these methods expect string-based keys and values, leaving it to the developer to figure out how to serialize accordingly before calling this.

Finally, I had the existing concrete implementations point to the new overloads and pass empty collections so as to avoid code duplication.

Issue reference

Please reference the issue this PR will close: #1303

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Documentation PR extended here, though generically and not in a C#-specific way: https://github.com/dapr/docs/pull/4217

WhitWaldo commented 5 months ago

@philliphoff I've updated my branch and resynced my local branch to master. I'm running the tests locally and everything I contributed here is testing fine. I'm showing errors when testing locally that appear entirely unrelated to my contribution:

I don't necessarily mind tackling either of these, but I handling them in the context of this PR may be out of scope. I'll keep poking at it and see what my changes might have to do with the error.

philliphoff commented 5 months ago

@WhitWaldo I agree that I don't see an immediate connection to your changes, but since the tests only seem to fail in this branch, we can't commit it until that's sorted out.

WhitWaldo commented 5 months ago

@philliphoff I show that all the tests are passing as of the last commit. Figured out the issue - there was some ambiguity about which method should be invoked given all the overloads available. One of them, added as part of this issue, added:

public override HttpRequestMessage CreateInvokeMethodRequest(HttpMethod httpMethod, string appId, string methodName,
    IReadOnlyCollection<KeyValuePair<string, string>> queryStringParameters)

The existing overload was:

public override HttpRequestMessage CreateInvokeMethodRequest<TRequest>(HttpMethod httpMethod, string appId, string methodName, TRequest data)

And at runtime then, it was taking the query string parameters and instead dropping them in as the request payload. I've remedied by changing the latter overload signatures to also accept the IReadOnlyCollection<KeyValuePair<string, string>> and simply pass it an empty collection when it's not provided and both locally and in the CI/CD, everything looks to be working now.