[WIP][ADD] jsonify_stored

[len-foss]

Variable/function names are not particularly good (it's a WIP after all).

[simahawk]

@len-foss would you have time to move on here?

[len-foss]

Definitely, I'll get a test suite done by the end of the week.

[len-foss]

@simahawk I think it's in a decent state now, not just a PoC anymore.

Not sure the cron should exist on the mixin - it makes sense to specialize crons for interesting inheriting models. But it also makes sense to have it as a default so that you don't need to re-implement it for basic models. Besides I've found better to put it in a test module, this way it also provides a reference implementation for the pre-init hook.

[simahawk]

Not sure the cron should exist on the mixin - it makes sense to specialize crons for interesting inheriting models. But it also makes sense to have it as a default so that you don't need to re-implement it for basic models.

I'd keep it on the mixin. It makes perfectly sense.

[len-foss]

@lmignon @simahawk So. I worked quite a bit more on it. The thing is, this PR use the compute method to update a second field (todo is actually set by either the trigger, or the recompute). It works, but it uses the ORM in a nonstandard way (and I don't see any trick to avoid that -- it could be possible to cheat using two dates, one "recompute date" and one "need update date", but then that would require a custom search method to to emulate the 'todo' state, which would be equivalent to r.recompute <= r.update. So would be a tad more complex). I've ported it to 13.0: https://github.com/OCA/server-tools/compare/13.0...acsone:13.0-jsonifystored-len As shown in the third commit, it required a flush to pass the test as before, but it's the only change.

I don't know how bad of an idea it is, to be frank. So I've created an alternate version that is much simpler, at https://github.com/OCA/server-tools/compare/12.0...acsone:12.0-jsonifystoredsimple-len. The forward-port should be entirely trivial for this branch. The only downside is that it does not allow to batch recomputes as well. But that may be improved by improving queue_job.

Also, I don't want to miss the forest for the trees. Most of the code about the export modifications could be removed if we forbid to modify exports manually. It would mean that an export modification would require an instance restart, which is actually reasonable. In Practice, that really should not happen, especially for any big db.

[simahawk]

@lmignon @simahawk So. I worked quite a bit more on it.

1st of all: thanks for your efforts! :handshake:

The thing is, this PR use the compute method to update a second field (todo is actually set by either the trigger, or the recompute). It works, but it uses the ORM in a nonstandard way (and I don't see any trick to avoid that -- it could be possible to cheat using two dates, one "recompute date" and one "need update date", but then that would require a custom search method to to emulate the 'todo' state, which would be equivalent to r.recompute <= r.update. So would be a tad more complex).

Another option is: build an hash using required keys and compute only when the hash changes. In this way you won't have to recompute all data when something changes. The compute_data method will check if the hash has changed and recompute on the fly. This of course will be mitigated by a cron that will update them under the hood.

I've made something similar here to avoid computing images over and over: https://github.com/shopinvader/odoo-shopinvader/blob/13.0/shopinvader_image/models/shopinvader_image_mixin.py#L24

I think this is going to simplify a lot the machinery as you won't have to mess up w/ depends but only force hash computation when ir.exports records or resolvers change.

Also, according to https://github.com/OCA/server-tools/pull/1926#discussion_r568386034 you can simplify more and remove the commit hook as well.

I've ported it to 13.0: 13.0...acsone:13.0-jsonifystored-len As shown in the third commit, it required a flush to pass the test as before, but it's the only change.

Cool, thanks!

I don't know how bad of an idea it is, to be frank. So I've created an alternate version that is much simpler, at 12.0...acsone:12.0-jsonifystoredsimple-len.

The forward-port should be entirely trivial for this branch. The only downside is that it does not allow to batch recomputes as well. But that may be improved by improving queue_job.

what do you mean? You should create your own batches by size.

Also, I don't want to miss the forest for the trees. Most of the code about the export modifications could be removed if we forbid to modify exports manually. It would mean that an export modification would require an instance restart, which is actually reasonable. In Practice, that really should not happen, especially for any big db.

Hmm I'm :-1: for this: it shouldn't require a restart.

[len-foss]

Is there any problem you can't solve with one more layer of indirection? I realized how to solve the compute issue when reading your comment. So the current version only uses standard ORM features, and there isn't even any trick, really. (modifying the depends isn't really an issue, re-setuping the model is something that is already done in standard, in the mail module).

I think the alternative simple version can still be considered seriously, but there isn't any technicality favouring one over the other anymore.

[simahawk]

@len-foss @lmignon hello :) I'm getting back to this subject. Have you done any progress here? I'd like to move it fwd but on v14.

[len-foss]

Well. Not really, I don't have the time. It works pretty well in production with the two timestamps system. There's one issue with dynamic trigger: if module B > A adds some new fields to the json export, then testing A would crash at the trigger setup. Because the new field can be on some model unrelated to the one we're working on, and that the order they are setup is 'random', there is no way around that. That might not be a deal breaker, but this is quite annoying (to test A, you make a fresh DB without B). Also there was no explicit spec on how to manage the asynchronous update, while all solutions have particular trade-offs; one way around that would be to actually test implementations with some realistic enough load, but that would require some time. All in all, it's quite finicky, has some definitive drawbacks that need to be made consciously, and has some problems. So I'm not sure it's such a good idea. Going forward, I'll get out of Odoo development as soon as I can, but I'll help if required before that.

[github-actions[bot]]

There hasn't been any activity on this pull request in the past 4 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 30 days. If you want this PR to never become stale, please ask a PSC member to apply the "no stale" label.