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

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

Merged
merged 8 commits into from
Jul 3, 2024

Conversation

WhitWaldo
Copy link
Contributor

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:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

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

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
…to be passed in via the SDK instead of being uncermoniously added to the end of the produced HttpRequestMessage URI

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
@WhitWaldo WhitWaldo requested review from a team as code owners June 19, 2024 12:22
…ting to work against Uri instead of HttpRequestMessage.

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
Copy link
Contributor

@philliphoff philliphoff left a comment

Choose a reason for hiding this comment

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

@WhitWaldo It looks like the build test failures are due to something in this PR (as they succeed in master). Can you take a look and see what's going on?

@philliphoff philliphoff added this to the v1.14 milestone Jun 25, 2024
@WhitWaldo
Copy link
Contributor Author

@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:

  • InvokeMethodAsync_VoidResponseNoHttpMethod_Success, InvokeMethodAsync_VoidresponseWithHttpMethod_Success, InvokeMethodAsync_VoidVoidNotHttpMethod_Success and InvokeMethodAsync_VoidVoidWithHttpMethod_Success are all failing because of "The client has 1 or more incomplete requests. Use request.Dismiss() if the test is uninterested in the response." which suggests some sort of shared client issue?
  • Seeing an error with TestWorkflowLogging that's failing because the file is in use by another process

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
Copy link
Contributor

@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.

/// <param name="uri">The uri to append the query string parameters to.</param>
/// <param name="queryStringParameters">The key/value pairs to populate the query string with.</param>
public static Uri AddQueryParameters(this Uri? uri,
IReadOnlyCollection<KeyValuePair<string, string>>? queryStringParameters)
Copy link
Contributor

Choose a reason for hiding this comment

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

Curious why not NameValueCollection?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fundamentally, I was looking to ensure that a read-only collection is passed. Conceptually, they wind up operationally working the same way. I don't see why an overload couldn't be added for a NameValueCollection as well.

…ry string and passing it as the payload for a request. Removed the offending method and reworked the remaining configurations so there's no API impact.

Signed-off-by: Whit Waldo <whit.waldo@innovian.net>
@WhitWaldo
Copy link
Contributor Author

@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.

@philliphoff philliphoff merged commit 3768a98 into dapr:master Jul 3, 2024
10 checks passed
@WhitWaldo WhitWaldo deleted the svc-invocation-query branch July 3, 2024 19:20
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.

Passing query string parameters on method invocation
3 participants