Enable access token caching #66

NRHelmi · 2023-04-04T11:11:44Z

Enabling access token caching by:

Adding a generic interface so that users can provide their custom access token handlers
Providing a default access token handler to cache token on disk

stanislav-bedunkevich · 2023-04-04T11:43:34Z

RelationalAI/AccessToken.cs

        public int ExpiresIn
        {
            get => _expiresIn;
            set => _expiresIn = value > 0 ? value : throw new ArgumentException("ExpiresIn should be greater than 0 ");
        }
+
+        public bool IsExpired => (DateTime.Now - DateTimeOffset.FromUnixTimeSeconds(CreatedOn)).TotalSeconds >= _expiresIn - 5; // Anticipate access token expiration by 5 seconds


can we get the expiration timestamp exposed next to this method? I'd like to control the renewal not based on this flag, but on the actual expiration time

torkins · 2023-04-04T12:00:47Z

RelationalAI/IAccessTokenHandler.cs

+    // Check DefaultAccessToken for an example
+    public interface IAccessTokenHandler
+    {
+        Rest Rest { get; set; }


The Rest client should not be part of the interface, nor the Logger. The interface is to simply provide token retrieval. Baking it into the interface conflates 1) The fact of even using the Rest client to do the implementation and 2) That the implementation logs at a ll, much less using the MS Logging extension.

makes sense thanks

torkins · 2023-04-04T12:02:32Z

RelationalAI/Client.cs

        {
            _context = context;
            _logger = logger ?? new LoggerFactory().CreateLogger("RAI-SDK");
-            _rest = new Rest(context, _logger);
+            _rest = new Rest(context, _logger, accessTokenHandler);


please implement the default access token handler creation here in the client, instead of in the REST layer, to keep the REST layer as thin as possible (think, as close to Curl as possible)

torkins · 2023-04-04T12:03:36Z

RelationalAI/DefaultAccessTokenHandler.cs

+    {
+        private static readonly SemaphoreSlim SemaphoreSlim = new SemaphoreSlim(1);
+
+        public Rest Rest { get; set; }


remove these public REST/Logger properties after fixing the interface as described in the interface

torkins · 2023-04-04T12:06:25Z

RelationalAI/DefaultAccessTokenHandler.cs

+        {
+            try
+            {
+                var data = File.ReadAllText(CacheName());


this class should fallback to in-memory caching if it cannot find the cache folder

that's right, this behavior is already handled in the caller method GetAccessTokenAsync() in the Rest class but it should be part of the handler implementation, moving it here

torkins · 2023-04-04T12:09:11Z

RelationalAI/DefaultAccessTokenHandler.cs

+                    dict.Add(creds.ClientId, token);
+                }
+
+                File.WriteAllText(CacheName(), JsonConvert.SerializeObject(dict));


similar comment, should fallback to memory cache is no file location

torkins · 2023-04-04T12:09:34Z

RelationalAI/Rest.cs

        {
            _context = context;
            HttpClient = new HttpClient();
            _logger = logger ?? new LoggerFactory().CreateLogger("RAI-SDK");
+
+            // Init AccessTokenHandler
+            AccessTokenHandler = accessTokenHandler ?? new DefaultAccessTokenHandler();


move default behavior into client

torkins · 2023-04-04T12:10:03Z

RelationalAI/Rest.cs

        }

        public HttpClient HttpClient { get; set; }

+        public IAccessTokenHandler AccessTokenHandler { get; set; }


why is this public?

the purpose is to make the Client class able to set the access token handler on the Rest class. Will change this field to make it internal
Otherwise we will need to use the Rest constructor the inject the AccessTokenHandler something like

public Client(Context context, ILogger logger = null, IAccessTokenHandler accessTokenHandler = null) { _context = context; _logger = logger ?? new LoggerFactory().CreateLogger("RAI-SDK"); _rest = new Rest(context, _logger, accessTokenHandler ?? new DefaultAccessTokenHandler(new Rest(context, null, null)); }

vs (current implementation)

public Client(Context context, ILogger logger = null, IAccessTokenHandler accessTokenHandler = null) { _context = context; _logger = logger ?? new LoggerFactory().CreateLogger("RAI-SDK"); _rest = new Rest(context, _logger); _rest.AccessTokenHandler = accessTokenHandler ?? new DefaultAccessTokenHandler(_rest); }

I lean toward using the second option

torkins · 2023-04-04T12:14:01Z

RelationalAI/Rest.cs

+                { "grant_type", "client_credentials" }
+            };
+            var resp = await RequestHelperAsync("POST", creds.ClientCredentialsUrl, data);
+            if (!(resp is string stringResponse))


In what scenario does this happen?

I'm not the one who implemented the body of this method RequestAccessTokenAsync, just making it public so that the IAccessTokenHandler implementations can use it to fetch new tokens

torkins · 2023-04-04T12:16:46Z

RelationalAI/AccessToken.cs

-            _createdOn = DateTime.Now;
+            _expiresIn = expiresIn;
+            Scope = scope;
+            CreatedOn = new DateTimeOffset(DateTime.Now).ToUnixTimeSeconds();


This is not the token creation time, this is the objects creation time (which could be created during deserialization from a cache for example). Take the creation time as an argument instead.

torkins · 2023-04-04T12:19:49Z

Needs test for default and non-default behavior

torkins · 2023-04-05T12:22:46Z

RelationalAI/Rest.cs

@@ -41,21 +41,16 @@ public class Rest

        private readonly ILogger _logger;

-        public Rest(Context context, ILogger logger, IAccessTokenHandler accessTokenHandler)
+        public Rest(Context context, ILogger logger)


Can we relax the ctor API here to default to null so the arg is not required at all for logger, now that clients are much more likely to use the direct Rest ctor, it'd be nice to not force ugly calls of Rest(ctx, null)

torkins · 2023-04-05T12:23:49Z

RelationalAI/Rest.cs

        }

        public HttpClient HttpClient { get; set; }

-        public IAccessTokenHandler AccessTokenHandler { get; set; }
+        internal IAccessTokenHandler AccessTokenHandler { get; set; }


why does the assembly need access to this prop?

Making the field internal so that the client class can set and manipulate the access token handler in the rest class

RelationalAI.Test/DefaultAccessTokenHandlerTests.cs

torkins · 2023-04-05T12:25:01Z

RelationalAI/DefaultAccessTokenHandler.cs

@@ -32,34 +31,40 @@ namespace RelationalAI
    public class DefaultAccessTokenHandler : IAccessTokenHandler


Needs handling for when filename not found -> INFO log msg, fallback to memory-based cache local ot the tokenhandler

torkins · 2023-04-11T12:33:12Z

CHANGELOG.md

@@ -1,5 +1,10 @@
 # Changelog

+# main


can you change to 0.9.13-alpha

torkins · 2023-04-11T12:42:00Z

RelationalAI.Test/DefaultAccessTokenHandlerTests.cs


 namespace RelationalAI.Test
 {
    [Collection("RelationalAI.Test")]
    public class DefaultAccessTokenHandlerTests : UnitTest
    {
+        private string TestCachePath()
+        {
+            var envHome = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "HOMEPATH" : "HOME";


please use the C# corelib env var for this for better portability: https://learn.microsoft.com/en-us/dotnet/api/system.environment.specialfolder?view=net-7.0

(System.Environment.UserProfile)

torkins · 2023-04-11T12:48:22Z

RelationalAI/DefaultAccessTokenHandler.cs

@@ -59,12 +62,12 @@ public async Task<AccessToken> GetAccessTokenAsync(ClientCredentials creds)
            return creds.AccessToken;
        }

-        private string CachePath()
+        private string DefaultCachePath()
        {
            var envHome = RuntimeInformation.IsOSPlatform(OSPlatform.Windows) ? "HOMEPATH" : "HOME";


same as in comment in test please use corelib portable environment enum

torkins

Please make the changes I mentioned

changelog adding version,
use corelib env enums everywhere

enable access token caching

e030ae6

stanislav-bedunkevich reviewed Apr 4, 2023

View reviewed changes

torkins reviewed Apr 4, 2023

View reviewed changes

torkins requested changes Apr 4, 2023

View reviewed changes

NRHelmi added 3 commits April 4, 2023 18:28

addressing PR comments

31bd146

fix

1db4488

convenient test

5e4ff46

NRHelmi requested review from stanislav-bedunkevich and torkins April 4, 2023 23:02

NRHelmi marked this pull request as ready for review April 4, 2023 23:03

torkins requested changes Apr 5, 2023

View reviewed changes

NRHelmi added 2 commits April 6, 2023 12:19

addressing PR comments

10f1af7

changelog update

028cbb6

NRHelmi requested a review from torkins April 6, 2023 11:24

torkins reviewed Apr 11, 2023

View reviewed changes

CHANGELOG.md Outdated

@@ -1,5 +1,10 @@

# Changelog

# main

Copy link

Collaborator

torkins Apr 11, 2023

Choose a reason for hiding this comment

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

can you change to 0.9.13-alpha

torkins reviewed Apr 11, 2023

View reviewed changes

torkins approved these changes Apr 11, 2023

View reviewed changes

fix changelog version & using userProfile enum to get homedir

c99bfa6

NRHelmi merged commit 79dad30 into main Apr 11, 2023

NRHelmi deleted the hnr-rai-11583-caching branch April 11, 2023 23:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable access token caching #66

Enable access token caching #66

NRHelmi commented Apr 4, 2023 •

edited

Loading

stanislav-bedunkevich Apr 4, 2023

torkins Apr 4, 2023

NRHelmi Apr 4, 2023

torkins Apr 4, 2023

torkins Apr 4, 2023

torkins Apr 4, 2023

NRHelmi Apr 4, 2023 •

edited

Loading

torkins Apr 4, 2023

torkins Apr 4, 2023

torkins Apr 4, 2023

NRHelmi Apr 4, 2023

torkins Apr 4, 2023

NRHelmi Apr 4, 2023

torkins Apr 4, 2023

torkins commented Apr 4, 2023

torkins Apr 5, 2023

torkins Apr 5, 2023

NRHelmi Apr 6, 2023

torkins Apr 5, 2023

torkins Apr 11, 2023

torkins Apr 11, 2023

torkins Apr 11, 2023

torkins left a comment

		@@ -32,34 +31,40 @@ namespace RelationalAI
		public class DefaultAccessTokenHandler : IAccessTokenHandler

Enable access token caching #66

Enable access token caching #66

Conversation

NRHelmi commented Apr 4, 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

NRHelmi Apr 4, 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

torkins commented Apr 4, 2023

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

torkins left a comment

Choose a reason for hiding this comment

NRHelmi commented Apr 4, 2023 •

edited

Loading

NRHelmi Apr 4, 2023 •

edited

Loading