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

[Aanzan426]

Correction (see comment below): an earlier version of this description claimed the query had no ORDER BY. That was wrong — Frappe injects one from the doctype's sort_field. The description below is corrected; the change itself is unchanged.

The problem

WikiPage.get_context loads every revision of a page, with the full content blob on each, and 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 things are wrong with it, one large and one small.

1. There is no LIMIT. Only two rows are ever used, but every revision is fetched with its full content. 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 ever appended, but any write that touches an older revision's row moves it to the front, and it is then handed to the template as current_revision. Sorting explicitly by creation says what is actually meant.

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 page with a single revision the difference is negligible either way.

Tests

Two tests added to wiki/wiki/doctype/wiki_page/test_wiki_page.py:

• test_get_context_orders_revisions_by_creation_not_modified — gives three revisions a modified order deliberately opposite to their creation order, so the default modified DESC sort and chronological order disagree, then asserts current_revision and 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 most easily broken by adding a limit.

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.

Found while profiling wiki page rendering under concurrent load.

[sankarsubramaniankvs]

Code Review - PR #742

Reviewer: OpenClaw Bot
Verdict: Request Changes
Files reviewed: 1 | Issues found: Critical 0 Major 1 Minor 0 Suggestions 0

Summary

The code change is directionally right: current_revision should not depend on unordered database results, and bounding the query to the two rows the template actually uses is a clean performance win. The issue is that this changes backend behavior and currently ships without a regression test, while the repo already has wiki/wiki/doctype/wiki_page/test_wiki_page.py covering the Wiki Page lifecycle.

What's Done Well

• order_by="creation desc" fixes the correctness problem directly.
• limit_page_length=2 is the right bound because only current and previous revision are used.
• The change is small and localized.

Issues

Major

wiki/wiki/doctype/wiki_page/wiki_page.py:304 - Missing regression test for revision ordering

This fixes a real backend correctness issue, but there is no test proving get_context() now chooses the newest revision as current_revision and the next newest as previous_revision. Without that coverage, a future refactor can drop the order_by or limit and silently reintroduce the same unstable behavior.

Please add a small test under wiki/wiki/doctype/wiki_page/test_wiki_page.py that creates or updates a page to produce at least three revisions, calls get_context(), and asserts:

• context.current_revision is the newest revision
• context.previous_revision is the second-newest revision
• older revisions are not selected

If you want to make the performance part explicit, monkeypatching/wrapping frappe.db.get_all to assert order_by="creation desc" and limit_page_length=2 is also fine, but a behavior test is the important bit.

File-by-File Summary

• wiki/wiki/doctype/wiki_page/wiki_page.py - Correct query change, needs test coverage.

Checks reviewed:

• GitHub reports no checks on fix/bound-revisions-query.

Reviewed by OpenClaw Bot - Unity Edu

[Aanzan426]

Tests added in 22c59e3, and a correction to my own description.

Correction

My original description said the query had no ORDER BY and that revisions[0] was therefore whatever the database returned first. That was wrong, and I want it on the record rather than quietly edited away.

Frappe supplies a default ORDER BY from the doctype's sort_field. Capturing the SQL actually generated:

unpatched   ORDER BY `tabWiki Page Revision`.`modified` DESC     LIMIT: none
patched     ORDER BY creation desc                               LIMIT: 2

So the ordering is deterministic today — it is just sorted by the wrong column. modified is when a row was last written, not when the revision was made; the two coincide only while revisions are purely appended, and any write touching an older revision's row promotes it to current_revision. That is a narrower claim than I originally made, and I have updated the description to match.

I caught this because the first version of the test passed without the fix applied. I had set creation but not modified, so the real sort order never changed and the test proved nothing.

What this means for the change

• The missing LIMIT is unaffected by the correction and is the larger practical win — 3.37 MB read to use 130 KB on a 107-revision page.
• The ordering change is still correct, just for the narrower reason above.

Tests

test_get_context_orders_revisions_by_creation_not_modified now sets modified to the opposite order from creation, so the default sort and chronological order genuinely disagree. Verified both ways:

with fix:      OK
without fix:   FAIL — AssertionError: 'n4r2tv3c54' != 'n53aiiithk'

test_get_context_handles_a_page_with_one_revision covers the single-revision path, where previous_revision must remain the "No Revisions" placeholder — the branch a limit is most likely to break.

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

Happy to drop the order_by and ship this as a pure LIMIT change if you consider the modified versus creation distinction too marginal to be worth the behaviour change.

[Aanzan426]

Tests added in 22c59e3, and a correction to my own description.

Correction

My original description said the query had no ORDER BY and that revisions[0] was therefore whatever the database returned first. That was wrong, and I want it on the record rather than quietly edited away.

Frappe supplies a default ORDER BY from the doctype's sort_field. Capturing the SQL actually generated:

unpatched   ORDER BY `tabWiki Page Revision`.`modified` DESC     LIMIT: none
patched     ORDER BY creation desc                               LIMIT: 2

So the ordering is deterministic today — it is just sorted by the wrong column. modified is when a row was last written, not when the revision was made; the two coincide only while revisions are purely appended, and any write touching an older revision's row promotes it to current_revision. That is a narrower claim than I originally made, and I have updated the description to match.

I caught this because the first version of the test passed without the fix applied. I had set creation but not modified, so the real sort order never changed and the test proved nothing.

What this means for the change

• The missing LIMIT is unaffected by the correction and is the larger practical win — 3.37 MB read to use 130 KB on a 107-revision page.
• The ordering change is still correct, just for the narrower reason above.

Tests

test_get_context_orders_revisions_by_creation_not_modified now sets modified to the opposite order from creation, so the default sort and chronological order genuinely disagree. Verified both ways:

with fix:      OK
without fix:   FAIL — AssertionError: 'n4r2tv3c54' != 'n53aiiithk'

test_get_context_handles_a_page_with_one_revision covers the single-revision path, where previous_revision must remain the "No Revisions" placeholder — the branch a limit is most likely to break.

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

Happy to drop the order_by and ship this as a pure LIMIT change if you consider the modified versus creation distinction too marginal to be worth the behaviour change.

[sankarsubramaniankvs]

Code Review - PR #742

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

Summary

The earlier blocker is addressed. The implementation now explicitly selects the two newest revisions by creation desc, which fixes the correctness issue around modified ordering and avoids loading the full revision history when only current/previous are used.

What's Done Well

• order_by="creation desc" captures the actual revision chronology instead of relying on the DocType default modified desc sort.
• limit_page_length=2 is the right bound for this code path because only current_revision and previous_revision are consumed.
• The added tests cover both the ordering regression and the single-revision placeholder branch, including the case where modified order disagrees with creation order.

File-by-File Summary

• wiki/wiki/doctype/wiki_page/wiki_page.py - Clean, localized query fix.
• wiki/wiki/doctype/wiki_page/test_wiki_page.py - Good regression coverage for the fixed behavior.

Checks reviewed:

• GitHub reports no checks on fix/bound-revisions-query.
• I did not run the Frappe test suite locally in this workspace.

Reviewed by OpenClaw Bot - Unity Edu