Automatically handle foreign key traversal evaluation in ValuesViewset - #15085
Automatically handle foreign key traversal evaluation in ValuesViewset#15085rtibbles wants to merge 3 commits into
Conversation
github-actions
Bot
added
DEV: backend
Build Artifacts
Smoke test screenshot |
🔵 Review postedLast updated: 2026-07-25 00:24 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15085 — the engine split into values_viewset/ is clean.
The auto-defer machinery documents its own limitations.
The test suite pins the tricky corners: relation-matrix fixtures, assertNumQueries budgets, same-model coalescing without cross-shape leaks.
CI passing. No UI files — Phases 3/3b N/A.
Findings, all inline:
- blocking — auto-fetches build unbounded
... __in=<all parent pks>lists, which this repo explicitly chunks around elsewhere. - suggestion — DEBUG output validation narrowed to required-only, exempting the
read_only+deferred_fields+consolidate()combination it was written to police. - suggestion — auto-deferred nested lists lost the ordering seam the join provided, with no replacement.
- suggestion — shared forward targets are aliased by identity into
consolidate()'s input. - suggestion — the docs' permission-scope claim holds for reverse fetches only.
- suggestion — every to-one nested serializer now costs a round trip, with no opt-out and no measurement against the joined path.
- nitpick —
serialize_querysetkwarg renamed without a shim. - nitpick —
?ordering=valid-field set changes for forward nested serializers. - nitpick — one non-strict
.get()among strict siblings.
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran a phased review pipeline over the pull request diff:
- Classified the diff to select review passes (core, frontend, backend) and whether manual QA was required
- Core review pass checked correctness, design, architecture, testing, completeness, and DRY/SRP/Rule-of-Three principles
- Specialized frontend/backend review passes applied framework-specific lenses where those files changed
- For UI changes: manual QA and an accessibility audit against a live dev server, when available
- Checked CI status and linked issue acceptance criteria
- Synthesized one review from those passes and chose the verdict from the findings, CI status, and QA evidence
| if self.is_many | ||
| else self.target_model._base_manager | ||
| ) | ||
| child_qs = manager.filter(**{self.link + "__in": pks}) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
blocking: Every parent pk from the level goes in as bind parameters, unchunked.
Same shape at engine.py:531, fetch.py:133, fetch.py:160.
The repo already treats this as a live constraint:
kolibri/core/mixins.py:158— "As a workaround to the SQLITE_MAX_VARIABLE_NUMBER … we pass in the list of ids … as an inline query statement".kolibri/plugins/coach/viewsets/unit_report.py:18—_IN_CHUNK_SIZE = 900.
Concrete regression:
FacilityUserSerializer.roles(auth/viewsets/facility_user.py:220) is amany=TruenestedModelSerializer, so it now auto-defers toRole.objects.filter(user__in=[…]).FacilityUserViewSetusesOptionalPageNumberPaginationwithpage_size = None, so a request withoutpage_sizereturns the whole facility.- On >999 users against a SQLite build at the old 999-variable limit (buster-era 3.27.x), that raises
OperationalError: too many SQL variables. - Before this branch the same request was one join with zero bind parameters for the user set.
The join → IN conversion is the point of the PR, so chunk in the fetch helpers (or route through the existing inline-literal trick), plus a test at >999 parents.
| child_items, child_pks, link_pairs, nested_forward_refs = engine.expand_level( | ||
| child_qs, self.child_path, method_context | ||
| ) | ||
| buckets = defaultdict(list) |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: ReverseAutoFetch.resolve never applies order_by, and there is no per-field hook to supply one.
Nested list order is therefore whatever the DB returns unless the child model declares Meta.ordering. Role has only unique_together, so FacilityUserSerializer.roles comes back unordered.
Neither escape hatch in AGENTS.md reaches an auto-deferred child: with the old join a dev could steer child order from the parent queryset (.order_by("id", "roles__kind")), and that ordering no longer touches the child query.
The new tests sort before asserting nested contents (test_api.py:1780, :1785), which is the tell.
Either honour the child model's Meta.ordering explicitly and document the requirement, or add a way to declare ordering per auto-deferred field.
| item = items[0] | ||
| item_keys = set(item.keys()) | ||
|
|
||
| missing = required_fields - item_keys |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: Narrowing missing from expected_fields - item_keys to required_fields - item_keys exempts the workflow the validator was written to police.
In DRF read_only=True implies required=False, and every field a viewset hands to consolidate() is read-only — LearnerClassroomSerializer.exams/lessons/courses (plugins/learn/viewsets/classroom.py:83), ClassroomSerializer.coaches (auth/viewsets/classroom.py:48).
So a consolidate() that forgets to populate a deferred field now passes DEBUG validation silently. test_validate_catches_consolidate_deleting_a_field still catches its case only because it deletes a required CharField.
The regression this narrowing fixes is specific to nested plain Serializer schemas, where DRF's SkipField genuinely omits optional keys.
Suggest keeping expected_fields - item_keys for the top-level engine-produced schema, and applying the required_fields relaxation only when recursing into nested schemas.
| One DB fetch for ``paths`` (same model, mergeable annotations), then | ||
| serialize each path's rows under its own plan and assign to its refs. | ||
|
|
||
| Parents sharing a forward target embed the *same* item dict by identity |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: "Harmless for the read-only JSON path" holds for the engine, but consolidate(items, queryset) receives these aliased dicts.
An in-place mutation of one parent's nested object silently lands on every other parent sharing that target pk.
Worth a line next to the consolidate() guidance in api_patterns.rst, where an implementer will actually read it.
| explicit_deferred: Tuple[str, ...], | ||
| ) -> Tuple[NestedCache, Optional[AutoFetch], Optional[str]]: | ||
| """ | ||
| Introspect one nested ``ModelSerializer`` field — always deferred, never |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: "Always deferred, never joined" also covers forward FK / OneToOne, which multiply nothing and joined for free.
- Affected in production:
LessonSerializer.classroom,plugins/learn/viewsets/{lesson,course}.py,auth/viewsets/facility.py:39,content/serializers.py:144and:270. - Batching plus tree-wide same-model merging usually makes this a good trade — N repeated join columns for one extra query.
- But rounds advance one forward hop at a time, so a to-one chain of depth 3 costs 3 extra round trips where it previously cost 0.
- Round trips are the expensive part on SQLite / Android / low-end hardware.
- The new benchmark compares auto-defer against explicit
consolidate(), not against the joined path this replaces.
A to-one measurement against develop, or an opt-in "join this to-one" escape, would make the tradeoff verifiable rather than asserted.
|
|
||
| Explicit ``deferred_fields`` is never auto-fetched. Use it when the fetch needs annotations, soft-delete filtering, or logic beyond a plain queryset. | ||
|
|
||
| Auto-fetched children are filtered by the parent pks the parent queryset already returned, inheriting its permission scope. Each auto-deferral decision emits a ``DEBUG``-level log on ``kolibri.core.utils.values_viewset.introspect``. |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
suggestion: True for reverse and scalar fetches, which key off parent pks.
Forward FK / OneToOne resolves as model._base_manager.filter(pk__in=all_pks) (engine.py:531) — keyed on the target's own pk, through the base manager, no filter backend.
That matches Django's descriptor semantics and the old LEFT JOIN, so it isn't a regression, but the sentence invites readers to assume KolibriAuthPermissionsFilter-equivalent scoping the forward path doesn't provide. Worth splitting the claim by direction.
| self._engine.validate_output(result) | ||
| return result | ||
|
|
||
| def serialize_queryset( |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: serializer_path= → path= on a documented consolidate() seam.
Both in-tree callers pass positionally, so nothing breaks here. Out-of-tree plugins calling serialize_queryset(qs, serializer_path="roles") get a TypeError. Cheap to keep an alias for a release.
| model_fields = {f.name for f in queryset.model._meta.get_fields()} | ||
| # Loop through every value in the view's values tuple | ||
| for field in view._values: | ||
| for field in view._engine.values: |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: For a viewset with a forward nested serializer, values used to hold the flattened child columns (collection__id, …) and now holds only the FK column.
So ?ordering=collection__id becomes invalid and ?ordering=collection valid, and remove_invalid_fields drops unknown terms silently — a stale client falls back to default ordering with no error.
No in-repo frontend uses a __-traversing ordering value, but the change is externally visible and untested.
| def resolve(self, engine, pks, raw_by_pk, method_context): | ||
| # Seed every field to None; the engine raises a need for each non-null | ||
| # target and fills it in once the batched forward query runs. | ||
| targets = [raw_by_pk[pk].get(self.link) for pk in pks] |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
nitpick: _make_plan guarantees self.link is in fetch_values, so .get() can only mask a plan-compilation bug as a spurious null FK.
SourceFieldEntry.extract (field_map.py:102) is deliberately strict for the same reason — prefer raw_by_pk[pk][self.link].
| LevelExpander = Callable[[Any, Optional["MethodContext"]], ExpandResult] | ||
|
|
||
|
|
||
| class OutputValidationError(Exception): |
There was a problem hiding this comment.
✅ Resolved — addressed in the current code.
praise: Good catch that keeping this a ValueError would let serialize_object turn DEBUG schema drift into a 404.
Nested fields that would cartesian-explode (multiple many=True) or nest too deep can't ride ValuesViewset's single values() query, so detect them and resolve each in a separate batched query. The serialization runtime lives in a ValuesEngine the viewset drives. Bucketing is keyed per distinct parent rather than per child row, and reverse-FK children pair with their link positionally instead of via a pk-keyed dict. Both fell out of profiling: str(UUID) per child row was the single largest cost, and at 100 authors x 300 books the derived path now matches a hand-written consolidate() (it was 16-34% slower). Link-pair dedup runs for M2M only. A reverse FK or 1:1 child has one parent, so its pairs are unique by construction; only an explicit through model, which has no unique constraint on the two FKs, can return the same (child, parent) twice. Also fixes a pre-existing bug this branch's fixtures exposed: a column read by both a rename and a ValuesMethodField was dropped from values() by rename promotion, because the refcount gating it counted only entry.source. Entries now declare read_columns. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EYSv7T5sxWKTyzverb1GXR
Assert the derived path matches an explicit consolidate() on output, query count, and timing, so the optimization can't silently regress. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EYSv7T5sxWKTyzverb1GXR
Also converts the file's bold pseudo-headings to real section headings. Bold-as-heading came in with the agent-authored backend_architecture docs; older hand-written docs use bold only for emphasis and bullet lead-ins. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EYSv7T5sxWKTyzverb1GXR
d4d4494 to
a3e9380
Compare
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #15085 — 9 of 9 prior findings resolved or acknowledged. Two new points on the inline-literal machinery, neither blocking.
The unbounded-IN fix landed with a behavioural test. So did the reverted DEBUG-validation narrowing. So did the consolidate() aliasing. The three "measure it, don't assert it" asks came back as a real benchmark arm. The Limitations section reports the upper bound rather than the flattering number.
CI: the single red check is Python postgres SSL smoke tests (3.9). It failed at Initialize containers — before checkout, so before any of this branch's code exists in the runner. Infra, not the PR. Python tests was still queued at review time. The touched suites (kolibri/core/test/test_api.py, exams, auth, courses) pass locally.
Prior-finding status
RESOLVED — kolibri/core/utils/values_viewset/fetch.py:239 — unbounded ... __in=<all parent pks> lists
RESOLVED — kolibri/core/utils/values_viewset/engine.py:575 — DEBUG validation narrowed to required-only
RESOLVED — kolibri/core/utils/values_viewset/engine.py:518 — shared forward targets aliased into consolidate()
RESOLVED — docs/backend_architecture/api_patterns.rst:226 — permission-scope claim holds for reverse fetches only
RESOLVED — kolibri/core/utils/values_viewset/introspect.py:287 — to-one round trip unmeasured against the joined path
RESOLVED — kolibri/core/api.py:379 — serialize_queryset kwarg renamed without a shim
RESOLVED — kolibri/core/utils/values_viewset/fetch.py:299 — non-strict .get() among strict siblings
ACKNOWLEDGED — kolibri/core/utils/values_viewset/fetch.py:277 — auto-deferred nested lists lost the ordering seam
ACKNOWLEDGED — kolibri/core/api.py:138 — ?ordering= valid-field set changes for forward nested serializers
RESOLVED — kolibri/core/utils/values_viewset/engine.py:45 — praise, DEBUG schema drift no longer a 404
@rtibblesbot's comments are generated by an LLM, and should be evaluated accordingly
How was this generated?
Ran an automatic code-only delta review triggered by new commits on a previously reviewed PR:
- Retrieved prior bot reviews via the GitHub API
- Classified each prior finding as RESOLVED, UNADDRESSED, ACKNOWLEDGED, or CONTESTED
- Only raised NEW findings for newly introduced code
- Core review pass only — specialized frontend/backend lenses and manual QA run when a review is explicitly requested
- Synthesized one review from the passes and chose the verdict from the findings, CI status, and QA evidence
| manager.model.__name__, | ||
| len(pks), | ||
| ) | ||
| lookup = InlineIn.lookup_name if all("'" not in str(pk) for pk in pks) else "in" |
There was a problem hiding this comment.
suggestion: The quote guard that makes InlineIn safe lives in the caller, not in the lookup.
InlineIninterpolates values straight into the statement ("'{}'".format(p), inherited fromUUIDIn.process_rhs).mixins.py:159registers it onField.inline_inis therefore a lookup on every model field in the process, third-party apps included.UUIDInhad the same coupling. It never escapedFilterByUUIDQuerysetMixin, which validates its ids at the single call site.
Moving the check into InlineIn.process_rhs, falling back to super().process_rhs(...) when any value contains a quote, buys two things:
- the lookup becomes safe by construction wherever it is reached from;
- it checks the values actually interpolated (
sqls_params, post-get_db_prep_value) rather thanstr(pk)on the pre-prepared Python value, which is what this line inspects.
Either way, worth a docstring line that the "in" fallback re-exposes the parameter cap the helper exists to avoid. A quote-carrying CharField pk on an unpaginated list lands back on too many SQL variables.
| # the MRO at itself, so a lookup on ``Field`` is invisible to them. | ||
| Field.register_lookup(InlineIn) | ||
| ForeignKey.register_lookup(InlineIn) | ||
| ManyToManyField.register_lookup(InlineIn) |
There was a problem hiding this comment.
nitpick: Redundant. The comment above is right about ForeignObject.get_lookups truncating the MRO. But ManyToManyField isn't a ForeignObject.
- Its MRO is
ManyToManyField → RelatedField → FieldCacheMixin → Field → RegisterLookupMixin. - It uses
RegisterLookupMixin.get_lookups, which walks the full MRO.Field.register_lookupalready covers it. - The M2M case doesn't reach here anyway.
filter(tags__inline_in=...)resolves the lookup against the target model's pk field.
ForeignKey is the only one that genuinely needs the extra line.
3 participants
Summary
The current ValuesViewset implementation from the first stage of the refactor handles a single foreign key traversal by doing a join
To prevent an explosion of results from the Cartesian product of many joins (in the case of M2M or one to many keys) it currently errors out if more than one relation is included in the serializer and demands it be satisfied by deferring the fetch and manual handling in the
consolidatemethod.This PR throws that out and takes on handling all kinds of relational lookup automatically, as long as it can be precisely specified by a DRF serializer.
In order to achieve this, it expands the ValuesViewset machinery considerably, but also separates it into discrete phases:
As part of this work, the overall structure was modularized to make it more comprehensible.
Lastly, there was an extant bug in the current implementation:
One caveat to the entire strategy is that this will frequently produce one extra query - as unlike hand coded consolidates that use JOINs, the strategy will always defer to a second query.
The benchmark scripts have also been rewritten, because they were not suitable for properly measuring the impact of the full support for relationship traversal. I reran them against existing develop with the new code to confirm that the only changes to existing viewsets was the additional query, and that with other optimizations, the overall timing differences were a wash. As these are ready only queries, this should scale even under load.
References
Fixes #14815
Reviewer guidance
My hope is that this makes the whole values viewset machinery significantly more readable - I have certainly found it more readable in my iterative reviews.
I think the only controversial thing here might be the usage of an extra query whenever there is a foreign key lookup - the most extreme example here is for the Facility endpoint, where if there is only one facility, it then does a full extra query for just the one facility dataset - but the timing difference is still miniscule in spite of the extra query.
AI usage
This was developed in extensive collaboration with Claude Code. Initial brainstorming started out with just minor tweaks to shift the existing serializer_introspection more in this direction, but as the code grew in both size and complexity (but with tests in place) I directed it to a more object oriented, modularized refactor - with the specific separation of concerns between introspection + planning, and the execution engine. Having this clear separation helped to fuel multiple optimizations and ensure that all decisions that could be made with foreknowledge about the serializer were made in the planning phase, and the engine was only responsible for execution.
Following implementation, this has been through multiple rounds of human and bot review, improving readability, finding edge cases, and squashing any remaining performance issues that occurred.