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

Support disabling compute on create #157

Open
stevelacey opened this issue Jan 23, 2025 · 1 comment
Open

Support disabling compute on create #157

stevelacey opened this issue Jan 23, 2025 · 1 comment

Comments

@stevelacey
Copy link

stevelacey commented Jan 23, 2025

Most of my computed fields are cached counts or booleans, usually defaulting to 0 or False. These fields often depend on related objects, so if you try something like self.thing.count() before the model is saved, it crashes because the relation doesn’t exist yet.

To handle this, I’ve been adding if instance._state.adding checks in my compute functions. While this works, it’s repetitive and clutters the code.

I created a wrapper that skips computing the value for new records unless explicitly requested. This simplifies compute functions, letting me avoid manual checks for new instances. For example, instead of writing:

lambda user: user.followers.count() if not user._state.adding else 0

I can just write:

lambda user: user.followers.count()

The default value is already handled by the field itself, e.g., models.PositiveIntegerField(default=0).

Here’s the wrapper:

from functools import wraps
import computedfields
from django.db import models

def computedfield_factory(field, compute, immediate=False, **kwargs):
    """Switch compute off by default for new records unless immediate is passed."""

    @wraps(compute)
    def func(instance):
        if immediate or not instance._state.adding:
            return compute(instance)
        return field.get_default()

    return computedfields.models.ComputedField(field, func, **kwargs)

Rethinking the default

Initially, I made immediate=False the default, as most of my computed fields don’t need immediate computation (only 2 out of 7 do). However, after reflecting on how technologies like Vue behave, I realized that watch functions (not computed properties) are the ones that defer execution by default. Computed properties, on the other hand, run immediately when accessed.

With that in mind, it might make more sense for the default behavior to be immediate=True—so that computed fields run immediately, consistent with existing behavior and developer expectations. You’d then opt out of immediate execution when necessary, making it an explicit choice rather than the default.

Next steps

I’m happy to open a pull request if there’s interest. I could also make the default for immediate configurable as a project-wide setting, allowing for compatibility with existing behavior while letting teams customize it for their needs.

@jerch
Copy link
Collaborator

jerch commented Jan 23, 2025

Yupp, this has been the case forever for m2m relations. It used to work for fk backrelations until django unified the "error behavioral pattern" for relations on newly created instances around v4. (Also see the 2nd note in the docs here: https://django-computedfields.readthedocs.io/en/latest/manual.html#automatic-updates)

In general I like the idea to provide a fallback value for those cases and using default looks fine to me.

But before proceeding, I think there are a few catches we should try to address:

  • Awareness of this create edge case. Although in a rather unfriendly exception way, the error currently kinda reminds ppl to explicitly handle it in their compute function. Therefore a fallback handling needs a strong emphasis in the docs and a more friendly (now less disruptive) reminder, that ppl should think twice about the correct defaults.
  • Real cause behind the scenes: At this point I wonder if the real cause should be addressed by computedfields. The real cause is the fact, that the instance has no DB repr, so we have no real db relations yet to operate on. A possible but quite involved solution would be to trigger a second save in cases, fields had to use the default fallback. This should be done before the cf resolver takes place to avoid nonsense cascading over dependency updates. This is tricky to get done right and I see it more like a nice-to-have.
  • About the default of such a setting: I suggest to keep the current error-out behavior as default, in a sense of least changing API surprise. Then with the cf local argument set to True ppl can opt-in the default fallback behavior. Edit Yes a project-wide setting to flip the default behavior sounds good to me as well (sorry overread that at first...)
  • In your other code snippet here Intermittent RelatedObjectDoesNotExist Error in Computed Fields #156 (comment) you used a try...except block to delegate to the default fallback behavior. I think thats not a good idea, as the exception handler might wrongly catch too much masking other errors.

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

No branches or pull requests

2 participants