refactor(catchall): rewrite version normalization in catchall to hand… by YishaiGlasner · Pull Request #3133 · Sefaria/Sefaria-Project

YishaiGlasner · 2026-03-05T16:01:03Z

Description

Rewrite version normalization in catchall to handle non-existent versions and support multi-panel URLs.
Replace _reader_redirect_add_languages with a more robust version normalization flow that supports per-panel version params (ven, ven1, etc.). The redirect now fires when current params differ from normalized ones, rather than whenever a '|' separator is missing. Also, when the required version doesn't exist it redirects to default (conserving the redirect for legacy urls without version language).

Functions description

_get_current_and_normalized_versions - returns current_versions a dict of all version params (ven, vhe, ven1...) in the request, and normalized_versions all the normalized params.
_get_normalized_versions - returns the normalized versions for specific panel (with tref and optional ven and vhe).
_reader_redirect_versions - returns a redirect with the normalized params.

…le non-existent versions and support multi-panel URLs Replace _reader_redirect_add_languages with a more robust version normalization flow that supports per-panel version params (ven, ven1, etc.). The redirect now fires when current params differ from normalized ones, rather than whenever a '|' separator is missing. Also, when the required version doesn't exist it redirects to default (conserving the redirect for legacy urls without version language).

Copilot

Pull request overview

This PR refactors the version normalization logic in the catchall view to handle non-existent versions (by redirecting to a version-less URL) and to support multi-panel URLs with per-panel version parameters (ven1, vhe1, etc.). The old _reader_redirect_add_languages function, which only triggered redirects when a | separator was missing, is replaced by a more general flow that compares current version params against their normalized forms and redirects whenever they differ.

Changes:

Replace _reader_redirect_add_languages with three new functions: _reader_redirect_versions, _get_normalized_versions, and _get_current_and_normalized_versions
Add support for multi-panel version normalization by processing p1/p2 panel refs and their corresponding ven1/vhe1 version params
Redirect to a URL with non-existent version params removed (rather than only normalizing format)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-05T16:05:42Z

reader/views.py

+    return redirect(f'/{tref}/?{query_params.urlencode()}')
+

+def _get_normalized_versions(tref, ven, vhe):


_get_normalized_versions calls Ref(tref).version_list() (a DB query) on line 393 before checking whether ven or vhe are None. Since _get_current_and_normalized_versions is called on every catchall request (including those with no version params), this introduces an unnecessary DB call on every page load. Consider adding an early return at the top of _get_normalized_versions when both ven and vhe are falsy, e.g. if not ven and not vhe: return [None, None].

Suggested change

def _get_normalized_versions(tref, ven, vhe):

def _get_normalized_versions(tref, ven, vhe):

if not ven and not vhe:

return [None, None]

reader/views.py

mergify · 2026-03-05T16:44:09Z

🧪 CI Insights

Here's what we observed from your CI run for 8e75dd9.

❌ Job Failures

Pipeline	Job	Health on `master`	Retries	🔍 CI Insights	📄 Logs
`Continuous`	`Continuous Testing: PyTest`		`0`	View	View

yodem

PR Review: Rewrite version normalization in catchall

Scope: Single file (reader/views.py), +47/-15 lines. Backend-only change.

CI Status: PyTest is failing. This needs investigation before merging.

issue: Fallback logic may silently serve wrong version (lines 407-408)

The cascading match in _get_normalized_versions:

version = (next((v for v in candidates if v['versionTitle'] == vtitle and v['languageFamilyName'] == lang), None)
           or next((v for v in candidates if v['languageFamilyName'] == lang), None)  # ← this
           or next((v for v in candidates if v['versionTitle'] == vtitle), None))

The second fallback matches any version with the same language family, ignoring the requested version title entirely. If a user requests English|Nonexistent_Title, they'd silently get an arbitrary English version. Is this the intended behavior? This could mask broken URLs rather than surfacing 404s, and the user would see different content than expected without any indication.

suggestion: Variable shadowing of `tref` (line 421)

def _get_current_and_normalized_versions(request, tref):
    ...
    for panel_num, tref in tref_mappings.items():  # shadows parameter

The loop variable tref shadows the function parameter. While functionally fine (the parameter value is preserved in tref_mappings['']), it's a readability trap. Consider renaming to panel_tref.

question: Handling of `p` params that aren't valid refs

Line 418 extracts p1, p2, etc. from query params and passes their values to Ref(tref).version_list() inside _get_normalized_versions. If a p1 value is not a valid ref, this will raise an exception. Is there any upstream validation ensuring these are always valid, or should there be a try/except here?

nitpick: Missing blank line (line 415-416)

PEP 8 recommends two blank lines between top-level function definitions. There's only one between _get_normalized_versions and _get_current_and_normalized_versions.

praise: Good optimization to skip DB query

Line 393-394 avoids hitting the database when no version params are present, which is likely the common case.

praise: Cleaner redirect logic

The new current_versions != normalized_versions check in catchall is much cleaner than the old approach of checking for | absence. It correctly handles all cases including already-normalized URLs (no unnecessary redirect).

praise: Multi-panel support

The old code only handled ven/vhe for the primary panel. The new code correctly handles ven1/vhe1, ven2/vhe2, etc. via tref_mappings. This is a meaningful improvement.

Summary

Aspect	Assessment
Correctness	Generally good, but the fallback logic (serving any version of matching language) may be too permissive
CI	Failing - PyTest needs investigation
Security	No new concerns - follows existing patterns
Performance	Improved (early return avoids unnecessary DB queries)
Readability	Good structure. Minor variable shadowing issue

Recommendation: Investigate CI failure and clarify the language-only fallback behavior before merging.

YishaiGlasner · 2026-03-08T10:48:08Z

PR Review: Rewrite version normalization in catchall

Scope: Single file (reader/views.py), +47/-15 lines. Backend-only change.

CI Status: PyTest is failing. This needs investigation before merging.

issue: Fallback logic may silently serve wrong version (lines 407-408)

The cascading match in _get_normalized_versions:
version = (next((v for v in candidates if v['versionTitle'] == vtitle and v['languageFamilyName'] == lang), None)
           or next((v for v in candidates if v['languageFamilyName'] == lang), None)  # ← this
           or next((v for v in candidates if v['versionTitle'] == vtitle), None))
The second fallback matches any version with the same language family, ignoring the requested version title entirely. If a user requests English|Nonexistent_Title, they'd silently get an arbitrary English version. Is this the intended behavior? This could mask broken URLs rather than surfacing 404s, and the user would see different content than expected without any indication.

suggestion: Variable shadowing of tref (line 421)
def _get_current_and_normalized_versions(request, tref):
    ...
    for panel_num, tref in tref_mappings.items():  # shadows parameter
The loop variable tref shadows the function parameter. While functionally fine (the parameter value is preserved in tref_mappings['']), it's a readability trap. Consider renaming to panel_tref.

question: Handling of p params that aren't valid refs

Line 418 extracts p1, p2, etc. from query params and passes their values to Ref(tref).version_list() inside _get_normalized_versions. If a p1 value is not a valid ref, this will raise an exception. Is there any upstream validation ensuring these are always valid, or should there be a try/except here?

nitpick: Missing blank line (line 415-416)

PEP 8 recommends two blank lines between top-level function definitions. There's only one between _get_normalized_versions and _get_current_and_normalized_versions.

praise: Good optimization to skip DB query

Line 393-394 avoids hitting the database when no version params are present, which is likely the common case.

praise: Cleaner redirect logic

The new current_versions != normalized_versions check in catchall is much cleaner than the old approach of checking for | absence. It correctly handles all cases including already-normalized URLs (no unnecessary redirect).

praise: Multi-panel support

The old code only handled ven/vhe for the primary panel. The new code correctly handles ven1/vhe1, ven2/vhe2, etc. via tref_mappings. This is a meaningful improvement.

Summary

Aspect Assessment
Correctness Generally good, but the fallback logic (serving any version of matching language) may be too permissive
CI Failing - PyTest needs investigation
Security No new concerns - follows existing patterns
Performance Improved (early return avoids unnecessary DB queries)
Readability Good structure. Minor variable shadowing issue
Recommendation: Investigate CI failure and clarify the language-only fallback behavior before merging.

@yodem added newline.
yes, the issue here is default rather than 404.
when the second panel has an invalid ref, the durrent behaviour is removing the panel, and it insists (as per my checking).
tests will probably pass now (it's arbitrary and Noah handles it).

reader/views.py

yitzhakc · 2026-03-09T08:56:42Z

reader/views.py

+            normalized.append(None)
+    return normalized
+
+def _get_current_and_normalized_versions(request, tref):


Please add a docstring, so one can understand in one line what this function is doing and returning 😄

…d _get_normalized_versions.

YishaiGlasner requested review from Copilot, yitzhakc and yodem March 5, 2026 16:01

Copilot started reviewing on behalf of YishaiGlasner March 5, 2026 16:01 View session

Copilot AI reviewed Mar 5, 2026

View reviewed changes

refactor(catchall): do not query db when not necessary.

d59ca4a

yodem reviewed Mar 8, 2026

View reviewed changes

chore: newline

e6b194f

yitzhakc requested changes Mar 9, 2026

View reviewed changes

YishaiGlasner added 3 commits March 12, 2026 11:37

docs(catchall): docstring for _reader_redirect_versions.

502e343

docs(catchall): docstring for _get_current_and_normalized_versions an…

70b15d1

…d _get_normalized_versions.

test(catchall): test versions redirections.

8e75dd9

YishaiGlasner requested a review from yitzhakc March 12, 2026 10:39

		return redirect(f'/{tref}/?{query_params.urlencode()}')


		def _get_normalized_versions(tref, ven, vhe):

Uh oh!

Conversation

YishaiGlasner commented Mar 5, 2026

Description

Functions description

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mergify bot commented Mar 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🧪 CI Insights

❌ Job Failures

Uh oh!

yodem left a comment

Choose a reason for hiding this comment

PR Review: Rewrite version normalization in catchall

issue: Fallback logic may silently serve wrong version (lines 407-408)

suggestion: Variable shadowing of tref (line 421)

question: Handling of p params that aren't valid refs

nitpick: Missing blank line (line 415-416)

praise: Good optimization to skip DB query

praise: Cleaner redirect logic

praise: Multi-panel support

Summary

Uh oh!

YishaiGlasner commented Mar 8, 2026

PR Review: Rewrite version normalization in catchall

issue: Fallback logic may silently serve wrong version (lines 407-408)

suggestion: Variable shadowing of tref (line 421)

question: Handling of p params that aren't valid refs

nitpick: Missing blank line (line 415-416)

praise: Good optimization to skip DB query

praise: Cleaner redirect logic

praise: Multi-panel support

Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yitzhakc Mar 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mergify bot commented Mar 5, 2026 •

edited

Loading

suggestion: Variable shadowing of `tref` (line 421)

question: Handling of `p` params that aren't valid refs

suggestion: Variable shadowing of `tref` (line 421)

question: Handling of `p` params that aren't valid refs