Support for multiple tokens

[ZejdCicak]

Hello,

I wanted to use this library for an application I'm making that will allow customers to authenticate, and use the Moloni API. The issue I'm facing, since I will have multiple customers using this application, is that in the code I've noticed you use TTLCache with a token key, so once one of the customers would authenticate their token will be used for everyone.

Would you be open to a solution that will allow providing a custom class (that extends MyAuth) to clients constructor, this will allow using a custom caching client, for example Redis, rather than relaying on in memory cache as well as modify any other things like per client id key?

I would be willing to provide such solution, but I would like to know if you're willing to make changes or should I fork the repository?

Thank you in advance!

[saleweaver]

Hi @ZejdCicak ,

thanks for raising this - it's actually an overview on my side, usually the cache key should be a hash of the refresh token, not a static "token"... This library is built upon my other one, https://github.com/saleweaver/python-amazon-sp-api, where this is implemented as this already.

If you want to port it over, or create your own, you're very welcome to do so - I'll happily merge it.

Here's the implementation in the other lib
https://github.com/saleweaver/python-amazon-sp-api/blob/master/sp_api/auth/access_token_client.py

[ZejdCicak]

Thank you, I've provided a solution here. Please let me know what you think!

[saleweaver]

Thanks for that. I've only seen it on mobile, but I think now we're recreating the cache whenever a new client gets created (unless you explicitly pass an existing cache).

I like the cache service, nice one. Maybe we should make it a singleton?

Copilot recommended sth like this, I'll have a look tomorrow as well:

To address the caching limitations while leveraging the service provided in the pull request, here's an alternative approach that ensures shared caching across multiple client instances:

Alternative Approach

1. Singleton Cache Service:
   Use a singleton pattern for the CacheService to ensure all client instances share the same cache instance. For example:

   from threading import Lock
   
   class CacheServiceSingleton:
       _instance = None
       _lock = Lock()
   
       def __new__(cls, *args, **kwargs):
           with cls._lock:
               if not cls._instance:
                   cls._instance = CacheService(*args, **kwargs)
           return cls._instance

2. Modify Client Initialization:
   Update the client to use this singleton CacheService by default if no explicit cache service is provided:

   class Client:
       def __init__(self, cache_service=None):
           self.cache_service = cache_service or CacheServiceSingleton()
           # Other initialization code...

3. Shared Tokens:
   With the singleton approach, all clients will share the same cache, ensuring tokens are reused across clients, reducing redundant retrievals.

4. Custom Cache Configuration:
   If needed, allow customizing the cache configuration for the singleton:

   class CacheServiceSingleton:
       _instance = None
       _lock = Lock()
   
       @classmethod
       def configure(cls, *args, **kwargs):
           with cls._lock:
               cls._instance = CacheService(*args, **kwargs)
   
       def __new__(cls, *args, **kwargs):
           with cls._lock:
               if not cls._instance:
                   cls._instance = CacheService(*args, **kwargs)
           return cls._instance

   This way, the cache configuration can be set globally before any client instances are created.

---

This approach keeps the flexibility introduced by the abstract CacheService while maintaining the benefits of a shared, top-level cache for tokens. Would you like help implementing or refining this solution further?

[ZejdCicak]

I've done it this way due to having client id in constructor of the CacheService so we can build unique key for each client, I wasn't sure how would you feel about adding client_id to get_key contract.

If you think adding client_id to get_key contract (which IMO wouldn't be a bad idea overall) is okay, I would be happy to change it to singleton approach.

[ZejdCicak]

I have prepared changes like this, I just didn't push them yet in case you have another idea about client id. The contract looks like this now:

@abstractmethod
def get_key(self, client_id: str) -> str:
   """Generate a cache."""
    pass

While CacheServiceSingleton looks like this:

class CacheServiceSingleton(AbstractCacheService):
    _instance = None
    _lock = Lock()

    def __new__(cls):
        # Check if an instance already exists without acquiring the lock
        if cls._instance is None:
            with cls._lock:
                if cls._instance is None:
                    cls._instance = super().__new__(cls)
                    cls._instance._cache = TTLCache(maxsize=10, ttl=3600)

        return cls._instance

[saleweaver]

Let's try that..

[ZejdCicak]

Gotcha! It's in the PR now.

[ZejdCicak]

@saleweaver Sorry to ping you again, any update on this and another PR I submitted? I would really appreciate if you could check them since I'm in a bit of a hurry, and this is blocking me, and I would rather contribute to your project, instead of forking it. Thank you!