Skip to content

RestClientOptions and its properties should have setters #2027

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

Closed
konarx opened this issue Mar 13, 2023 · 6 comments
Closed

RestClientOptions and its properties should have setters #2027

konarx opened this issue Mar 13, 2023 · 6 comments

Comments

@konarx
Copy link

konarx commented Mar 13, 2023

It is related to changes that happened within the #1963 scope

Use case:
It is a common practice that the Authentication token is not provided during the initialization of the RestClient and has to be fetched later from a specific URI (eg. "/auth").

This was working in v108:

        _restClient = new RestClient("https://my-base-uri");

        var request = new RestRequest("auth")
            .AddJsonBody(new UserCredentials("user", "password123"));

        var response = await tempClient.PostAsync<LoginResponse>(request);
       
        _restClient.Authenticator = new JwtAuthenticator(response.Token);

This is a workaround in v109:

        //Creating a temporary RestClient only to perform a POST call to get the token
        var tempClient = new RestClient("https://my-base-uri");

        var request = new RestRequest("auth")
            .AddJsonBody(new UserCredentials("user", "password123"));
        
        var response = await tempClient.PostAsync<LoginResponse>(request);
           
        //In v109, the 'RestSharp.ReadOnlyRestClientOptions.Authenticator' has no setter.
        //For this case, a new instance of RestClient must be created
        _restClient = new RestClient(new RestClientOptions
           {
               BaseUrl = new Uri("https://my-base-uri"),
               Authenticator = new JwtAuthenticator(response.Token)
           });

Outcomes:
Instanciating 2 RestClient just to change one (or more) properties in Options is not optimal. My humble opinion is to create an instance of RestClient and to be able to change just the specific properties you may require.

@alexeyzimarev
Copy link
Member

I disagree that the options object exposed via the Options property must have setters. I closed around 10 issues when people were assigned options after clearing the client using those setters, and expecting the client to work differently. It defeats the purpose of options and goes against the options pattern where options are used to configure the service when it's instantiated. That's why none of the .NET components that uses options allows to change the options after the component is instantiated. That's also why they never expose the options object. However, RestSharp users want to inspect the options, so I made it public. It's also useful when options properties are used in extensions, that's why it's also exposed via the interface. It was quite a lot of work to achieve that, but it works now.

In short:

  • Most of the options are used to configure the HTTP client and the message handler. Changing those options won't have any effect, and it was confusing, people reported this behaviour as bugs.
  • Making the options mutable makes the client not thread-safe without wrapping each setter in a lock.
  • Changing the client behaviour at runtime causes unpredictability

What you want is to support the authentication flow and reuse the client to make the authentication call before making other calls. However, it's a completely different issue compared to making the options mutable.

@konarx
Copy link
Author

konarx commented Mar 13, 2023

Thank you @alexeyzimarev ,

I agree with your statement, so the feature request remains. The RestClient should not be instanciated twice to make the authentication call. If you agree, I will proceed and rephrase the topic & the description of this issue.

@alexeyzimarev
Copy link
Member

Isn't it a duplicate of #2024

@repo-ranger
Copy link

repo-ranger bot commented Mar 13, 2023

Duplicate issue created! Closing in 15 seconds...

@repo-ranger repo-ranger bot closed this as completed Mar 13, 2023
@alexeyzimarev alexeyzimarev reopened this Mar 13, 2023
@alexeyzimarev
Copy link
Member

Sorry about that, the repo ranger is too aggressive

@alexeyzimarev
Copy link
Member

Ok, it is certainly a duplicate of #2024, I renamed that issue to make it clear that it concerns the particular issue with reassigning the authenticator. The title of this issue is a bit confusing as I described the reason for making options immutable. Please add any further comments to #2024.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants