Infinite loop due to fields.depends adding the same field in a function field

albert · May 9, 2020, 10:30am

We hit a problem which it seems to me that it could be considered a bug, but I’m not 100% sure.

The problem was with our account_invoice_line_payment module.

We have a “company” field as a fields.Function that uses this on_change_with_company method:

NaN-tic/trytond-account_invoice_line_payment/blob/master/payment.py#L378




@fields.depends('line', '_parent_line.amount', 'amount',
    methods=['on_change_with_currency_digits'])
def on_change_with_difference(self, name=None):
    if not self.line or not self.amount:
        return Decimal(0)
    digits = self.on_change_with_currency_digits()
    amount = (self.line.amount + self.line.tax_amount) - self.amount
    return amount.quantize(Decimal(str(10 ** -digits)))


@fields.depends('group', '_parent_group.company', 'company')
def on_change_with_company(self, name=None):
    if self.group:
        return self.group.company.id


@classmethod
def search_group_field(cls, name, clause):
    return [('group.%s' % clause[0],) + tuple(clause[1:])]


@fields.depends('group', '_parent_group.currency')
def on_change_with_currency_digits(self, name=None):

As you can see we had the “company” field itself as a “depends” field. It is important to note that in this case we removed this dependency because we really don’t need it and that solved the problem. But having it there caused an infinite loop.

The client did this RPC call:

ERROR trytond.protocols.dispatcher <class 'trytond.pool.account.invoice.line.payment'>.read(*([30972], ['company', 'difference', 'difference_move', 'company.rec_name', 'difference_move.rec_name', 'rec_name', '_timestamp'], ....

But the request failed with an infinte loop where the looping part looks like this:

Traceback (most recent call last):
File "/trytond/trytond/model/modelstorage.py", line 1557, in __getattr__
read_data = self.read(list(ids), list(ffields.keys()))
File "/trytond/trytond/model/modelsql.py", line 836, in read
getter_results = field.get(ids, cls, field_list, values=result)
File "/trytond/trytond/model/fields/function.py", line 106, in get
return dict((name, call(name)) for name in names)
File "/trytond/trytond/model/fields/function.py", line 106, in <genexpr>
return dict((name, call(name)) for name in names)
File "/trytond/trytond/model/fields/function.py", line 101, in call
return dict(([r.id](http://r.id), method(r, name)) for r in records)
File "/trytond/trytond/model/fields/function.py", line 101, in <genexpr>
return dict(([r.id](http://r.id), method(r, name)) for r in records)
File "/trytond/trytond/model/fields/field.py", line 116, in wrapper
_set_value(self, field)
File "/trytond/trytond/model/fields/field.py", line 91, in _set_value
if not hasattr(record, field):
File "/trytond/trytond/model/fields/field.py", line 334, in __get__
return inst.__getattr__([self.name])

The problem is that in field.py:116 Tryton tries to set (and thus compute) the value for the ‘company’ field before calling on_change_with_company() because ‘company’ itself is in the depends. So there’s an infinite loop.

As I already stated, there’s no problem for us in this case but there’s a couple of observations I wanted to make:

I was surprised to see that @fields.depends() is trying to run _set_value() here. Unless I miss something, it looks like a waste of CPU cycles when on_change_with is being called from the ORM. Of course, it is necessary when it’s being called from RPC. I don’t know if there’s something we could do to prevent it.
I can imagine some (maybe corner) case in which it would could be desireable to have the field itself in the list of depends fields. For example, if the field was a Function field with setter we may be interested in its value in on_change_with.

For example, the following would be a valid scenario that would fail:

my_field = fields.Function(fields.Char('My Field'), 'on_change_with_my_field',
        setter='set_my_field')

@fields.depends('other_field', 'my_field')
def on_change_with_my_field(self, name=None):
    if self.my_field:
        return self.my_field
    return self.other_field

What do you think? Am I missing something? Is this actually a bug?

ced · May 9, 2020, 11:35am

It does not look like a valid scenario. Something can not depend on its own value to define its own value.

albert · May 9, 2020, 2:24pm

You’re right. It makes sense only as an on_change_with but not as a getter method.