fix: order and bound the revisions query in WikiPage.get_context

[Aanzan426]

Bounds and orders the revisions query in WikiPage.get_context. This is the Unity-owned copy of the change — mergeable by us, on the branch we actually deploy (master, 2372f859f6).

Also raised upstream as frappe/wiki#742 so the fix is on record with the maintainers. That one is not mergeable by us and frappe/wiki:master has had no commits since 2025-12-18, so it should be treated as a contribution, not as our delivery path.

The problem

get_context loads every revision of a page, with the full content blob on each, then uses only revisions[0] and revisions[1]:

revisions = frappe.db.get_all(
    "Wiki Page Revision",
    filters=[["wiki_page", "=", self.name]],
    fields=["content", "creation", "owner", "name", "raised_by", "raised_by_username"],
)
context.current_revision = revisions[0]
if len(revisions) > 1:
    context.previous_revision = revisions[1]

Two problems, one large and one small.

1. There is no LIMIT. Only two rows are ever used. On a page with 107 revisions that is 3.37 MB read to use 130 KB, and it grows with edit history — so the most-edited pages, usually also the most-read, pay the most.

2. The ordering is by the wrong column. There is an implicit ORDER BY — Frappe supplies one from the doctype's sort_field, so the query emits:

ORDER BY `tabWiki Page Revision`.`modified` DESC

modified is when the row was last written, not when the revision was made. They coincide while revisions are only appended, but any write touching an older revision's row moves it to the front, and it is then handed to the template as current_revision.

The fix

  fields=["content", "creation", "owner", "name", "raised_by", "raised_by_username"],
+ order_by="creation desc",
+ limit_page_length=2,

Impact

Measured on a page with 107 revisions:

	before	after
query time	32.7 ms	5.7 ms
revision content read	3.37 MB	130 KB

On a single-revision page the difference is negligible either way — this is not a median-case win, it is an outlier and correctness win.

Tests

• test_get_context_orders_revisions_by_creation_not_modified — gives three revisions a modified order deliberately opposite to their creation order, so the default sort and chronological order disagree, then asserts current_revision / previous_revision are the two newest by creation. Verified to fail on master and pass with this change.
• test_get_context_handles_a_page_with_one_revision — pins the single-revision case, where previous_revision must stay the "No Revisions" placeholder. That is the branch a LIMIT is most likely to break.

Full test_wiki_page.py suite: 4 tests, all passing.

Compatibility

No behaviour change for callers. The same two values are assigned, and len(revisions) > 1 still distinguishes a one-revision page: the query returns one row, so the placeholder path is preserved.

Deployment note

Merging this puts the fix in the Unity fork. It only reaches walnut if the bench is pointed at the fork for wiki — a separate decision, and one that cuts against the overrides-first approach used for walnut_customizations#114. Merging here is still worth it: it means the fix exists somewhere we control rather than only in an upstream PR that may sit indefinitely.

Ref: WalnutAppSuite/walnut_customizations#114

[sankarsubramaniankvs]

Code Review - PR #1

Reviewer: OpenClaw Bot
Verdict: Approve
Files reviewed: 2 | Issues found: Critical 0, Major 0, Minor 0, Suggestion 0

Summary

This is a small, targeted fix to WikiPage.get_context: it makes revision selection deterministic by creation time and avoids loading every revision when only the current and previous revisions are used. The tests cover both the corrected ordering behavior and the single-revision placeholder path.

What's Done Well

• The production change is minimal: explicit order_by="creation desc" plus limit_page_length=2.
• The ordering test deliberately makes modified DESC disagree with creation DESC, which proves the actual bug rather than only testing the happy path.
• The single-revision test protects the branch most likely to regress from adding the limit.
• The PR description clearly explains the fork/upstream split and why this Unity-owned PR is the deployable path.

Issues

No blocking issues found.

File-by-File Summary

• wiki/wiki/doctype/wiki_page/wiki_page.py - Clean. Query now selects the two intended rows deterministically and cheaply.
• wiki/wiki/doctype/wiki_page/test_wiki_page.py - Clean. Coverage is focused and relevant.

Note: GitHub reports no checks on this branch, so I reviewed the code and test intent but did not see CI results here.

Reviewed by OpenClaw Bot - Unity Edu

[Aanzan426]

Closing this — not because the fix is wrong, but because the delivery path costs more than the fix is worth. Recording the reasoning so it can be re-opened on evidence rather than re-litigated from scratch.

It does not help the work it was raised for

Ref: WalnutAppSuite/walnut_customizations#114 is parent-facing help-page performance. Parents never execute this code. WikiPage.get_context runs only when the wiki's own renderer serves a wiki URL; parents are redirected into the /help/student-life-std1to10 Web Page wrapper, which reads Wiki Page content with frappe.get_all and never calls get_context. So the benefit is staff-path only, and independent of the caching work — which has since shipped as WalnutAppSuite/edu_quality#3552 with the wiki changes stripped out entirely.

The measured benefit is narrower than the PR implies

Re-measured on the walnut bench, all 1,169 pages that have revisions:

Median revisions per page	1
Exactly one revision	830 of 1,169 (71%)
More than 10	71
More than 50	5
Worst page	147 revisions, 1.75 MB

The 107-revision page quoted above (3.37 MB read to use 130 KB) is real, and it is the second-worst page on the site. For 71% of pages the query returns a single row either way — nothing to bound, and no ordering to get wrong. This is a genuine tail fix, not a median one, and the PR body says as much; the point here is only that the tail is ~71 pages, none of them parent-facing.

Correcting one claim in the description above

It only reaches walnut if the bench is pointed at the fork for wiki — a separate decision.

On the walnut dev bench that decision is already made — apps/wiki has unity → UnityAppSuite/wiki as its fetch/push remote, currently on unpatched master @ 2372f85. So merging here would propagate on the next pull. Production has not been verified. That makes the fork question more live, not less: merging means every future bench update on wiki has to reconcile against a patched fork, permanently, to carry two lines.

On doing it as an override instead

Per @badal8381's suggestion — worth writing down what is actually possible, because the answer is "half of it".

The query is inline in get_context, which spans lines 216–337 (~122 lines). There is no get_revisions() seam to override.

• Copy the method into an override — 122 lines duplicated to change 2. That is a fork in disguise, and drifts silently the moment upstream touches get_context.
• super() then fix up — call super().get_context(context), then overwrite context.current_revision / previous_revision from our own order_by="creation desc", limit_page_length=2 query. This fixes the ordering bug, which is the correctness half. It does not fix the unbounded read — super() has already pulled every revision — and it adds one extra query, so it is fractionally slower.

So an override can buy correctness but not performance. If the modified-vs-creation ordering turns out to bite someone, that override is the cheap answer and needs no fork.

The fix stays on record

frappe#742 remains open and is the right home for it. Note frappe/wiki:master has had no commits since 2025-12-18, so that should be treated as a filing, not a plan.

What would justify re-opening

• A report of a wrong current_revision being displayed — the ordering bug actually biting, rather than being theoretical.
• Wiki URLs becoming parent-facing (i.e. the redirect into the Web Page wrapper being removed).
• The revision tail growing — the 5 pages above 50 revisions becoming many.
• A decision to run the Unity wiki fork in production for other reasons, at which point this rides along for free.