feat: cache functions and fake relations in filters #127

lukka5 · 2023-05-08T20:18:15Z

Update CHANGELOG.md
Update README.md (as needed)
New dependencies were added to pyproject.toml
pip install succeeds with a clean virtualenv
There are new or modified tests that cover changes
Test coverage is maintained or expanded

salesforce-cla · 2023-05-08T20:18:19Z

Thanks for the contribution! It looks like @lukka5 is an internal user so signing the CLA is not required. However, we need to confirm this.

lukka5 · 2023-05-08T20:22:28Z

django_declarative_apis/machinery/filtering.py

@@ -280,4 +311,5 @@ def apply_filters_to_object(inst, filter_def, expand_header=""):
        expand_children=expand_dict,
        klass=inst.__class__,
        filter_cache={},
+        model_cache={},


decided to add a new cache instead of reusing filter_cache since they key/values types are completely different from filter cache.

Filter cache using is storing

key: (expandables, class, pk)

value: defaultdict(...) with filtered/formatted results

Model cache:

key: (class, pk)

value: model instances

lukka5 · 2023-05-08T20:26:37Z

django_declarative_apis/machinery/filtering.py

+        logger.debug("ev=model_cache, status=miss, key=%s", cache_key)
+        related_instance = getattr(inst, field_name)
+        model_cache[cache_key] = related_instance
+    setattr(inst, field_name, related_instance)


idea here is to pre-populate the related instance so if the function decides to access it or use it (normally it does) we don't do another query to the db. In example:

FILTERS = { 'counter': lambda inst: inst.counter.count + 1 }

When that function filter is called counter has already been populated by a counter from cache or stored for later used by another filter function.

lukka5 · 2023-05-08T20:27:41Z

django_declarative_apis/machinery/filtering.py

@@ -67,6 +68,34 @@ def _get_unexpanded_field_value(inst, field_name, field_type):
        return {display_key: getattr(obj, display_key)}


+def _is_relation(field):
+    return field.is_relation or getattr(field, "is_fake_relation", False)


this adds support for fake relationships. In example CharFields acting as foreign keys.

rohansroy · 2023-05-30T22:23:30Z

django_declarative_apis/machinery/filtering.py

+    else:
+        logger.debug("ev=model_cache, status=miss, key=%s", cache_key)
+        related_instance = getattr(inst, field_name)
+        if not isinstance(related_instance, models.Model):


Seems very generic, can't we use the instance type of val_cls

good catch. yeah, I like it

rohansroy · 2023-05-30T22:24:13Z

django_declarative_apis/machinery/filtering.py

+    try:
+        field_meta = inst._meta.get_field(field_name)
+    except (AttributeError, FieldDoesNotExist):
+        return


I'm curious why we're returning None here, shouldn't we bubble up the error? The user is trying to cache a field that doesn't exist

The problem is that field_name is a consumer defined string for specifying the output field name after filtering it. So it can have aj different name of the actual field. For example:

FILTERS = { 'my_increased_counter': lambda inst: inst.counter.count + 1 }

Here field_name would be my_increased_counter which doesn't exists in the model.

Right, but shouldn't we alert the user that the field doesn't exist somehow?

Code won't break if the field doesn't exists. It just won't get cached.

Or do mean alerting the user that using my_increased_counter as field name he won't get caching? In that case I'm not sure how much useful it is but yeah we could add some logging

I think I understand what you're saying now. This PR looks good, approved!

rohansroy · 2023-05-30T22:25:11Z

django_declarative_apis/machinery/filtering.py

+        return
+    if not _is_relation(field_meta) or _is_reverse_relation(field_meta):
+        return  # no `attname` in reverse relations
+    val_pk = getattr(inst, field_meta.attname)


can't we just wrap this in a try catch block and log the error or reraise the error?

that's a good point. I had run over the code again to be sure and the problem is that some fields have attname but are not relations so we cannot cache them

rohansroy · 2023-05-30T22:34:43Z

django_declarative_apis/machinery/filtering.py

@@ -144,18 +174,17 @@ def _make_filter_cache_key(expand_children, klass, pk):
    return (str(expand_children), klass, str(pk))


+def _make_model_cache_key(klass, pk):
+    return (klass, str(pk))


what will this key look like if we have to manually inspect it in cache? Can we have a more human readable cache key here, maybe something like {klass.__cache__.__name}:{str(pk)}

nevermind, i see it matches the style of the cache key of the other cache

lukka5 added 3 commits May 8, 2023 02:53

Add missing logger name

cdcc923

Fix not caching fake relationships

e36fa6f

Fix not caching related instances when using function filters

cb999ae

salesforce-cla bot added the cla:missing label May 8, 2023

lukka5 commented May 8, 2023

View reviewed changes

lukka5 added cla:signed cla:missing and removed cla:missing cla:signed labels May 8, 2023

Empty commit to re-trigger cla bot

9c4c3ac

salesforce-cla bot removed the cla:missing label May 8, 2023

Update CHANGELOG

308f982

lukka5 changed the title ~~feat: filter cache improvements~~ feat: cache function and fake relations in filters May 8, 2023

lukka5 changed the title ~~feat: cache function and fake relations in filters~~ feat: cache functions and fake relations in filters May 8, 2023

lukka5 added 2 commits May 9, 2023 11:36

Only cache models in model cache

686065c

Bump version

4011413

lukka5 force-pushed the feature/improve-filter-caching branch from 4bfb611 to 4011413 Compare May 9, 2023 14:36

rohansroy self-requested a review May 30, 2023 22:13

rohansroy reviewed May 30, 2023

View reviewed changes

rohansroy self-requested a review May 31, 2023 22:14

rohansroy previously approved these changes May 31, 2023

View reviewed changes

Address review comments

0855863

lukka5 dismissed rohansroy’s stale review via 0855863 June 2, 2023 21:11

lukka5 requested a review from rohansroy June 5, 2023 03:08

rohansroy approved these changes Jun 5, 2023

View reviewed changes

rohansroy self-requested a review June 5, 2023 15:41

rohansroy approved these changes Jun 5, 2023

View reviewed changes

lukka5 merged commit 3771323 into salesforce:main Jun 5, 2023

lukka5 deleted the feature/improve-filter-caching branch June 5, 2023 16:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: cache functions and fake relations in filters #127

feat: cache functions and fake relations in filters #127

lukka5 commented May 8, 2023 •

edited

Loading

salesforce-cla bot commented May 8, 2023

lukka5 May 8, 2023

lukka5 May 8, 2023

lukka5 May 8, 2023

rohansroy May 30, 2023

lukka5 Jun 2, 2023

lukka5 Jun 2, 2023

rohansroy May 30, 2023

lukka5 Jun 2, 2023

rohansroy Jun 5, 2023

lukka5 Jun 5, 2023

rohansroy Jun 5, 2023

rohansroy May 30, 2023

lukka5 Jun 2, 2023 •

edited

Loading

rohansroy Jun 5, 2023

rohansroy May 30, 2023

rohansroy May 31, 2023

feat: cache functions and fake relations in filters #127

feat: cache functions and fake relations in filters #127

Conversation

lukka5 commented May 8, 2023 • edited Loading

salesforce-cla bot commented May 8, 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

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

lukka5 Jun 2, 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

lukka5 commented May 8, 2023 •

edited

Loading

lukka5 Jun 2, 2023 •

edited

Loading