django-computedfields icon indicating copy to clipboard operation
django-computedfields copied to clipboard

Intermittent `RelatedObjectDoesNotExist` Error in Computed Fields

Open hayder-mustafa-aswar opened this issue 11 months ago • 10 comments

Issue Overview

I am encountering an intermittent RelatedObjectDoesNotExist error inside one of my computed functions that depends on another computed field. The error occurs sporadically and can be temporarily resolved by restarting the server.

Here is an example of the problematic code:

price_total = ComputedField(
    MonetaryField(
        verbose_name="Total",
        editable=False,
        blank=True,
    ),
    depends=[
        ("self", ["currency", "quantity", "discount", "price_unit"]),
        ("taxes", []),
    ],
    compute=lambda self: self._compute_price_total(),
)

def _compute_price_total(self):
    if self.display_type != AccountMoveLineDisplayType.PRODUCT:
        return 0.0
    line_discount_price_unit = self.price_unit * (1 - (self.discount / 100.0))

    if self.taxes:
        taxes_res = self.taxes.compute_all(
            line_discount_price_unit,
            quantity=self.quantity,
            currency=self.currency,
            product=self.product,
            contact=self.contact,
            is_refund=self.is_refund,
        )
        return taxes_res["total_included"]
    else:
        return self.quantity * line_discount_price_unit

Error Details

The error looks like this:

RelatedObjectDoesNotExist: AccountMoveLine has no currency.

Here is the definition of the currency computed field that seems to cause the issue:

currency = ComputedField(
    models.ForeignKey(
        to="base.Currency",
        verbose_name="Currency",
        related_name="+",
        related_query_name="+",
        on_delete=models.PROTECT,
    ),
    depends=[("self", ["display_type", "company"]), ("move", ["currency"])],
    compute=lambda self: self._compute_currency(),
)

def _compute_currency(self):
    if self.move.is_invoice(include_receipts=True):
        return self.move.currency.id
    return self.currency.id or self.company.currency.id

Observations This issue only happens sometimes and can be resolved by stopping and restarting the server. Debugging indicates that the cf_mro (computed fields method resolution order) appears incorrect at times. Below is the relevant portion of the code for debugging:

def update_computedfields(
    self,
    instance: Model,
    update_fields: Optional[Iterable[str]] = None
) -> Optional[Iterable[str]]:
    """
    Update values of local computed fields of `instance`.
    """
    model = type(instance)
    if not self.has_computedfields(model):
        return update_fields
    cf_mro = self.get_local_mro(model, update_fields)
    if update_fields:
        update_fields = set(update_fields)
        update_fields.update(set(cf_mro))
    print(f"cf_mro {model} - {cf_mro}")
    for fieldname in cf_mro:
        setattr(instance, fieldname, self._compute(instance, model, fieldname))
    if update_fields:
        return update_fields
    return None
cf_mro <class 'AccountMoveLine'> - [........., 'price_total', ..........,  'currency_id', ]

Key Questions

  • Why does this error occur intermittently?
  • Why does restarting the server temporarily fix the issue?
  • Could the incorrect cf_mro order be the root cause?
  • What causes the currency field to become unavailable or not yet computed when price_total tries to use it?

Expected Behavior

  • The currency field should be computed and available before the price_total field is computed.
  • The computed fields dependency resolution (cf_mro) should always order dependent fields correctly.

Steps to Reproduce

  • Use the code provided above for the price_total and currency fields.
  • Trigger the computation of price_total in a context where its dependencies might not be resolved yet.

Additional Notes

The issue seems related to how the computed fields dependencies are resolved (cf_mro). Any insight into why cf_mro is inconsistent or incorrect would be greatly appreciated.

hayder-mustafa-aswar avatar Dec 16 '24 22:12 hayder-mustafa-aswar

At a first glance - yes this looks like an issue with the mro. Can you re-check, whether for a successful run the mro looks the same?

To further debug things - can you show, what the admin gives for that model under "Computedfields" >> "Computed Fields Models" >> your_model including its ModelGraph? (You can switch on the admin helper views in your settings.py by defining COMPUTEDFIELDS_ADMIN = True, for the graph you need graphviz to be installed).

jerch avatar Dec 16 '24 23:12 jerch

In a successful run, the MRO is different — the currency_id appears before price_total, preventing the issue from occurring.

I noticed that the MRO displayed in the admin panel changes every time I restart the server. This seems odd to me because, as far as I understand, the MRO should only change if there's a modification to the database model or the class hierarchy. Could you confirm if my understanding is correct, or is there another reason why the MRO would behave this way?

hayder-mustafa-aswar avatar Dec 17 '24 14:12 hayder-mustafa-aswar

Eww thats a serious bug - yepp, the mro must be stable for a certain dependency configuration, it may never flip positions.

jerch avatar Dec 17 '24 15:12 jerch

Yes, this is a critical bug, and I hope it can be resolved as soon as possible since I have a project similar to Odoo that relies on it. I will also continue to investigate the issue further, and if I discover anything useful, I will share it with you promptly.

hayder-mustafa-aswar avatar Dec 17 '24 15:12 hayder-mustafa-aswar

We got a sentry alert about RelatedFieldDoesNotExist today, where it certainly exists. Tested restarting and indeed the MRO changed in the admin.

Before restart:

{
    "mro": [
        "cached_display_name",
        "follower_count",
        "following_count"
    ],
    "fields": {
        "following_count": [
            "following_count"
        ],
        "follower_count": [
            "follower_count"
        ],
        "cached_display_name": [
            "cached_display_name"
        ],
        "user": [
            "cached_display_name",
            "follower_count",
            "following_count"
        ]
    }
}

After restart:

{
    "mro": [
        "follower_count",
        "following_count",
        "cached_display_name"
    ],
    "fields": {
        "cached_display_name": [
            "cached_display_name"
        ],
        "following_count": [
            "following_count"
        ],
        "follower_count": [
            "follower_count"
        ],
        "user": [
            "follower_count",
            "following_count",
            "cached_display_name"
        ]
    }
}

Seems like potentially enforcing alphabetical order could be a quick fix?

Image

wadewilliams avatar Jan 22 '25 19:01 wadewilliams

@wadewilliams Sorry I had no time yet to look at the issue and where the mixup happens. In theory this should not have happened, because of how it is coded:

  • initially declaration order
  • do the topsort with dicts (thus if fields are equal in ranking, the initial declaration order should be preserved)

By that there is no possibility to mix things up (thats at least what I thought lol). My guess currently goes to the way how django creates fk fields on models - could it be, that they are not stable in init time?

Edit: Alphabetical ordering might be a quickfix yes, but it would have to be done on the topological groups, not model globally.

jerch avatar Jan 22 '25 20:01 jerch

I shipped this patch for the time being that at least suppresses the error:

from functools import wraps
import computedfields
from django.core.exceptions import ObjectDoesNotExist

def computedfield_factory(field, compute, **kwargs):
    @wraps(compute)
    def func(instance):
        # Suppress intermittent RelatedObjectDoesNotExist error
        # https://github.com/netzkolchose/django-computedfields/issues/156
        try:
            return compute(instance)
        except ObjectDoesNotExist:
            return field.get_default()

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

ComputedField = computedfield_factory

Note values do end up wrong with this but it's better than erroring.

stevelacey avatar Jan 23 '25 07:01 stevelacey

@stevelacey Thx for the idea, for follow-up I gonna comment on #157 instead.

jerch avatar Jan 23 '25 09:01 jerch

@hayder-mustafa-aswar I tried to provoke the glitching MRO issue but was not able to find a faulty pattern. Can you give me a stripped down code example, that still shows the weird behavior?

Furthermore - in your snippets of your first post, isnt there a logical error hidden? Seems you try to access the field to be calculated by the compute function in the compute function? Here:

# field: self.currency with compute function self._compute_currency
currency = ComputedField(
    models.ForeignKey(
        to="base.Currency",
        verbose_name="Currency",
        related_name="+",
        related_query_name="+",
        on_delete=models.PROTECT,
    ),
    depends=[("self", ["display_type", "company"]), ("move", ["currency"])],
    compute=lambda self: self._compute_currency(),
)

def _compute_currency(self):
    if self.move.is_invoice(include_receipts=True):
        return self.move.currency.id
    # here: access to self.currency,
    # this def. not gonna work, if self.currency has no prior value!
    return self.currency.id or self.company.currency.id

jerch avatar May 07 '25 20:05 jerch

@wadewilliams Thx for the diagrams and MRO outputs. From the looks of the diagram the changing MRO here is still valid, as the fields have no sub dependencies on each other, means it should not make any difference, in which order "follower_count", "following_count", "cached_display_name" are calculated. (BTW these MRO flippings can happen from the set() normalization of computed fields in the bootstrapping phase, so my depiction above as strictly in declaring order was not correct.)

Here is an example from the exampleapp, where arbitrary flipping is not allowed anymore (models.SelfRef):

Image

Here it is important, that c5 gets always calculated last, while the order of [c1, c8, c6] doesnt matter, but must be calculated before any of the other cX. So a valid MRO might look like this (DFS as calculated):

"mro": ["c1", "c2", "c3", "c4", "c8", "c7", "c6", "c5"]

or like this (manually crafted BFS-like):

"mro": ["c1", "c8", "c6", "c3", "c7", "c4", "c2", "c5"]

Can you share more details about your RelatedFieldDoesNotExist and when it happens? Is that really related to the MRO fields flipping? If so - could it be, that there is a dependency missing in depends?

jerch avatar May 07 '25 21:05 jerch

@hayder-mustafa-aswar I tried to provoke the glitching MRO issue but was not able to find a faulty pattern. Can you give me a stripped down code example, that still shows the weird behavior?

Furthermore - in your snippets of your first post, isnt there a logical error hidden? Seems you try to access the field to be calculated by the compute function in the compute function? Here ...

@jerch Yes, the _compute_currency() snippet has a real logical bug (it tries to read self.currency while computing self.currency), but it's not related to main bug, and i'm sorry but i can give an example of the code, it's a lot of code with many tables and depends

hayder-mustafa-aswar avatar May 31 '25 21:05 hayder-mustafa-aswar

@hayder-mustafa-aswar I cannot help you w'o a small repro, sorry.

jerch avatar May 31 '25 22:05 jerch

@ all

Gonna close this bug, as the initial issue cannot be repro'ed and the MRO discussion did not reveal anything buggy. Since there were different inputs from different ppl - please open a new ticket, if you still experience your concrete issue.

jerch avatar Jul 07 '25 15:07 jerch