Add a command palette for searching models, records and commands - #1121
Add a command palette for searching models, records and commands#1121vahidzhe wants to merge 4 commits into
Conversation
… names
against the in-memory view registry without touching the database, searches
records inside a single scoped model with exactly one query, and fans out
across opted-in models for unscoped searches.
Models join the unscoped search by setting palette_search = True. The default
is False so that adding the palette to an existing admin never starts querying
every registered table. The fan-out is capped by palette_search_max_models and
gated by palette_search_min_chars, and runs concurrently on async engines.
Below the matches the palette offers 'go to' and 'create' for the model that
best fits the current term. ModelView.palette_commands returns them and can be
overridden to add, replace or reorder entries.
is_visible, is_accessible and check_can_view_details are all enforced, so the
palette cannot surface a model or row the user could not otherwise reach, and
the endpoint sits behind the same login_required as every other admin route.
The top bar previously rendered only when a language switcher was configured,
so it is now unconditional with the switcher itself left conditional.
All strings are translated, including those rendered by JavaScript. Strings
interpolating a model name are whole sentences with {name} and {count}
placeholders substituted client-side, so translators control word order.
| function saPaletteEsc(value) { | ||
| return $("<div>").text(value == null ? "" : value).html(); | ||
| } |
There was a problem hiding this comment.
This func escapes for element content, not for attribute values. $("
There was a problem hiding this comment.
class Doc(Base):
slug = Column(String(200), primary_key=True) # user-suppliable slug
title = Column(String(80))
s.add(Doc(slug='x" tabindex=0 autofocus onfocus=window.__pwned=1 y="',
title="quarterly report"))Endpoint:
{"pk": "x\" tabindex=0 autofocus onfocus=window.__pwned=1 y=\"",
"url": "http://testserver/admin/doc/details/x\" tabindex=0 autofocus onfocus=window.__pwned=1 y=\""}
which is rendered:
DOM attrs: class="sa-row", data-url="http://testserver/admin/doc/details/x",
tabindex="0", autofocus="", onfocus="window.__pwned=1", y=""
window.__pwned is 1 with zero user interaction — autofocus + tabindex makes the injected row focusable and fires onfocus on render. An onmouseover variant fires as soon as the admin's cursor crosses the results list.
.replace(/"/g, """)
.replace(/'/g, "'");
Add these
| def palette_base_query(view: ModelView) -> Select: | ||
| """Base statement for palette search. | ||
|
|
||
| Kept separate from ``ModelView.list_query`` (which takes a ``Request`` and | ||
| may apply per-request scoping that does not make sense for a global search). | ||
| Relationships are intentionally *not* eager-loaded: the label is rendered | ||
| from already-selected columns, so we avoid extra joins. | ||
| """ | ||
|
|
||
| return select(view.model) |
There was a problem hiding this comment.
Returns 500 for any model that has relationship and uses that in str.
smth like this
class Player(Base):
team = relationship("Team")
def __str__(self):
return f"{self.name} ({self.team.name if self.team else '-'})"There was a problem hiding this comment.
Smth:
def palette_base_query(view: ModelView) -> Select:
stmt = select(view.model)
for relation in view._list_relations:
stmt = stmt.options(selectinload(relation))
return stmt| Kept separate from ``ModelView.list_query`` (which takes a ``Request`` and | ||
| may apply per-request scoping that does not make sense for a global search). |
There was a problem hiding this comment.
I think it does. If I check user session in request, then that should apply to global as well
| def palette_search_query(self, term: str) -> Select: | ||
| return ( | ||
| select(Article) | ||
| .where(Article.search_vector.match(term)) | ||
| .limit(self.palette_search_limit) | ||
| ) |
There was a problem hiding this comment.
This is not correct, your views does not have palette_search_query function
| function saPaletteFetch() { | ||
| // Native trim: jQuery 4.0 removed $.trim. | ||
| var term = ($("#sa-palette-input").val() || "").trim(); | ||
| $.ajax({ | ||
| url: window.SA_PALETTE_URL, | ||
| method: "GET", | ||
| dataType: "json", | ||
| data: saPaletteScope ? { q: term, scope: saPaletteScope } : { q: term }, | ||
| headers: { "X-Requested-With": "XMLHttpRequest" }, | ||
| success: saPaletteRender, | ||
| error: function () { | ||
| $("#sa-palette-results").html( | ||
| '<div class="sa-empty">' + saPaletteEsc(saPaletteText("searchFailed")) + "</div>" | ||
| ); | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
This func fires a new $.ajax on every debounce tick and never cancels the previous one, and saPaletteRender reads the term back out of the input box at render time (line 158) rather than using the term the response was for. Responses are not guaranteed to arrive in order, and the shorter term is usually the slower query because it matches more rows — so the common typing pattern loses the race.
| function saPaletteSingular(name) { | ||
| if (/ies$/.test(name)) { | ||
| return name.slice(0, -3) + "y"; | ||
| } | ||
| if (/s$/.test(name)) { | ||
| return name.slice(0, -1); | ||
| } | ||
| return name; | ||
| } |
| if not view._search_fields or not view.can_view_details: | ||
| return [] |
There was a problem hiding this comment.
Not tested. Reachable via a scoped search against a model with can_view_details = False or with no column_searchable_list
| @login_required | ||
| async def palette(self, request: Request) -> Response: | ||
| """Command-palette search endpoint. | ||
|
|
||
| Model name matches are served from the in-memory view registry (no DB | ||
| access). A ``?scope=<identity>`` query runs a single query against one | ||
| model. An unscoped query fans out only across models that set | ||
| ``palette_search = True``, capped and run concurrently. See | ||
| ``sqladmin.palette.build_palette_response`` for details. | ||
| """ |
There was a problem hiding this comment.
login_required answers an expired session with a 302 to the HTML login page. jQuery follows it, dataType: "json" fails to parse the login page, and the user sees "Search failed" with no hint that they need to log in again. Returning 401 for X-Requested-With: XMLHttpRequest (which palette.js:146 already sends) and having the error handler redirect would be a nicer dead end.
| palette_search_min_chars: int = 2, | ||
| palette_search_max_models: int = 8, |
There was a problem hiding this comment.
Validate values: max(0, value)
| palette_search: ClassVar[bool] = False | ||
| """Whether this model joins *unscoped* command-palette search. | ||
|
|
||
| Defaults to ``False`` so adding the palette never silently fans a query out | ||
| across every model. Reachable from an unscoped query only when this is | ||
| ``True`` and ``column_searchable_list`` is set. Scoped search (a single | ||
| model picked first) always works regardless, being one query on one model. | ||
| """ | ||
|
|
||
| palette_search_limit: ClassVar[int] = 5 |
There was a problem hiding this comment.
Move this a little down/up. You kinda split search related args (column_searchable_list, search_auto_submit)
|
@mmzeynalli Thanks, especially for the XSS one. Fixed all ten, pushed as a new commit 37 tests now (was 30), passing on SQLite and a real PostgreSQL 16. ruff, mypy
Relationship in
Docs example calls a method that doesn't exist. Correct -> it was a module Debounce race condition. Fixed with a request sequence counter plus
Untested 302 redirect breaks the JSON client. Added Unvalidated negative config values. Clamped with ClassVar placement. Moved Ready for another pass. |
mmzeynalli
left a comment
There was a problem hiding this comment.
The one I'd call blocking is the javascript: sink in palette.js — it's one line to guard. The asyncio.gather failure isolation and the | tojson escaper are correctness; the "one query" badge, the silent model cap, and the layout change are accuracy and scope.
On the layout change specifically: it affects every deployment, not just palette users, so it needs calling out in the description even if it stays in this PR.
| $(document).on("click", "#sa-palette-results .sa-row", function () { | ||
| var url = $(this).attr("data-url"); | ||
| if (url) { | ||
| window.location.href = url; |
There was a problem hiding this comment.
data-url is escaped correctly as an attribute, but this executes javascript: URLs. A command with url: "javascript:alert('sink')" fires on click.
Nothing server-side can produce that today, since every URL comes from request.url_for. But palette_commands is documented as returning an arbitrary url and the docstring example builds one, so a command URL derived from a DB field would be stored XSS. Can we guard at the sink?
if (url && /^(\/|https?:)/i.test(url)) { window.location.href = url; }
|
|
||
| stmt = view.list_query(request) | ||
| for relation in view._list_relations: | ||
| stmt = stmt.options(selectinload(relation)) |
There was a problem hiding this comment.
The eager-loading is right — the DetachedInstanceError reasoning in the docstring is correct. But it means "exactly one query" isn't accurate, and the UI shows a badge that literally says "one query" (oneQuery in palette.html).
Counted with an engine listener, scoped search on a model with one relationship in column_list:
SQL statements issued -> 2
SELECT doc.id, doc.title, doc.tag_id FROM doc WHERE lower(...) LIKE ...
SELECT tag.id, tag.name FROM tag WHERE tag.id IN (?, ?, ?, ?, ?)
With three relations, it's four. Could the badge go, or become 1 + len(view._list_relations)?
| searchable = [v for v in accessible if v.palette_search and v._search_fields][ | ||
| : admin.palette_search_max_models | ||
| ] | ||
| chunks = await asyncio.gather( |
There was a problem hiding this comment.
Without return_exceptions=True, one bad model breaks the whole palette. I gave one model a palette_search_query that raises:
unscoped, one model raises -> 500
scoped into healthy model -> 200, 1 records
palette_search_query is a documented override, and a transient error on one table would do the same. Could we degrade instead — return_exceptions=True, log it, skip that model's chunk? The user loses one model's hits rather than the search box.
| # ---- unscoped record fan-out (opt-in models only) ----------------------- | ||
| records: list[PaletteResult] = [] | ||
| if len(term) >= admin.palette_search_min_chars: | ||
| searchable = [v for v in accessible if v.palette_search and v._search_fields][ |
There was a problem hiding this comment.
The cap slices in registration order, so which models get searched is incidental. With 15 searchable models registered I get hits from 7 and no indication about the rest — a term that only matches the twelfth model reads as "Nothing found".
The cap is right; the silence is the problem. Could the response carry searched/total counts so the UI can say "searched 8 of 15 models, keep typing to narrow"?
| <script> | ||
| window.SA_PALETTE_URL = "{{ url_for('admin:palette') }}"; | ||
| window.SA_PALETTE_I18N = { | ||
| models: "{{ _('Models') }}", | ||
| modelsHint: "{{ _('registry, no database query') }}", | ||
| commands: "{{ _('Commands') }}", | ||
| records: "{{ _('Records') }}", | ||
| // {name} is substituted client-side so translators keep word order. | ||
| recordsIn: "{{ _('Records in {name}') }}", | ||
| oneQuery: "{{ _('one query') }}", | ||
| searchInside: "{{ _('Search inside') }}", | ||
| goTo: "{{ _('Go to {name}') }}", | ||
| create: "{{ _('Create {name}') }}", | ||
| page: "{{ _('page') }}", | ||
| new: "{{ _('new') }}", | ||
| open: "{{ _('open') }}", | ||
| more: "{{ _('{count} more, keep typing to narrow') }}", | ||
| noMatches: "{{ _('No matches') }}", | ||
| nothingFound: "{{ _('Nothing found') }}", | ||
| searchFailed: "{{ _('Search failed') }}", | ||
| searchPlaceholder: "{{ _('Search models, records, commands') }}", | ||
| searchInsidePlaceholder: "{{ _('Search inside {name}') }}" | ||
| }; | ||
| </script> No newline at end of file |
| <div class="container-fluid"> | ||
| {# Language switcher, aligned to the right. #} | ||
| {# Command palette trigger, aligned to the left. #} | ||
| <button type="button" class="sa-palette-trigger" data-sa-palette-open |
There was a problem hiding this comment.
This hunk does two things beyond adding the trigger: it moves Logout out of the sidebar into the topbar, and makes the topbar render unconditionally where it was gated on show_switcher. Rendered against both trees with an auth backend and no i18n config:
main : logout inside sidebar: True <header> topbar exists: False
head : logout inside sidebar: False <header> topbar exists: True
So every deployment gets a new topbar and a relocated Logout, whether or not they use the palette. Reasonable — the trigger needs a home — but it will break anyone who overrode layout.html, so it deserves a line in the PR description and the changelog.
Adds a command palette to the admin: a modal that searches registered models,
records inside them, and commands, from any page.
Why
Finding a specific record today means picking the right model from the sidebar,
loading its list page, then searching within it. On an admin with a few dozen
models that is three steps and two page loads for something the user could name
immediately.
How it works
The palette answers three questions and each has a deliberately different cost.
Model names are matched against the in-memory view registry. No database
access, so this stays instant regardless of how many models are registered.
Records in one model are searched after clicking Search inside on a row,
which pins that model as the scope. Exactly one query runs against exactly one
model, again independent of how many exist.
Records across models are searched when typing without a scope. This is the
only mode that fans out, so it is opt-in per model, capped, and skipped entirely
for terms shorter than
palette_search_min_chars.palette_searchdefaults toFalse. Adding the palette to an existing admintherefore changes nothing about query load until a maintainer opts a model in.
Models that are not opted in remain reachable through Search inside, since
scoping is an explicit user action costing a single query.
Admintakespalette_search_min_chars(default2) andpalette_search_max_models(default8). Queries run concurrently on asyncengines; sync engines go through the existing worker-thread path.
Scoped search
Clicking Search inside pins a model. The chip's × clears it.
Commands
Below the matches, the palette offers commands for the best-matching model.
With an empty box no commands are shown, since there is no match to act on.
ModelView.palette_commandsreturns them and can be overridden:Permissions
The palette exposes nothing the rest of the admin would not:
is_visible/is_accessible404if named as a scopecheck_can_view_detailscan_view_details = Falsecan_create = Falselogin_requiredHow it renders in each configuration
The trigger lives in the top bar. That bar previously only rendered when a
language switcher was configured, so this PR makes the
<header>unconditional and leaves the switcher itself conditional. All four combinations
were checked:
i18n_configauthentication_backendTranslations
Every string is translated, including those rendered by JavaScript as results
come in. 19 new strings were added to the
az,de,ruandtrcatalogs(71/71 for each).
Strings that interpolate a model name are whole sentences with placeholders
rather than concatenated fragments, so word order stays translatable:
Braces rather than
%(name)s: Jinja'sgettexttreats%(...)sas its owninterpolation and raises
KeyErrorwhen the variable is not supplied at rendertime, which is the case here since substitution happens in the browser.