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

Add middlewares management #72

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Add middlewares management #72

wants to merge 7 commits into from

Conversation

twidi
Copy link
Collaborator

@twidi twidi commented Jan 16, 2013

The goal of this middleware system is to add an entry point to help enhance the limpyd behavior without touching its core.

A middleware is a class which surround a redis call when a command n a field is called.

The pre_command method of each middleware, is called before the redis call, then the post_command method of each middleware in reverse order.

If a pre_command method returns something (not None), the redis call won't be done and the result value will be used instead (and other pre_command methods won't be called too).

A post_command method must return a Result object, which contains the value got from redis (or from a pre_command method).

A pre_command method accepts two arguments:

  • command, a Command object, with three fields (name (of the command) and args and kwargs to pass to the command)
  • context, a dict initialized with a sender entry, which is the object on which the original command apply (a field or a model)

The command object can be modified if wanted, before used to call redis.
The context dict can also be modified, to add properties useful to the middleware that it would need in the post_command for example. Note that each called middleware share the same context dictionary.

A post_command method accepts three arguments:

  • command, the Command object passed through the pre_command methods
  • result, a Result object with a single field, value, which hold the result of the redis call
  • context, the same dictionary passed through the pre_command methods.

The result can be modified, and it's value returned from the last post_command metho will be used as a result to the call. (The command and context objects can still be modified, but it is not really useful)

To define a new middleware, simply inherit from BaseMiddleware in limpyd.middlewares and define a pre_command and/or a post_command (they are both optional, only the existing methods will be used).

To use middlewares, you must declare it when creating your database:

database = RedisDatabase(
    middlewares=[
            AMiddleware(with_some=params),
            AnotherMiddleware(),
        ], 
    **my_connection_settings
)

An example (but usable) middleware is provided in limpyd.middlewares, to log commands, their results (optional) and the duration (optional too): LoggingMiddleware. It can log stuff like:

[#1] Command(name='set', args=(u'mynamespace:mymodel:1:myfield', 'foo'), kwargs={})"
[#1, in 321µs] Result(value=1)

twidi added 7 commits January 9, 2013 09:07
namedtuples are immutables, but we must allow edition of commands and
results inside the middleware, so we use real objects, but wuth __slots__
instead of __dict__, which is faster and much memory efficient
Conflicts:
	limpyd/fields.py
	run_tests.py
@twidi
Copy link
Collaborator Author

twidi commented Jan 16, 2013

Missing docs, but what about using my loooong PR description ?

@yohanboniface
Copy link
Collaborator

Yes, for the doc :)
Just add a field in doc/ and point to it in index.rst.

@@ -230,12 +232,18 @@ def __init__(self, *args, **kwargs):
self._creation_order = RedisField._creation_order
RedisField._creation_order += 1

def proxy_get(self):
def proxy_get(self, _direct=False):
Copy link
Collaborator

Choose a reason for hiding this comment

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

As usual, I don't feel comfortable when changing the method behaviour from a parameter. IMHO, here we need two methods.

Copy link
Collaborator

Choose a reason for hiding this comment

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

BTW, I don't get the need of this direct call. Is there a constraint not to call a middleware from inside a command call ? Or is it specific to index calls? Need your lights here ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We have to decide: ALL redis commands are passed to the middleware, or ONLY ones issued by the user.
Because when we manage indexes, we have mainly direct calls to redis (sadd...), but also some proxy_get, and both should be managed the same way for consistency

NB1: btw i knew you wouldn't like this update, it was here to initiate the conversation ;)
NB2: i'll merge master into this branch later, when done

Copy link
Collaborator

Choose a reason for hiding this comment

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

Humm, not easy.
Except for performance reasons, I think we need a very generic hook, so at this point my preference goes to calling middlewares for every calls from Limpyd.
About performance, I don't know what can be the cost of calling the middlewares hook for every call.


for middleware in self.prepared_middlewares['pre_command']:
result = middleware.pre_command(command, context)
if result:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

if result is not None ?

# _models keep an entry for each defined model on this database
self._models = dict()

if middlewares is not None:
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe the check is not None is not necessary

@@ -126,3 +172,64 @@ def has_scripting(self):
except:
self._has_scripting = False
return self._has_scripting

@property
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

could use @cached_property

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants