feat: cache queries in filters #124
Conversation
| expand_children=None, | ||
| klass=None, | ||
| filter_cache=None, | ||
| debug_indent="", |
There was a problem hiding this comment.
I think this is separator rather than indent. Have I misunderstood the intent?
There was a problem hiding this comment.
it's an indent, it just helps to make the logs easier to match up with what level of the response object the filters are working on. We can probably take the debug logging out now that the filters are working.
| filter_def, | ||
| expand_children=expand_dict, | ||
| klass=inst.__class__, | ||
| filter_cache={}, |
There was a problem hiding this comment.
How concerned should we be about the size of this object that we're passing? It could be massive, right?
There was a problem hiding this comment.
Maybe worthwhile doing something like this to track?
cache_obj = {}
filtered_val = _apply_filters_to_object(..., filter_cache=cache_obj, ...)
logger.info(... cache_obj.__sizeof__()...)
return filtered_val
There was a problem hiding this comment.
@smholloway it's pass-by-reference, so no concern about size when passing it in function calls. I don't even really think we need to track it; the total filter_cache object is going to be comparable in size to the overall response dict that we're returning anyway.
There was a problem hiding this comment.
Alright. Makes sense to me 👍
| expand_children=None, | ||
| klass=None, | ||
| filter_cache=None, | ||
| debug_indent="", |
There was a problem hiding this comment.
This is debug-specific code. A slight shift tracking depth would put a spin on it (then the prefix could just be " " * depth.
|
I have no problems with this change and would approve. I defer to Demian for a more thorough review. |
| ): | ||
| debug_indent += " " | ||
| is_cacheable = False | ||
| debug_log = getattr(settings, "DDA_FILTER_CACHE_DEBUG_LOG", False) |
There was a problem hiding this comment.
Why use an environment variable rather than rely on built-in log configuration? Instead of logger.info this could use logger.debug and then this module's log level could be set through config.
| expand_this, | ||
| expand_children, | ||
| filter_cache, | ||
| indent, |
There was a problem hiding this comment.
This isn't used anymore, is it? Should it be removed?
| @@ -107,22 +146,60 @@ def _get_filtered_field_value( | |||
| return None | |||
|
|
|||
|
|
|||
| def _make_filter_cache_key(expand_children, klass, pk): | |||
| return (str(expand_children), klass, str(pk)) | |||
There was a problem hiding this comment.
klass would end up being the string repr of the instance's class, right? Would f"{klass.__module__}.{klass.__name__}" be slightly preferable?
There was a problem hiding this comment.
klass is the actual type object, not a str.
expand_children has to be stringified because dicts are not hashable. pk has to be stringified because otherwise we get a mix of uuid and str that don't match like they should.
There was a problem hiding this comment.
Ah, I'm dumb. I was thinking it was being serialized for some reason.
pyproject.tomlpip installsucceeds with a clean virtualenv