[GrandComicsDatabase/gcd-django] Fix character-order revision commit handling (PR #736)
Adam Hernandez
Summary
- reconcile character-order revisions through
CharacterThroughOrderandCharacterThroughOrderRevision - batch character removals, order-code updates, and additions instead of querying once per character
- add regression coverage for additions, removals, reordered codes, and bounded query count
Root cause
The commit path treated StoryCharacter and StoryCharacterRevision as if they contained order_code. That value belongs to the explicit through rows, so existing code could raise AttributeError, save the wrong model, and fail to persist order changes.
Impact
Committing a character-order revision now applies the revision through rows to the display through rows, keyed by the persistent story-character appearance. The reconciliation uses two reads and at most one batched delete, update, and insert.
Validation
pytest -q apps/oi/tests/db/test_character_order_revision.py --reuse-db— 4 passedpytest -q apps/oi/tests --reuse-db— 257 passed, 9 existing warningspytest -q --reuse-db— 452 passed, 9 existing warnings- scoped
flake8, Python compilation, andgit diff --check
Scope
The unsaved-form-instance concern from the original issue is not changed, following Jochen's clarification that there are no unsaved instances on this path. This PR contains no schema or migration changes.
Closes #730
You can view, comment on, or merge this pull request online at:
https://github.com/GrandComicsDatabase/gcd-django/pull/736
Commit Summary
- 34599f4 fix(oi): reconcile character order revisions
File Changes
(2 files)
Patch Links:
- https://github.com/GrandComicsDatabase/gcd-django/pull/736.patch
- https://github.com/GrandComicsDatabase/gcd-django/pull/736.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
gemini-code-assist[bot]
@gemini-code-assist[bot] commented on this pull request.
Code Review
This pull request optimizes the _post_save_object method in apps/oi/models.py by refactoring the character order reconciliation process to use bulk database operations (bulk_update, bulk_create, and bulk delete), significantly reducing the number of queries. It also introduces a comprehensive test suite in apps/oi/tests/db/test_character_order_revision.py to verify these changes and assert query limits. The review feedback suggests a further optimization to use self.character_order_id instead of self.character_order to avoid unnecessary database lookups for the foreign key relation.
> + current_orders = {
+ current_order.story_character_id: current_order
+ for current_order in CharacterThroughOrder.objects.filter(
+ order=self.character_order)
+ }
+ revision_orders = CharacterThroughOrderRevision.objects.filter(
+ order=self).select_related('story_character')
+ desired_orders = {}
+ for revision_order in revision_orders:
+ story_character_id = \
+ revision_order.story_character.story_character_id
+ if story_character_id is None:
+ raise IntegrityError(
+ 'Character order revision contains an uncommitted '
+ 'story character revision.')
+ desired_orders[story_character_id] = revision_order.order_code
+
+ removed_ids = current_orders.keys() - desired_orders.keys()
+ if removed_ids:
+ CharacterThroughOrder.objects.filter(
+ order=self.character_order,
+ story_character_id__in=removed_ids).delete()
+
+ changed_orders = []
+ new_orders = []
+ for story_character_id, order_code in desired_orders.items():
+ if story_character_id in current_orders:
+ current_order = current_orders[story_character_id]
+ if current_order.order_code != order_code:
+ current_order.order_code = order_code
+ changed_orders.append(current_order)
else:
- character.order_code = character_revisions.get(
- character__id=character.id,
- universe=character.universe).order_code
- character.save()
- for character_revision in character_revisions:
- if not characters.filter(
- id=character_revision.character.id,
- universe=character_revision.universe).exists():
- order_code = character_revision\
- .characterthroughorderrevision_set.get(order=self).order_code
- self.character_order.characters.add(
- character_revision.story_character,
- through_defaults={'order_code': order_code})
+ new_orders.append(CharacterThroughOrder(
+ order=self.character_order,
+ story_character_id=story_character_id,
+ order_code=order_code))
+
+ if changed_orders:
+ CharacterThroughOrder.objects.bulk_update(
+ changed_orders, ['order_code'])
+ if new_orders:
+ CharacterThroughOrder.objects.bulk_create(new_orders)
Using self.character_order_id (or order_id=self.character_order_id) instead of self.character_order avoids an unnecessary database query to fetch the related CharacterOrder object when editing an existing revision, as Django lazy-loads foreign key relations.
current_orders = { current_order.story_character_id: current_order for current_order in CharacterThroughOrder.objects.filter( order_id=self.character_order_id) } revision_orders = CharacterThroughOrderRevision.objects.filter( order=self).select_related('story_character') desired_orders = {} for revision_order in revision_orders: story_character_id = \ revision_order.story_character.story_character_id if story_character_id is None: raise IntegrityError( 'Character order revision contains an uncommitted ' 'story character revision.') desired_orders[story_character_id] = revision_order.order_code removed_ids = current_orders.keys() - desired_orders.keys() if removed_ids: CharacterThroughOrder.objects.filter( order_id=self.character_order_id, story_character_id__in=removed_ids).delete() changed_orders = [] new_orders = [] for story_character_id, order_code in desired_orders.items(): if story_character_id in current_orders: current_order = current_orders[story_character_id] if current_order.order_code != order_code: current_order.order_code = order_code changed_orders.append(current_order) else: new_orders.append(CharacterThroughOrder( order_id=self.character_order_id, story_character_id=story_character_id, order_code=order_code)) if changed_orders: CharacterThroughOrder.objects.bulk_update( changed_orders, ['order_code']) if new_orders: CharacterThroughOrder.objects.bulk_create(new_orders)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco pushed 1 commit.
- f23c3ef perf(oi): reconcile character orders by id
—
View it on GitHub or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
Adam Hernandez
@DeusExTaco commented on this pull request.
Addressed in f23c3ef. The lookup, bulk delete, and new through-row construction now use character_order_id. The query-count test also reloads CharacterOrderRevision before reconciliation so the five-query bound is verified with an uncached relation. The focused tests pass (4), and the full suite passes (452, with 9 existing warnings).
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.
JochenGCD
—
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications, keep track of coding agent tasks and review pull requests on the go with GitHub Mobile for iOS and Android. Download it today!
You are receiving this because you are subscribed to this thread.