django-computedfields
django-computedfields copied to clipboard
netzkolchose
Add support for Django 2.1
Pull Request Test Coverage Report for Build 367
- 5 of 7 (71.43%) changed or added relevant lines in 1 file are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage decreased (-0.3%) to 93.501%
| Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
|---|---|---|---|
| computedfields/resolver.py | 5 | 7 | 71.43% |
| <!-- | Total: | 5 | 7 |
| Totals | |
|---|---|
| Change from base Build 354: | -0.3% |
| Covered Lines: | 964 |
| Relevant Lines: | 993 |
💛 - Coveralls
Is there a reason why you want Django 2.1 support back? Note that it is already 7 months overdue.
Maybe we should discuss, whether there should be an option to disable bulk_update internally. I introduced it mainly as performance optimization, which gives ~30% speedup for updates of dummy fields on larger querysets. The raw perf numbers were like:
- ~ -3% for a single instance (yes, it is actually worse for that, but the change is negligible)
- ~ 10% for querysets up to 10 records
- ~ 20% for querysets up to 50 records
- ~ 35 % for querysets >100 records
Furthermore I had to introduce the COMPUTEDFIELDS_BATCHSIZE, since for querysets with 1000+ entries the perf decreased back to ~30% gain (stable in 10000+), prolly due to expensive string creation for the SQL command on python side (did not investigate).
@jerch in fact your suggestion of making this an option that can be enabled even on newer Django versions makes more sense, as some uses of DCF might rely on the models save() method being called to trigger some processing depending on the computed fields value. I'll make a change and introduce an option
@mobiware
... as some uses of DCF might rely on the models save() method being called to trigger some processing depending on the computed fields value.
Ah good point, this also would need some stronger emphasis for the bulk_update case in the docs in the sense, that save will not be called during dependent updates triggered from the resolver.
Edit:
Note that calling save from bulk_updater will update cfs twice beforehand, if the method was decorated with @precomputed. To avoid that, the original function would have to be preserved somehow in the decoration step and called instead.
@mobiware Thinking more about the save invocation, I am not sure yet, whether it should be done at all. Note that field updates done by the resolver are quite special and may not fullfill the expectations of a typical save overwrite. They always deploy the update_fields argument, which most people are not even aware of. Thus they might miss the fact, that their overloaded code doesn't do anything on db level, because their changed field is not added to update_fields.
But I am not sure, what the exact expectations of save overloads are, and when ppl would expect them to run.
Going two steps back and looking at Django docs and why there is an ability to overload save at all - it is mainly to give ppl a hook into late instance alterations, before stuff lands in the db. All on instance level, while bulk actions do not run the overloads.
Carried over to computed fields it is again the difference between using bulk_update as a bulk action vs. instance save calls. To not introduce to much API ambiguity with ifs and elses here and there (we somewhat already have that with save and @precomputed), we should try to stick to those paradigms in the sake of a comprehensible API.
Plz dont get me wrong, I am not against doing this, but I am not yet convinced that calling save from bulk_updater is the right solution. It might need a bit more thinking about useful API shaping (do we need a custom save_computed(...) method to be overloaded?)
@jerch To give you perhaps a bit more context, we have an indexing system that indexes model object to speed up searches and we want to allow searches also on related fields.
Imagine for instance a User model with a ForeignKey to Company, we want to be able to search by company in the User table, so we need to include a computed field company_name depending on Company.name into the User index.
In fact index refresh takes place in a post_save signal handler, not in a save() overload, sorry if my previous explanations weren't clear about that part, but it still means we need the save() method called on dependent models (for instance in the above case we want User.save() to be called when a Company changes its name).
We could have indexing take place as a background task on a regular basis but we wanted the index to be updated in real time whenever something changes, so we thought we could use DCF dependency resolving capabilities to good use to solve this problem.
@mobiware For that particular case - would it be easier for you with a custom signal, that indicates, that some cfs were just updated, maybe with the affected models/records in arguments?
While thinking about it - there is another problem from directly invoking save (or its signals):
The db will be in de-sync state in between, while the update cascade is running. Technically it is only safe to rely on computed fields, after the initial update_dependent call has finished (not for its recursive calls). With a custom signal we could work around that, thus you could safely call your custom logic afterwards.
@mobiware If this here is not pressing for you, I suggest we try to get the multi table thingy working first. Then we can dig into this here. My current gut feeling regarding this here is somewhat along custom signals for the inter-model updates and maybe some way to overload update_computedfields for local ones.
@mobiware Would like to hear your ideas, how such a signal could look like. Also did some thinking/investigation on securing the sync state (related to the question, when it is safe to work with CF values) and opened #55.
@mobiware Plz see #56 to discuss the layout of a custom signal and/or method hooks.