[RFC] v109

[alexeyzimarev]

RFC for v109.

New features

Done, awaiting feedback

• IRestClient interface with Options, Serializers, ExecuteAsync, and DownloadStreamAsync
• Target frameworks: .NET 6, .NET 9, .NET Framework 4.7.1, .NET Standard 2.0
• Using the Configuration pattern for options, serializers, etc

Planned

• RestClientFactory implementation. It is unclear if we should use IHttpClientFactory or an internal factory like it's done by Flurl. Using both might work too.

Breaking changes

• Serializers configuration using a delegate passed to the constructor. UseXXX methods to configure serializers moved to the SerializerConfig
• Authenticator, Encode, EncodeQuery moved to RestClientOptions
• Options exposed by the client changed the type to IRestClientOptions where most of the properties are get-only. Also, properties that configure HttpClient or HttpMessageHandler are not included to IRestClientOptions.

Motivation

The client instance should not be as mutable as it is today. The current API allows making configuration changes at runtime, which can produce undesired side-effects when the client instance is used in multi-threaded scenarios.

Example serializer configuration

Before:

var client = new RestClient(url);
client.UseNewtonsoftJson();

After:

var client = new RestClient(url, configureSerializers: cfg => cfg.UseNewtonsoftJson());

[nesc58]

Sounds good. I have tested the prelease and it works good for me.
We have used the method BuildUri(request) for logging purpose because the uri contains the whole uri including query parameters. This changed to be internally now. Is it possible to keep the method public on the RestClient implementation or provided as extension method? I don't need the BuildUri method on the IRestClient interface. The interface should be clean and minimalistic and this method is not relevant for an implementation contract.

Thank you for your great work

[alexeyzimarev]

The BuildUrl method can't be an extension as it accesses the private properties of the client. If it is made public, it should be part of the interface. I also don't like it being there, need to think about it.

[nesc58]

The only property not including in the interface is DefaultParameters.
If this will not be a part of the interface or of the IRestClientOptions it could be optional:

public static Uri BuildUri(this IRestClient restClient, RestRequest request, ParametersCollection? defaultParameters = null) {
    defaultParameters   ??= new ParametersCollection();
    var (uri, resource) =   restClient.Options.BaseUrl.GetUrlSegmentParamsValues(request.Resource, restClient.Options.Encode, request.Parameters, defaultParameters);
    var mergedUri = uri.MergeBaseUrlAndResource(resource);

    var finalUri = mergedUri.ApplyQueryStringParamsValuesToUri(
        request.Method,
        restClient.Options.Encoding,
        restClient.Options.EncodeQuery,
        request.Parameters,
        defaultParameters
    );
    return finalUri;
}

If this method will not be accessable in future it is bad for me but solveable.

Another part I want to discuss is the IDisposable on IRestClient. I know it it really common to use IDispoable on interface level because it makes a lot of things easier. But the main purpose to inherit IDispoable is to release (unmanaged) resources. An interface self not contains any resources. But I can understand the descicion to use it on interface level ot make a lot of things easier.

The last point: Is it necessary that I can inherit RestClient? Currently there are no members to override. I think it could be public sealed class RestClient : IRestClient when no virtual members exists.

I am really looking forward to the next release 👍 The changes are great and I hope the people who hates the v108 will be happy to see the interface again and the new cleaned up configuration pattern

[alexeyzimarev]

I am not saying it won't be there. The code you pasted shows how many private functions it uses, not just properties.

I am not sure how people use it. For example, HttpClient doesn't expose how they build the URL internally, although it is quite primitive compared with RestSharp.

I have nothing against bringing it to the interface. The only issue is that for any composition-based implementation (think Polly-based retry client), the implementation of BuildUrl will be to call the wrapped client.

Considering the diagnostics need, there's an issue to add an ActivitySource and start activities when making requests. The activity payload can contain the URL, and adding a simple listener to the activity source would solve the logging needs.

That's why I am interested in using particular public functions, as they might be used for the needs of alternative solutions.

[Bidthedog]

Any chance of adding support for request-based cookies as per #1792?

[alexeyzimarev]

Any chance of adding support for request-based cookies as per #1792?

It's already there in v109 preview

[Bidthedog]

Oh, OK, sorry! I'll see if I can check it out today - thanks @alexeyzimarev :)

[rbobrowicz]

Hello,

I'm not sure if I should comment here or open a separate issue, but we noticed an issue with cookies in the v109 preview.

Essentially, the problem we have is that we use RestSharp to talk to a third party API that we don't control and that API sends us invalid cookies. They look something like this: Set-Cookie: [redacted]; Expires=Thu, 21 Dec 2023 01:16:59 GMT; Path=/; Domain=; Secure; HttpOnly; SameSite=None. The Domain=; bit is invalid as far as I understand the relevant cookie RFC.

Now, this used to work in v108, but in v109 it throws an error because the SetCookies() method of CookieContainer throws the following exception if you give it that header:

{System.Net.CookieException: An error occurred when parsing the Cookie header for Uri '[redacted]'.
 ---> System.Net.CookieException: The 'Domain'='' part of the cookie is invalid.
   at System.Net.Cookie.VerifySetDefaults(CookieVariant variant, Uri uri, Boolean isLocalDomain, String localDomain, Boolean setDefault, Boolean shouldThrow)
   at System.Net.CookieContainer.CookieCutter(Uri uri, String headerName, String setCookieHeader, Boolean isThrow)
   --- End of inner exception stack trace ---
   at System.Net.CookieContainer.CookieCutter(Uri uri, String headerName, String setCookieHeader, Boolean isThrow)
   at System.Net.CookieContainer.SetCookies(Uri uri, String cookieHeader)
   at RestSharp.RestClient.ExecuteRequestAsync(RestRequest request, CancellationToken cancellationToken)} 

Which leaves us between a rock and a hard place. We can't actually get the third party to fix their cookie headers, and it appears there is currently no way to disable response cookie parsing in v109 of RestSharp.

We don't actually care about the cookies at all, just the request body, so would it be possible to have a flag of some sort to disable cookie parsing so that an invaild cookie doesn't kill the request?

[alexeyzimarev]

I think it can be fixed. Will check before the new year.

[Bidthedog]

Do you have an expected / planned ETA for v109?

[alexeyzimarev]

I want to include the client factory in the new major, but I haven't figured it out yet.

[Bidthedog]

Is there anything I can help with, or is it one of those things you just need to hammer away at until it looks right? ;)

[alexeyzimarev]

Here's the discussion.

#1791

The easiest would be to do the same as Flurl does (use base URL as the cached instances key), but they neither do any lifecycle management nor handle cases when the HttpClient is (should be) configured differently even if the base URL is the same. I would prefer using IHttpClientFactory from Microsoft, but it won't cover those massive number of cases when people instantiate the new client instance for each request. There, the Flurl approach with some improvements like lifecycle management would be better.

[alexeyzimarev]

It's, basically, figuring out what to do rather than how. The how is relatively easy.

[Bidthedog]

I'll take a look at the thread in detail and see what the priorities of the client I'm doing the work for is - I'll let you know if I can help soon.

[hoerup]

What's the outlook of the 109 release ? We'd really like to upgrade to net7 but are not keen on using a preview release in production.

[alexeyzimarev]

The new preview is out. The largest (breaking) change compared with the previous preview is that the Options property returns a read-only version of the options. There's no way now to manipulate the options as it wasn't supposed to be used like that anyway, but some changes worked whilst other didn't. As a result, the Authenticator property is gone from the client as it's in the options and the setter didn't work anyway.

A couple of extensions for setting coding functions are marked obsolete and aggressively disabled by throwing a not implemented exception. I think they should be removed so the incompatibility can be found and resolved at runtime. The drawback is of course that the Obsolete attribute tells you want to do and if the function is gone you'd have to check the release notes.

[billybooth]

• Target frameworks: .NET 6, .NET 9, .NET Framework 4.7.1, .NET Standard 2.1

I think (and hope) that this is a typo and that NET Standard 2.0 is still going to be supported in v109.

[alexeyzimarev]

It is, you are right

        <TargetFrameworks>netstandard2.0;net471;net6.0;net7.0</TargetFrameworks>

[alexeyzimarev]

Btw, the media type issue is a bug in .NET 7, still open dotnet/runtime#81506
Another one is the reason why upload file tests are breaking now. It seems that the API endpoint doesn't return any response, although the request handler does return a valid response.

[alexeyzimarev]

I am still considering getting back the client-level cookie container for cases when a singleton client is used, and the container will have authentication cookies.

[alexeyzimarev]

Finally the v109 branch has all the PRs merged with all the test passing.

[nesc58]

For me everything works fine with. Thanks for working on RestSharp.

I have used the 109.0.0-preview2. Is there a newer package for the latest changes of the v109 branch to test?

[alexeyzimarev]

There's an issue there with cookies in the response. If the request has no cookies, which it doesn't by default, the response cookies won't be propagated to RestResponse.

[alexeyzimarev]

v109 is out. Big thanks to everyone who contributed by code and comments/suggestions!

[nsulikowski]

I used
BuildUri(this IRestClient restClient, RestRequest request, ParametersCollection? defaultParameters = null)
all over the place. Can you bring it back, in some way?
Thanks

[DaleMckeown]

@alexeyzimarev Docs need updating - https://restsharp.dev/serialization.html#newtonsoftjson-aka-json-net still recommends client.UseNewtonsoftJson();

[DaleMckeown]

Also, I think

var client = new RestClient(url, configureSerializers: cfg => cfg.UseNewtonsoftJson());

should be

var client = new RestClient(url, configureSerialization: cfg => cfg.UseNewtonsoftJson());

[alexeyzimarev]

Updated the docs

[DaleMckeown]

@alexeyzimarev Do you know what happened to client.BuildUri(restRequest);? It seems to have vanished.

[nsulikowski]

They made it internal. This library really likes changing everything around very frequently. It's painful.
Could we bring BuildUri back, or a way to get the url used for the request?

[DaleMckeown]

Agree, very painful. I've got 30+ projects failing builds right now. Think I'll be sticking with 108 for the time being until the bugs are ironed out.

[alexeyzimarev]

@nsulikowski the BuildUri method is not an extension, it uses the client DefaultParameters property, which is not part of the interface.

I actually found out that exposing RestClient.DefaultParameter makes the client not thread-safe if someone would use DefaultParameters.AddParameter (non-locking) instead of client.AddDefaultParameter (locking).

Not sure that to do about it though. I opened a new issue for it: #2025

[alexeyzimarev]

I think the discussion about BuildUri that happened in this issue somehow slipped away, my apologies. Let me know another issue about it. Please avoid non-constructive commends, and help me understand how to satisfy the needs properly. Check the #2025 issue about default parameters, it is related to BuildUri as the method needs to get the default URL segment parameters to build the request URI.

[DaleMckeown]

No worries, this happens in software development!

Not sure about other people's use cases but I was simply using client.BuildUri(restRequest); as a convenient way of getting the absolute URI of a request, instead of having to manually build it myself.

Mostly this is for response/error logging, and is easy for me to replicate using new Uri().

I've not come across any other breaking issues, but there could be other methods that are no longer publically exposed in RestSharp.

[alexeyzimarev]

Thanks for the explanation. It would be great to continue the conversation in #2026.

[alexeyzimarev]

I've not come across any other breaking issues, but there could be other methods that are no longer publically exposed in RestSharp.

I don't think so, as RestClient didn't have many public methods, and BuildUrl was one of the few. The reason it was made internal is to define the interface scope. I didn't want to have public members on the client level that won't be a part of the interface. I still failed to achieve this as the DefaultParameters is public, and AddDefaultParameter is also public. That's why I now created an issue for that too. Basically, solving the default parameters issue would also help solving the BuildUri problem.

[karelz]

Btw, the media type issue is a bug in .NET 7, still open dotnet/runtime#81506 Another one is the reason why upload file tests are breaking now. It seems that the API endpoint doesn't return any response, although the request handler does return a valid response.

@alexeyzimarev can you please help us understand how you ran into the .NET 7.0 runtime issue?
We are debating backport into servicing and if more people ran into it, that would help us make the case.
Commenting on the Runtime issue would be best, if it is ok for you. Thank you!

[alexeyzimarev]

@karelz I didn't, I got a few issues opened by people who migrated to .NET 7 and suddenly started to get their code crashing when it worked before. Because I didn't know the motivation behind that breaking change, it was fixed here, it wasn't hard.

[karelz]

@alexeyzimarev ok, so your (RestSharp) customers ran into the breaking change in Runtime, but reported it to you and you fixed it (worked around it) in RestSharp. Did I get it right?
Is there link to the PR that worked around it in RestSharp?
Is there list of people who ran into it and reported it to you?

Thanks for your help BTW, appreciate it! (hopefully my questions are not too demanding)

[alexeyzimarev]

@alexeyzimarev ok, so your (RestSharp) customers ran into the breaking change in Runtime, but reported it to you and you fixed it (worked around it) in RestSharp. Did I get it right?

@karelz That's correct. Here are the issues:

• mediaType null or empty when using RestSharp 108.0.2 with .NET 7 #1969
• The value cannot be null or empty. (Parameter 'mediaType') #1999
• AddPostParameters with MultiPartFormDataContent throws error The value cannot be null or empty. (Parameter 'mediaType') #2000

It was fixed here as part of the v109 PR, which has lots of changes for that release. Here's the commit that addresses the media type header issue: 193b6ca