Added Option to add Interceptors on Client Level #2108

fseidl-bauradar · 2023-06-28T13:49:21Z

Description

Added Possibility to add Client wide Interceptors, to check Request and Responses on multiple positions

Purpose

This pull request is a:

Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist

I have added tests that prove my fix is effective or that my feature works
I have added necessary documentation (if appropriate)

fseidl-bauradar · 2023-06-28T13:51:01Z

Added because of changes #2076

alexeyzimarev · 2023-07-04T08:40:46Z

.gitignore

+.temp/
+global.json


global.json should not be ignored.

Ok, I will keep this in mind. I thought that it would be a IDE config file

alexeyzimarev · 2023-07-04T08:41:41Z

RestSharp.sln

@@ -35,7 +35,7 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "RestSharp.Tests.Serializers
 EndProject
 Project("{2150E333-8FDC-42A3-9474-1A3956D46DE8}") = "SourceGen", "SourceGen", "{55B8F371-B2BA-4DEE-AB98-5BAB8A21B1C2}"
 EndProject
-Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "SourceGenerator", "gen\SourceGenerator\SourceGenerator.csproj", "{FE778406-ADCF-45A1-B775-A054B55BFC50}"
+Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "SourceGenerator", "gen\SourceGenerator\SourceGenerator.csproj", "{FE778406-ADCF-45A1-B775-A054B55BFC50}"


Changes to this file aren't required. Also, adding Appveyor-specific build targets need to be removed.

Seems like my ide is doing this by itself, I will try to keep this out from git in the future

alexeyzimarev · 2023-07-04T08:42:25Z

src/RestSharp/Options/RestClientOptions.cs

@@ -64,6 +65,8 @@ public class RestClientOptions {
    /// </summary>
    public IAuthenticator? Authenticator { get; set; }

+    public List<Interceptor> Interceptors { get; set; } = new();


It should not be a list as lists are mutable, and the list of interceptors being mutable will make the client not thread safe.

Ok, that's a point I didn't had in mind, is there a way to allow adding a interceptor after creating a inititing the client.

Currently I am using a Interceptor to refresh the jwt Token if it's outdated. For this I need a RestClient to access the backend over a Service.

You can convert it to array or IReadOnlyCollection

But doesn't mean this that it is impossible to add an interceptor after initiating the client?

Ok, I checked the Microsoft Reference, as I understand now you can not change the list directly, but you can change the list that is wrapped by ReadOnlyCollection.
But how do we get the wrapped List? Should we add a Methode for adding a interceptor.
Or is there a better possibility doing this.

As fas as I see there are two ways to solve this.

class RestClientOptions { private ImmutableList<InterceptorBase> _interceptors; public void AddInterceptor(InterceptorBase interceptor){ object mutex = new object(); lock(mutex) { if(_interceptors.Contains) return; _interceptors = _interceptors.Add(interceptor); } } }

As I see there should be a solutiong for ensuring that the AuthenticationInterceptor is the first in the list.

Therefore I would suggest provide an option to define the Authenticator, after the client have been created.

class RestClientOptions { public void SetAuthenticator(IAutenticator authenticator){ object mutex = new object; lock(mutex){ Authenticator = authenticator; } } }

Do you overtake functions defined in the RestClientOptions into the RestClientReadonlyOptions?

I am not sure adding mutation functions to the options class is a great idea. It's just a property bag, and the current source generator treats it like that. But I get your point. Let me think about for a bit.

alexeyzimarev · 2023-07-04T08:44:50Z

src/RestSharp/RestClient.Async.cs

-
-            var responseMessage = await HttpClient.SendAsync(message, request.CompletionOption, ct).ConfigureAwait(false);
-
+            responseMessage = await HttpClient.SendAsync(message, request.CompletionOption, ct).ConfigureAwait(false);


What was the reason to move the declaration of responseMessage to line 100?

The reason, was that I handle the response inside the interceptor (line 123) and prevent the tryCatch-Block from, catching exceptions thrown inside the interceptor.
For example I use an interceptor, to preprocess ErrorMessage to convert them back into Exceptions.

alexeyzimarev · 2023-07-04T08:45:36Z

src/RestSharp/RestSharp.csproj

@@ -39,4 +39,7 @@
    <ItemGroup>
        <ProjectReference Include="$(RepoRoot)\gen\SourceGenerator\SourceGenerator.csproj" OutputItemType="Analyzer" ReferenceOutputAssembly="false" />
    </ItemGroup>
+    <ItemGroup>


It doesn't sound right, adding this package.

I think, we can remove it, seems like I added it by accident, is still building after removing it.

If you commit the change, I'll mark this as resolved. Then I will merge your PR and figure out what to do with mutability.

alexeyzimarev · 2023-07-04T08:46:17Z

test/RestSharp.InteractiveTests/RestSharp.InteractiveTests.csproj

@@ -8,4 +8,7 @@
        <ProjectReference Include="$(RepoRoot)\src\RestSharp\RestSharp.csproj" />
        <ProjectReference Include="..\RestSharp.Tests.Shared\RestSharp.Tests.Shared.csproj" />
    </ItemGroup>
+    <ItemGroup>
+      <PackageReference Include="xunit.assert" Version="2.4.2" />


This project doesn't change, why did you add a reference?

Think this happed during checking, why the failing tests not working, wanted to find the reason for this.

alexeyzimarev · 2023-07-04T08:47:06Z

test/RestSharp.Tests.Serializers.Xml/SampleClasses/twitter.cs

@@ -105,7 +105,7 @@ public class complexStatus {
    public string in_reply_to_screen_name { get; set; }

    // ignore contributors for now
-    [DeserializeAs(Name = "user.following")]
+    [DeserializeAs(Name = "User.following")]


That's wrong as the message format is camel case.

This, was just a thing I found, I just changed the name of the User-Class to starting UpperCase, according to the C# Convention for Classes

Yeah, I get it. Have the same urge to fix things sometimes. But, it is better to keep the PR scope to its purpose as things might get break unexpectedly.

We can let it unchanged if you want, it's not as important for me

alexeyzimarev · 2023-07-13T11:23:18Z

@fseidl-bauradar I am ready to merge this if you find time to address my comments.

fseidl-bauradar mentioned this pull request Jun 28, 2023

Added Option to add Interceptors on Client Level #2076

Closed

5 tasks

alexeyzimarev reviewed Jul 4, 2023

View reviewed changes

fseidl-bauradar closed this Jul 19, 2023

fseidl-bauradar force-pushed the dev branch 2 times, most recently from 901e5c3 to 6197f31 Compare July 19, 2023 09:11

fseidl-bauradar mentioned this pull request Jul 19, 2023

Added Option to add Interceptors on Client Level #2118

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Option to add Interceptors on Client Level #2108

Added Option to add Interceptors on Client Level #2108

fseidl-bauradar commented Jun 28, 2023 •

edited

Loading

fseidl-bauradar commented Jun 28, 2023

alexeyzimarev Jul 4, 2023

fseidl-bauradar Jul 18, 2023

alexeyzimarev Jul 4, 2023

fseidl-bauradar Jul 18, 2023 •

edited

Loading

alexeyzimarev Jul 4, 2023

fseidl-bauradar Jul 18, 2023

alexeyzimarev Jul 18, 2023

fseidl-bauradar Jul 18, 2023 •

edited

Loading

fseidl-bauradar Jul 18, 2023 •

edited

Loading

fseidl-bauradar Jul 19, 2023 •

edited

Loading

fseidl-bauradar Jul 19, 2023

fseidl-bauradar Jul 19, 2023 •

edited

Loading

fseidl-bauradar Jul 19, 2023

alexeyzimarev Jul 19, 2023

alexeyzimarev Jul 4, 2023

fseidl-bauradar Jul 18, 2023 •

edited

Loading

alexeyzimarev Jul 4, 2023

fseidl-bauradar Jul 18, 2023

alexeyzimarev Jul 19, 2023

alexeyzimarev Jul 4, 2023

fseidl-bauradar Jul 18, 2023

alexeyzimarev Jul 4, 2023

fseidl-bauradar Jul 18, 2023

alexeyzimarev Jul 18, 2023

fseidl-bauradar Jul 18, 2023

alexeyzimarev commented Jul 13, 2023


		var responseMessage = await HttpClient.SendAsync(message, request.CompletionOption, ct).ConfigureAwait(false);

		responseMessage = await HttpClient.SendAsync(message, request.CompletionOption, ct).ConfigureAwait(false);

		.temp/
		global.json

Added Option to add Interceptors on Client Level #2108

Added Option to add Interceptors on Client Level #2108

Conversation

fseidl-bauradar commented Jun 28, 2023 • edited Loading

Description

Purpose

Checklist

fseidl-bauradar commented Jun 28, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fseidl-bauradar Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fseidl-bauradar Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

fseidl-bauradar Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

fseidl-bauradar Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fseidl-bauradar Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fseidl-bauradar Jul 18, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeyzimarev commented Jul 13, 2023

fseidl-bauradar commented Jun 28, 2023 •

edited

Loading

fseidl-bauradar Jul 18, 2023 •

edited

Loading

fseidl-bauradar Jul 18, 2023 •

edited

Loading

fseidl-bauradar Jul 18, 2023 •

edited

Loading

fseidl-bauradar Jul 19, 2023 •

edited

Loading

fseidl-bauradar Jul 19, 2023 •

edited

Loading

fseidl-bauradar Jul 18, 2023 •

edited

Loading