Shine does not work after issue8234

Support Developer
1

In Issue 8234: Allow to cache on_change calls - Tryton issue tracker changes were done to both sao and tryton that make them directly call “on_change_with_fieldname” methods if there’s only one on_change_with call to be done.

Here’s the code for tryton:

https://hg.tryton.org/tryton/rev/cb0a2d8c4a99#l2.36

Shine does not create the fields (nor on_change_with_xxx fields) but instead overrides ModelView’s “on_change_with()” method, so after issue8234 users get a FORBIDDEN error.

What was the reason to call on_change_with_fieldname() instead of on_change_with()? Could that specific change be reverted? Is there some other alternative that allows us to create fields and on_change_with() calls on dynamically?

2

Being able to cache the result.

I do not think because it is a very good performance improvement. We still need to activate it on more places.
More over, it was always allowed for any client to call on_change fields directly (it was the initial implementation). We just switched to a grouped call for performance (it reduce round-trip) but we always added on_change* methods to RPC.

If you create the fields dynamically, you should probably be able to create also the methods.

3

In fact, we do not create the models nor the fields dynamically, we simply override fields_get(), read() and the rest of the methods returning the information depending on the context.

We took this approach in shine because creating models dynamically was the way we did it with babi and it has several issues. Some of the ones that come to mind:

  • A patch is required to trytond to instantiate models when using multiple processes
  • tryton expects to be able to browse all the models and it can make it slow (for example, ir.attachment reads all models for its resource Reference field)
  • Although we remove the model from the pool we have no mechanism to remove it from the pools of other processes

On the other hand, we didn’t have to re-implement ORM methods (read/write/delete) so it was somewhat easier.

What approach do you think would be the best / most future proof?

4

I think the current aproach is better because it is like using a non sql based model storage which I think is something we should support.

BTW, did you tryed using a table_query for browsing the data? This will reduce the number of functions to implement.

For the on_change issue I imagine you can override the __getattr__ method of your model to call a custom function when some on_change_with function is called.

5

I don’t remember the details right now but I’m not sure that will work given that we create fields that are not stored but that use the formula’s computation.

Unfortunately that will not be enough. See:

https://hg.tryton.org/trytond/file/tip/trytond/protocols/dispatcher.py#l144

Also, we recently we had to add a patch to trytond to allow dynamic fields as required by sale_pos_template_quantities module.

My feeling is that there not many modules that implement dynamic fields and models. We should probably decide if they’re supposed to work and have a pattern (and probably a test) to define how they should be implemented.

For example, what is the right way to discover existing fields in the model? There are places where cls._fields is used, that’s why we implemented an adapter in shine, but in other places a different strategy is used: for example the __init__ method our patch changes and __setup__ where dir() is used: trytond: 10426538d63f trytond/model/model.py

6

I like it. Sounds good to me.

7

For me, field must be an attribute of the Model. _fields is a cache to easy loops over all fields.