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

809 set properties in http request message #1001

Conversation

james-s-tayler
Copy link
Contributor

What kind of change does this PR introduce?
This PR introduces a new feature to Refit. Being able to pass runtime state into the HttpRequestMessage.Properties dictionary in order to access it inside a DelegatingHandler by adding a Property attribute to a parameter.

public interface IGitHubApi
{
    [Post("/users/new")]
    Task CreateUser([Body] User user, [Property("SomeKey")] string someValue);

    [Post("/users/new")]
    Task CreateUser([Body] User user, [Property] string someOtherKey);
}

class RequestPropertyHandler : DelegatingHandler
{
    public RequestPropertyHandler(HttpMessageHandler innerHandler = null) : base(innerHandler ?? new HttpClientHandler()) {}

    protected override async Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        // See if the request has a the property
        if(request.Properties.ContainsKey("SomeKey")
        {
            var someProperty = request.Properties["SomeKey"];
            //do stuff
        }

        if(request.Properties.ContainsKey("someOtherKey")
        {
            var someOtherProperty = request.Properties["someOtherKey"];
            //do stuff
        }

        return await base.SendAsync(request, cancellationToken).ConfigureAwait(false);
    }
}

PR also includes:

  • full documentation on
    • how to use the feature
    • behavior of edge cases / internals
  • comprehensive set of unit tests covering:
    • adding a property with a key
    • adding a property without specifying its key
    • adding properties with duplicate keys
    • how the feature interacts with Query, Header, and Multipart parameters

What is the current behavior?
I opened issue #809 for it about a year ago.

Currently Refit doesn't support doing this natively, and it can only be worked around in a pretty hacky way with strings only.
That issue has seen some interest since opening it. The feature is available in RestEase and on the roadmap for Flurl.

What might this PR break?
The excellent code-coverage of the unit tests has ensured it hasn't broken anything fundamentally.
Additionally I've taken great care to add enough coverage to tease out any subtle bugs the feature might have introduced into Query, Header or Multipart functionality. Happy to add any additional test cases if deemed necessary.

Please check if the PR fulfills these requirements

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

Other information:
I love Refit and this closes a functionality gap making Refit the only one out of Refit, RestSharp, RestEase, and Flurl to support HttpClient, IHttpClientFactory, and populating properties into HttpRequestMessage.Properties

@dnfadmin
Copy link

dnfadmin commented Nov 22, 2020

CLA assistant check
All CLA requirements met.

Copy link
Member

@clairernovotny clairernovotny left a comment

Choose a reason for hiding this comment

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

This PR looks great, just one small thing and I can merge it.

@@ -206,6 +206,19 @@ public HeaderAttribute(string header)
public string Header { get; }
}

[AttributeUsage(AttributeTargets.Parameter)]
Copy link
Member

Choose a reason for hiding this comment

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

This type should have doc comments on it and its members

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added some docs to it :)

@@ -666,6 +675,11 @@ private async Task<T> DeserializeContentAsync<T>(HttpResponseMessage resp, HttpC
}
}

foreach (var property in propertiesToAdd)
{
ret.Properties.Add(property.Key, property.Value);
Copy link
Member

Choose a reason for hiding this comment

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

I know there's a test for the Dup, but Add won't throw here if the key is already present?

Copy link
Contributor Author

@james-s-tayler james-s-tayler Nov 24, 2020

Choose a reason for hiding this comment

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

I've updated this to use the indexer rather than Add method. I think propertiesToAdd never contains duplicate keys and since Refit itself is constructing the HttpRequestMessage before any DelegatingHandlers get to run I believe it's going to always be empty, so I think it's likely OK, but have changed it to more of a 'last write wins' in keeping with how the rest of the feature works and erring on the side of not blowing up if it ever happens.

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feature: Add the possibility to set Properties in HttpRequestMessage
3 participants