Feat access token cache #1249

EngRajabi · 2020-05-30T17:11:08Z

New Feature

Cache response access token

Motivation for New Feature

Improve performance for cache identity response.
Improve performance for cache disk file repository.
In large projects and with many simultaneous requests, this cache is required.

EngRajabi · 2020-06-23T10:05:23Z

@TomPallister

Hossein-Edraki

That's Good

raman-m · 2023-07-19T17:38:18Z

Hi Mohsen!
Thanks for the PR!

What issue is this PR related to?

raman-m · 2023-07-19T17:57:26Z

@EngRajabi commented on May 30, 2020:

improve performance for cache identity response.
improve performance for cache disk file repository.

In comparison to what performance indicators was the current performance improved?
Could you rewrite the description of your feature request please?

EngRajabi · 2023-07-20T07:21:25Z

I was in an enterprise project that used the reference token type. I had a performance problem. Because in every request, it needs to request the identity of the server. The problem was solved with this cache.

EngRajabi

merge conflict

raman-m · 2023-07-20T13:21:36Z

@EngRajabi commented on July 20, 2023

Did you use Service Discovery + Identity Server setup, or classic setup + Identity Server Bearer Tokens?
🆗 We will talk more during code review...

raman-m · 2023-07-20T13:22:52Z

Unfortunately there are no builds for these commits!
So, I cannot start code review without having an actual valid/green build...

raman-m · 2023-08-01T13:29:35Z

Mohsen, congrats! 🎉
The feature branch has been rebased onto ThreeMammals:develop successfully!

Now we can go with code review...

And, once again,
What issue is this PR related to?

RaynaldM · 2023-08-01T14:21:36Z

src/Ocelot/Configuration/Repository/DiskFileConfigurationRepository.cs

            _environmentFilePath = $"{AppContext.BaseDirectory}{ConfigurationFileName}{(string.IsNullOrEmpty(hostingEnvironment.EnvironmentName) ? string.Empty : ".")}{hostingEnvironment.EnvironmentName}.json";
+            _cacheKey = "InternalDiskFileConfigurationRepository";


could be a constant

Agree with Ray!
It should be public static const field or property.

What is the strange key value?
Can we write "BlaBlaBlaKey"? 🤣

Suggested change

_cacheKey = "InternalDiskFileConfigurationRepository";

_cacheKey = nameof(DiskFileConfigurationRepository);

raman-m

I have found

1 major issue
a few minor issues aka hardcode
minor styling & formatting issues

Could you fix them, Mohsen, please?

raman-m · 2023-08-04T11:36:11Z

src/Ocelot.Provider.Consul/ConsulFileConfigurationRepository.cs

@@ -83,7 +83,7 @@ public async Task<Response> Set(FileConfiguration ocelotConfiguration)

            if (result.Response)
            {
-                _cache.AddAndDelete(_configurationKey, ocelotConfiguration, TimeSpan.FromSeconds(3), _configurationKey);
+                _cache.AddAndDelete(_configurationKey, ocelotConfiguration, TimeSpan.FromSeconds(5), _configurationKey);


${\color{orange}Hardcode\ has\ been\ detected!}$

It is better to define public static const field.
Is this value used somewhere else?

As I understood, we cache for 5 seconds, right?
Why 3 or 5 seconds?

raman-m · 2023-08-04T11:43:24Z

src/Ocelot/Authentication/Middleware/AuthenticationMiddleware.cs

        public AuthenticationMiddleware(RequestDelegate next,
-            IOcelotLoggerFactory loggerFactory)
+            IOcelotLoggerFactory loggerFactory, IMemoryCache memoryCache)


Styling issue

Suggested change

public AuthenticationMiddleware(RequestDelegate next,

IOcelotLoggerFactory loggerFactory)

IOcelotLoggerFactory loggerFactory, IMemoryCache memoryCache)

public AuthenticationMiddleware(

RequestDelegate next,

IOcelotLoggerFactory loggerFactory,

IMemoryCache memoryCache)

Goal: Each param is located on own line.
Or, put these params into one line. It's OK because there are up to 3 params only.

raman-m · 2023-08-04T11:45:05Z

src/Ocelot/Authentication/Middleware/AuthenticationMiddleware.cs

+                Logger.LogInformation(
+                    $"{httpContext.Request.Path} is an authenticated route. {MiddlewareName} checking if client is authenticated");


Formatting issue: Should be one line!

raman-m · 2023-08-04T12:04:15Z

src/Ocelot/Authentication/Middleware/AuthenticationMiddleware.cs

+                    var result =
+                        await httpContext.AuthenticateAsync(downstreamRoute.AuthenticationOptions
+                            .AuthenticationProviderKey);


Formatting issue: 3 lines!
But it should be one line actually.

raman-m · 2023-08-04T12:05:26Z

src/Ocelot/Authentication/Middleware/AuthenticationMiddleware.cs

+
+                    userClaim = result.Principal;
+
+                    _memoryCache.Set(cacheKey, userClaim, TimeSpan.FromMinutes(5));


${\color{orange}Hardcode\ has\ been\ detected!}$

5 is hardcode.
Should be public static const field.

raman-m · 2023-08-04T12:07:48Z

src/Ocelot/Configuration/Repository/DiskFileConfigurationRepository.cs

            _environmentFilePath = $"{AppContext.BaseDirectory}{ConfigurationFileName}{(string.IsNullOrEmpty(hostingEnvironment.EnvironmentName) ? string.Empty : ".")}{hostingEnvironment.EnvironmentName}.json";
+            _cacheKey = "InternalDiskFileConfigurationRepository";


Agree with Ray!
It should be public static const field or property.

What is the strange key value?
Can we write "BlaBlaBlaKey"? 🤣

raman-m · 2023-08-04T12:10:23Z

src/Ocelot/Configuration/Repository/DiskFileConfigurationRepository.cs

            _environmentFilePath = $"{AppContext.BaseDirectory}{ConfigurationFileName}{(string.IsNullOrEmpty(hostingEnvironment.EnvironmentName) ? string.Empty : ".")}{hostingEnvironment.EnvironmentName}.json";
+            _cacheKey = "InternalDiskFileConfigurationRepository";


Suggested change

_cacheKey = "InternalDiskFileConfigurationRepository";

_cacheKey = nameof(DiskFileConfigurationRepository);

raman-m · 2023-08-04T12:15:48Z

src/Ocelot/Configuration/Repository/DiskFileConfigurationRepository.cs

@@ -63,6 +76,9 @@ public Task<Response> Set(FileConfiguration fileConfiguration)
            }

            _changeTokenSource.Activate();
+
+            _cache.AddAndDelete(_cacheKey, fileConfiguration, TimeSpan.FromMinutes(5), _cacheKey);


${\color{orange}Hardcode\ has\ been\ detected!}$

5 should be public static const field or property.

Why do you use _cacheKey as a value of the region parameter?

raman-m · 2023-08-04T12:56:19Z

src/Ocelot/Configuration/Repository/DiskFileConfigurationRepository.cs

+            {
+                return Task.FromResult<Response<FileConfiguration>>(new OkResponse<FileConfiguration>(configuration));
+            }
+


${\color{red}No\ adding\ the\ value\ to\ the\ cache!}$

Current implementation reads the value from the cache, and returns the value if it exists.

When the cache is empty, after reading the file content in lock and deserialization in line 52, we have to add the value to the cache immediately.

So, you forgot to renew the cache by Add method after the line 52:

lock (_lock) { jsonConfiguration = System.IO.File.ReadAllText(_environmentFilePath); } var fileConfiguration = JsonConvert.DeserializeObject<FileConfiguration>(jsonConfiguration); // TODO Add this line _cache.AddAndDelete(_cacheKey, fileConfiguration, TimeSpan.FromMinutes(5), _cacheKey); // OR _cache.Add(_cacheKey, fileConfiguration, TimeSpan.FromMinutes(5), _cacheKey); // return

raman-m · 2023-08-04T13:15:56Z

@EngRajabi

Questions 👇

Based on PR title "Feat access token cache", I would say that I cannot get: how do you manage caching of access token?
What did I see in the PR that you manage content of the file configuration.
Do we need to change the title of PR?
Or
Did you forget to write a code for caching of access token?

Maybe you were inspired by some issue from backlog?
What issue is this PR related to?

src/Ocelot/Configuration/Repository/DiskFileConfigurationRepository.cs

Wrap with braces

Hossein-Edraki approved these changes Oct 7, 2020

View reviewed changes

raman-m assigned EngRajabi Jul 19, 2023

raman-m changed the base branch from master to develop July 19, 2023 17:38

raman-m self-requested a review July 19, 2023 17:58

raman-m added feature A new feature question Initially seen a question could become a new feature or bug or closed ;) needs feedback Issue is waiting on feedback before acceptance waiting Waiting for answer to question or feedback from issue raiser labels Jul 19, 2023

EngRajabi commented Jul 20, 2023

View reviewed changes

raman-m force-pushed the feat_accessToken_cache branch from cd5675c to 0561ad5 Compare August 1, 2023 13:21

raman-m removed question Initially seen a question could become a new feature or bug or closed ;) waiting Waiting for answer to question or feedback from issue raiser labels Aug 1, 2023

raman-m requested review from binarymash, jps, geffzhang, RaynaldM, philproctor, anktsrkr, sharpn, abooraja, wast, yangzhongke and thiagoloureiro August 1, 2023 13:31

raman-m requested a review from Hossein-Edraki August 1, 2023 13:31

RaynaldM approved these changes Aug 1, 2023

View reviewed changes

wast approved these changes Aug 2, 2023

View reviewed changes

raman-m requested changes Aug 4, 2023

View reviewed changes

RaynaldM self-requested a review August 7, 2023 07:12

RaynaldM approved these changes Aug 7, 2023

View reviewed changes

src/Ocelot/Configuration/Repository/DiskFileConfigurationRepository.cs Show resolved Hide resolved

raman-m force-pushed the feat_accessToken_cache branch from 0561ad5 to c8c0634 Compare October 13, 2023 12:52

EngRajabi and others added 5 commits October 13, 2023 19:54

add cache

b5cc673

fix exeption

6d64de8

add cache file repository

2825067

SA1503 Braces should not be omitted.

5ea7861

Wrap with braces

Add required namespaces

4c2b93e

raman-m force-pushed the feat_accessToken_cache branch from c8c0634 to 4c2b93e Compare October 13, 2023 16:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat access token cache #1249

Feat access token cache #1249

EngRajabi commented May 30, 2020 •

edited by raman-m

EngRajabi commented Jun 23, 2020

Hossein-Edraki left a comment

raman-m commented Jul 19, 2023

raman-m commented Jul 19, 2023

EngRajabi commented Jul 20, 2023

EngRajabi left a comment

raman-m commented Jul 20, 2023

raman-m commented Jul 20, 2023 •

edited

raman-m commented Aug 1, 2023

RaynaldM Aug 1, 2023

raman-m Aug 4, 2023

raman-m Aug 4, 2023

raman-m left a comment

raman-m Aug 4, 2023 •

edited

raman-m Aug 4, 2023 •

edited

raman-m Aug 4, 2023 •

edited

raman-m Aug 4, 2023 •

edited

raman-m Aug 4, 2023 •

edited

raman-m Aug 4, 2023

raman-m Aug 4, 2023

raman-m Aug 4, 2023 •

edited

raman-m Aug 4, 2023

raman-m commented Aug 4, 2023 •

edited

		_environmentFilePath = $"{AppContext.BaseDirectory}{ConfigurationFileName}{(string.IsNullOrEmpty(hostingEnvironment.EnvironmentName) ? string.Empty : ".")}{hostingEnvironment.EnvironmentName}.json";
		_cacheKey = "InternalDiskFileConfigurationRepository";

	_cacheKey = "InternalDiskFileConfigurationRepository";
	_cacheKey = nameof(DiskFileConfigurationRepository);

		Logger.LogInformation(
		$"{httpContext.Request.Path} is an authenticated route. {MiddlewareName} checking if client is authenticated");


		userClaim = result.Principal;

		_memoryCache.Set(cacheKey, userClaim, TimeSpan.FromMinutes(5));

Feat access token cache #1249

Are you sure you want to change the base?

Feat access token cache #1249

Conversation

EngRajabi commented May 30, 2020 • edited by raman-m

New Feature

Motivation for New Feature

EngRajabi commented Jun 23, 2020

Hossein-Edraki left a comment

Choose a reason for hiding this comment

raman-m commented Jul 19, 2023

raman-m commented Jul 19, 2023

EngRajabi commented Jul 20, 2023

EngRajabi left a comment

Choose a reason for hiding this comment

raman-m commented Jul 20, 2023

raman-m commented Jul 20, 2023 • edited

raman-m commented Aug 1, 2023

RaynaldM Aug 1, 2023

Choose a reason for hiding this comment

raman-m Aug 4, 2023

Choose a reason for hiding this comment

raman-m Aug 4, 2023

Choose a reason for hiding this comment

raman-m left a comment

Choose a reason for hiding this comment

raman-m Aug 4, 2023 • edited

Choose a reason for hiding this comment

${\color{orange}Hardcode\ has\ been\ detected!}$

raman-m Aug 4, 2023 • edited

Choose a reason for hiding this comment

raman-m Aug 4, 2023 • edited

Choose a reason for hiding this comment

raman-m Aug 4, 2023 • edited

Choose a reason for hiding this comment

raman-m Aug 4, 2023 • edited

Choose a reason for hiding this comment

${\color{orange}Hardcode\ has\ been\ detected!}$

raman-m Aug 4, 2023

Choose a reason for hiding this comment

raman-m Aug 4, 2023

Choose a reason for hiding this comment

raman-m Aug 4, 2023 • edited

Choose a reason for hiding this comment

${\color{orange}Hardcode\ has\ been\ detected!}$

raman-m Aug 4, 2023

Choose a reason for hiding this comment

${\color{red}No\ adding\ the\ value\ to\ the\ cache!}$

raman-m commented Aug 4, 2023 • edited

Questions 👇

EngRajabi commented May 30, 2020 •

edited by raman-m

raman-m commented Jul 20, 2023 •

edited

raman-m Aug 4, 2023 •

edited

raman-m Aug 4, 2023 •

edited

raman-m Aug 4, 2023 •

edited

raman-m Aug 4, 2023 •

edited

raman-m Aug 4, 2023 •

edited

raman-m Aug 4, 2023 •

edited

raman-m commented Aug 4, 2023 •

edited