Skip to content
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

Configuration getter #72

Closed
ahopkins opened this issue Feb 22, 2018 · 13 comments
Closed

Configuration getter #72

ahopkins opened this issue Feb 22, 2018 · 13 comments
Assignees
Milestone

Comments

@ahopkins
Copy link
Owner

One of the new ways in Version 1 to create settings is with the setter function on the Configuration class.

from sanic_jwt import Configuration

class MyConfiguration(Configuration):
    def set_access_token_name(self):
        return 'jwt'

Initialize(
    app,
    configuration_class=MyConfiguration)

Then, the Initialization class goes and installs all the various config settings on a config object.

>>> print(config.access_token_name)
'jwt'

I propose that we change the way that config is used throughout the backend to make it a callable. Instead of doing config.access_token_name it would become config.get('access_token_name').

Why?

Then, we can not only have dynamic attributes set at initialization, but also when called.

class MyConfiguration(Configuration):
    async def get_access_token_name(self):
        return await some_fancy_function()
@vltr
Copy link
Collaborator

vltr commented Feb 22, 2018

Well, some of the options may be more flexible with this, adding even the possibility to pass arguments (like request) when calling for some attributes (like this comment), but some other options could get a little confusing.

What could be done in Configuration is to use a Metaclass or implement the __getattr__ method to check if the given key exists (as get_{} or _{}), if it's callable or a coroutine and so on ...

@ahopkins
Copy link
Owner Author

ahopkins commented Feb 22, 2018 via email

@vltr
Copy link
Collaborator

vltr commented Feb 22, 2018

And what needs checking (even if this means going back to runtime checks, like secrets, keys) going hand-picked into __getattribute__ ...

@ahopkins
Copy link
Owner Author

That would a nice slick way to do it. Let's slate this for v1.1

@ahopkins ahopkins added this to the v1.1 milestone Mar 19, 2018
@ahopkins ahopkins mentioned this issue Mar 19, 2018
13 tasks
@ahopkins
Copy link
Owner Author

ahopkins commented Apr 3, 2018

Per a discussion between myself and @vltr, the current direction is to move this towards an implementation that would enable the following API:

from sanic_jwt import DynamicConfiguration

class MyConfiguration(DynamicConfiguration):
    async def get_access_token_name(self, request, user):
        return await user.get_unique_token_name()

Initialize(
    app,
    configuration_class=MyConfiguration)

Already in the current version exists the ability to set the setting at runtime:

from sanic_jwt import Configuration

class MyConfiguration(Configuration):
    def set_access_token_name(self):
        return 'jwt'

Initialize(
    app,
    configuration_class=MyConfiguration)

This methodology would NOT override that. Instead the idea is that you would set the settings that would be used as a fallback. But, if it needs to be dynamically calculated, it can happen at run time.

@vltr
Copy link
Collaborator

vltr commented Apr 3, 2018

I was thinking about the computing penalty we discussed and the only way to properly cache the result of those functions is if the arguments can be hashable ... If not, the cache won't work, so request would not be a good parameter, for example. But if a header or cookie are set as parameters, then (caching) could work.

I mean, I still don't know yet what parameters could be passed on to DynamicConfiguration methods, neither if we should stipulate them - or at least provide a set of possible (hashable) parameters ...

@vltr vltr self-assigned this Apr 5, 2018
vltr added a commit that referenced this issue Apr 9, 2018
@ahopkins
Copy link
Owner Author

ahopkins commented Apr 9, 2018

I have been playing with a different approach. Rather than having different Configuration classes, what if there is just a single one. And, instead the configuration items themselves are instances capable of being passed arguments and being aware if a getter class is set.

What do I mean by this?

request = 'fakerequestobject'


class ConfigItem:
   def __init__(self, default, inject_request=True):
       self._value = default
       self.inject_request = inject_request

   def __repr__(self):
       return self.value

   @property
   def value(self):
       if hasattr(self.config, self.get_name):
           args = []
           getter = getattr(self.config, self.get_name)

           if self.inject_request:
               args.append(request)

           return getter(*args)
       return self._value


class Configuration:
   def __new__(cls, *args, **kwargs):
       i = super().__new__(cls, *args, **kwargs)
       items = [(x, getattr(cls, x)) for x in dir(cls) if isinstance(getattr(cls, x), ConfigItem)]

       for name, item in items:
           name = name[1:]
           get_name = 'get_{}'.format(name)

           item.config = i
           item.name = name
           item.get_name = get_name
           setattr(i, name, item)

       return i

   _access_token_name = ConfigItem('access_token')
   _refresh_token_name = ConfigItem('refresh_token')


class MyCustomConfig(Configuration):
   def get_refresh_token_name(self, request):
       return 'myrefresh_' + request


print('with plain Configuration()')
config = Configuration()
print(config.access_token_name)
print(config.refresh_token_name)

print('------------')

print('with custom MyCustomConfig()')
config = MyCustomConfig()
print(config.access_token_name)
print(config.refresh_token_name)

Run this snippet.

Basically, the idea is that we maintain config.<item> (or, at least some variation of it. Its an instance now, so we can mess with it however we see fit). Then, if a dynamic value is wanted, all that the developer needs to do is subclass and create a get_<item> method.

How about passing kwargs? Well, we just define them at initialization of the Configuration class:

class Configuration:
    _access_token_name = ConfigItem('access_token')
    ...
    _refresh_token_name = ConfigItem('refresh_token')
    ...   
    _url_prefix = ConfigItem('/auth', inject_request=False)

The other main advantage that I see is that this means that when I (as a developer) am subclassing Configuration I have some choices:

  • completely override a config item

    class MyConfig(Configuration):
        access_token_name = 'mytoken'
    
  • firmly set the item

    class MyConfig(Configuration):
        def set_access_token(self):
            return 'mytoken'
    
  • or, dynamically retrieve on each request

    class MyConfig(Configuration):
        def get_access_token_name(self, request):
            return 'mytoken'
    

Thoughts?

@vltr
Copy link
Collaborator

vltr commented Apr 10, 2018

It does looks like a nice approach. The million dollar question still remains (in my head): how do we inject the request object inside this configuration, without caching it or leaking it to other requests being processed at the same time? I don't see it working by just calling config.<item>. And make a copy of the config for every request sounds like overkill (I'm just wondering the options). A contextmanager for this will also be hard to implement, specially with #70 in mind. Any ideas?

@ahopkins
Copy link
Owner Author

When the request comes in, it passes thru the decorators, which do have access to the request. Perhaps it is a contextmanager like approach.

The decorators have access to the Initialize instance, which has the Configuration instance, which are had by the ConfigItem instances, which can decide whether or more to pass the attributes. Maybe not the most elegant. But it should work.

And, I'll have to look at your proposed fix for #70, but I don't see this interfering. The bigger question I would have would be thinking of a way to cache the return so it is calculated only once per request. This perhaps could also be done with a contextmanager inside the decorator that executes the getters and caches the result on the config object.

@vltr
Copy link
Collaborator

vltr commented Apr 10, 2018

For #70 I didn't proposed anything yet since I'm trying to deal with this issue first 😄

As for decorators, you're refering to @protected and @scopes? I was thinking that these dynamic configurations would be sanic-jwt wide, not only valid for decorators ...

@ahopkins
Copy link
Owner Author

Right, they are, and they should be.

But, the whole point of a dynamic config setting is to so some request specific operation. Presumably, this would need the request object. The only way this enters sanic-jwt is thru the decorators. Otherwise, the package is completely blind to the request. Again, as it should be.

So, when a request comes in thru a @protected route, then there should be a calculation of the config settings (where applicable). I'll try and put my idea into practice and push some code.

@vltr
Copy link
Collaborator

vltr commented Apr 10, 2018

Allright, now I got the idea. I was thiking (and personally wanting) a more wide approach to these configurations, even inside core sanic-jwt, if applicable ... I'll have to think about it now 😕

vltr added a commit that referenced this issue Apr 12, 2018
@vltr vltr mentioned this issue Apr 17, 2018
14 tasks
@ahopkins
Copy link
Owner Author

Finally getting this one geared up for release :-D

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