Fix Entity.order and Entity.section updates during sync - #4374
Conversation
mathjazz
left a comment
mathjazz
left a comment
There was a problem hiding this comment.
Nice work.
The order fix works, but if I delete the reorder block only test_change_entities and test_fluent_group_comment_change fail — one shuffles strings, the other removes one. Neither covers the case in the issue title: a string added at the top, pushing existing ones down.
|
|
||
| class EntityFactory(DjangoModelFactory): | ||
| resource = SubFactory(ResourceFactory) | ||
| section = SubFactory(SectionFactory, resource=SelfAttribute("..resource")) |
There was a problem hiding this comment.
Nit: EntityFactory now always creates a Section, so it models something that doesn't occur in practice.
| + model_update(prev_ent, "string", next_ent.string) | ||
| + model_update(prev_ent, "comment", next_ent.comment) | ||
| + model_update(prev_ent, "meta", next_ent.meta) | ||
| + model_update(prev_ent, "section", next_ent.section) |
There was a problem hiding this comment.
model_update() reads prev_ent.section and lazily fetches the section once per entity, on every changed resource, even when nothing changed.
Please add select_related("section").
There was a problem hiding this comment.
Done, including a .defer() for most of the Section fields. The equality check only actually looks at the primary key. I'd've thought that the request would be optimized out, but apparently it isn't, even though it's not needed.
There was a problem hiding this comment.
Nice refinement. It's safe today, but any future prev_ent.section.comment reintroduces exactly the N+1 this commit removed. A short comment ("Only section PK is fetched, do not read its fields here") would be a cheap insurance.
…resh_from_db() Co-authored-by: Matjaž Horvat <matjaz.horvat@gmail.com>
2 participants
Fixes #2115
Prerequisite for #4212
We currently don't have data for the correct order & section-level comments, so those won't be fixes in all cases until the relevant resources are sync'd (at which point they'll be fixed using data from the source repo), so we may want to do a force-sync for all projects at some point.
The data migration took about 20 seconds on my machine.