Lots of fixes coming from downstream.

crudkit/dsl.py

@@ -74,6 +74,17 @@ def _related_predicate(Model, path_parts, op_key, value):
     # wrap at this hop using the *attribute*, not the RelationshipProperty
     return attr.any(pred) if rel.uselist else attr.has(pred)
 
+def split_sort_tokens(tokens):
+    simple, dotted = [], []
+    for tok in (tokens or []):
+        if not tok:
+            continue
+        key = tok.lstrip("-")
+        if ":" in key:
+            key = key.split(":", 1)[0]
+        (dotted if "." in key else simple).append(tok)
+    return simple, dotted
+
 def build_query(Model, spec: QuerySpec, eager_policy=None):
     stmt = select(Model)
 
@@ -102,11 +113,25 @@ def build_query(Model, spec: QuerySpec, eager_policy=None):
             continue
         stmt = stmt.where(FILTER_OPS[op_key](col, val) if op_key else (col == val))
 
-    # order_by
-    for key in spec.order_by:
-        desc_ = key.startswith("-")
-        col = getattr(Model, key[1:] if desc_ else key)
-        stmt = stmt.order_by(desc(col) if desc_ else asc(col))
+    simple_sorts, _ = split_sort_tokens(spec.order_by)
+
+    for token in simple_sorts:
+        direction = "asc"
+        key = token
+        if token.startswith("-"):
+            direction = "desc"
+            key = token[1:]
+        if ":" in key:
+            key, d = key.rsplit(":", 1)
+            direction = "desc" if d.lower().startswith("d") else "asc"
+
+        if "." in key:
+            continue
+
+        col = getattr(Model, key, None)
+        if col is None:
+            continue
+        stmt = stmt.order_by(desc(col) if direction == "desc" else asc(col))
 
     if not spec.order_by and spec.page and spec.per_page:
         pk_cols = inspect(Model).primary_key

crudkit/eager.py

@@ -1,8 +1,21 @@
-from typing import List
+from __future__ import annotations
+from typing import Iterable, List, Sequence, Set
 from sqlalchemy.inspection import inspect
-from sqlalchemy.orm import Load, joinedload, selectinload
+from sqlalchemy.orm import Load, joinedload, selectinload, RelationshipProperty
 
-def default_eager_policy(Model, expand: List[str]) -> List[Load]:
+class EagerConfig:
+    def __init__(self, strict: bool = False, max_depth: int = 4):
+        self.strict = strict
+        self.max_depth = max_depth
+
+def _rel(cls, name: str) -> RelationshipProperty | None:
+    return inspect(cls).relationships.get(name)
+
+def _is_expandable(rel: RelationshipProperty) -> bool:
+    # Skip dynamic or viewonly collections; they don’t support eagerload
+    return rel.lazy != "dynamic"
+
+def default_eager_policy(Model, expand: Sequence[str], cfg: EagerConfig | None = None) -> List[Load]:
     """
     Heuristic:
       - many-to-one / one-to-one: joinedload
@@ -12,31 +25,51 @@ def default_eager_policy(Model, expand: List[str]) -> List[Load]:
     if not expand:
         return []
 
+    cfg = cfg or EagerConfig()
+    # normalize, dedupe, and prefer longer paths over their prefixes
+    raw: Set[str] = {p.strip() for p in expand if p and p.strip()}
+    # drop prefixes if a longer path exists (author, author.publisher -> keep only author.publisher)
+    pruned: Set[str] = set(raw)
+    for p in raw:
+        parts = p.split(".")
+        for i in range(1, len(parts)):
+            pruned.discard(".".join(parts[:i]))
+
     opts: List[Load] = []
+    seen: Set[tuple] = set()
 
-    for path in expand:
+    for path in sorted(pruned):
         parts = path.split(".")
+        if len(parts) > cfg.max_depth:
+            if cfg.strict:
+                raise ValueError(f"expand path too deep: {path} (max {cfg.max_depth})")
+            continue
+
         current_model = Model
-        current_inspect = inspect(current_model)
+        # build the chain incrementally
+        loader: Load | None = None
+        ok = True
 
-        # first hop
-        rel = current_inspect.relationships.get(parts[0])
-        if not rel:
-            continue  # silently skip bad names
-        attr = getattr(current_model, parts[0])
-        loader: Load = selectinload(attr) if rel.uselist else joinedload(attr)
-        current_model = rel.mapper.class_
-
-        # nested hops, if any
-        for name in parts[1:]:
-            current_inspect = inspect(current_model)
-            rel = current_inspect.relationships.get(name)
-            if not rel:
+        for i, name in enumerate(parts):
+            rel = _rel(current_model, name)
+            if not rel or not _is_expandable(rel):
+                ok = False
                 break
             attr = getattr(current_model, name)
-            loader = loader.selectinload(attr) if rel.uselist else loader.joinedload(attr)
+            if loader is None:
+                loader = selectinload(attr) if rel.uselist else joinedload(attr)
+            else:
+                loader = loader.selectinload(attr) if rel.uselist else loader.joinedload(attr)
             current_model = rel.mapper.class_
 
-        opts.append(loader)
+        if not ok:
+            if cfg.strict:
+                raise ValueError(f"unknown or non-expandable relationship in expand path: {path}")
+            continue
+
+        key = (tuple(parts),)
+        if loader is not None and key not in seen:
+            opts.append(loader)
+            seen.add(key)
 
     return opts

crudkit/html/templates/crudkit/_macros.html

@@ -32,78 +32,109 @@
 {%- endfor -%}
 {%- endmacro %}
 
+{# helper: centralize the query string once #}
+{% macro _q(model, page, per_page, sort, filters, fields_csv) -%}
+/ui/{{ model }}/frag/rows
+?page={{ page }}&per_page={{ per_page }}
+{%- if sort %}&sort={{ sort }}{% endif -%}
+{%- if fields_csv %}&fields_csv={{ fields_csv|urlencode }}{% endif -%}
+{%- for k, v in (filters or {}).items() %}&{{ k }}={{ v|urlencode }}{% endfor -%}
+{%- endmacro %}
+
 {% macro pager(model, page, pages, per_page, sort, filters, fields_csv) -%}
 {% set p = page|int %}
 {% set pg = pages|int %}
-{% set prev = [1, p-1]|max %}
-{% set nxt = [pg, p+1]|min %}
-<nav id="pager">
-    {% if page > 1 %}
-    <button type="button"
-        hx-get="/ui/{{ model }}/frag/rows?page=1&per_page={{ per_page }}{% if sort %}&sort={{ sort }}{% endif %}{% if fields_csv %}&fields_csv={{ fields_csv|urlencode }}{% endif %}{% for k,v in filters.items() %}&{{k}}={{ v|urlencode }}{% endfor %}"
-        hx-target="#rows" hx-swap="innerHTML">First</button>
-    <button type="button"
-        hx-get="/ui/{{ model }}/frag/rows?page={{ prev }}&per_page={{ per_page }}{% if sort %}&sort={{ sort }}{% endif %}{% if fields_csv %}&fields_csv={{ fields_csv|urlencode }}{% endif %}{% for k,v in (filters or {}).items() %}&{{k}}={{ v|urlencode }}{% endfor %}"
-        hx-target="#rows" hx-swap="innerHTML"
-        hx-on:click="document.querySelector('#pager-state input[name=page]').value='{{ prev }}'">Prev</button>
-    {% endif %}
+{% set prev = 1 if p <= 1 else p - 1 %} {% set nxt=pg if p>= pg else p + 1 %}
 
-    <span>Page {{ page }} / {{ pages }}</span>
+    <nav class="pager-nav" aria-label="Pagination">
+        {% if p > 1 %}
+        <button type="button" class="page-btn" data-page="1"
+            hx-get="{{ _q(model, 1, per_page, sort, filters, fields_csv) }}" hx-target="#rows" hx-swap="innerHTML"
+            aria-label="First page">First</button>
 
-    {% if page < pages %} <button type="button"
-        hx-get="/ui/{{ model }}/frag/rows?page={{ page+1 }}&per_page={{ per_page }}{% if sort %}&sort={{ sort }}{% endif %}{% if fields_csv %}&fields_csv={{ fields_csv|urlencode }}{% endif %}{% for k,v in filters.items() %}&{{k}}={{ v|urlencode }}{% endfor %}"
-        hx-target="#rows" rel="next" hx-swap="innerHTML">Next</button>
-        <button type="button"
-            hx-get="/ui/{{ model }}/frag/rows?page={{ nxt }}&per_page={{ per_page }}{% if sort %}&sort={{ sort }}{% endif %}{% if fields_csv %}&fields_csv={{ fields_csv|urlencode }}{% endif %}{% for k,v in (filters or {}).items() %}&{{k}}={{ v|urlencode }}{% endfor %}"
-            hx-target="#rows" hx-swap="innerHTML"
-            hx-on:click="document.querySelector('#pager-state input[name=page]').value='{{ nxt }}'">Last</button>
+        <button type="button" class="page-btn" data-page="{{ prev }}"
+            hx-get="{{ _q(model, prev, per_page, sort, filters, fields_csv) }}" hx-target="#rows" hx-swap="innerHTML"
+            rel="prev">Prev</button>
+        {% else %}
+        <button type="button" class="page-btn" disabled>First</button>
+        <button type="button" class="page-btn" disabled>Prev</button>
         {% endif %}
-</nav>
-{%- endmacro %}
 
-{% macro form(schema, action, method="POST", obj_id=None, hx=False, csrf_token=None) -%}
-<form action="{{ action }}" method="post" {%- if hx %} hx-{{ "patch" if obj_id else "post" }}="{{ action }}"
-    hx-target="closest dialog, #modal-body, body" hx-swap="innerHTML" hx-disabled-elt="button[type=submit]" {%- endif
-    -%}>
-    {%- if csrf_token %}<input type="hidden" name="csrf_token" value="{{ csrf_token() }}">{% endif -%}
-    {%- if obj_id %}<input type="hidden" name="id" value="{{ obj_id }}">{% endif -%}
-    <input type="hidden" name="fields_csv" value="{{ request.args.get('fields_csv','id,name') }}">
+        <span aria-live="polite">Page {{ p }} / {{ pg }}</span>
 
-    {%- for f in schema -%}
-    <div class="field" data-name="{{ f.name }}">
-        {% set fid = 'f-' ~ f.name ~ '-' ~ (obj_id or 'new') %}
-        <label for="{{ fid }}">{{ f.label or f.name|replace('_',' ')|title }}</label>
-        {%- if f.type == "textarea" -%}
-        <textarea id="{{ fid }}" name="{{ f.name }}" {%- if f.required %} required{% endif %}{% if f.maxlength %}
-            maxlength="{{ f.maxlength }}" {% endif %}>{{ f.value or "" }}</textarea>
-        {%- elif f.type == "select" -%}
-        <select id="{{ fid }}" name="{{ f.name }}" {% if f.required %}required{% endif %}>
-            <option value="">{{ f.placeholder or ("Choose " ~ (f.label or f.name|replace('_',' ')|title)) }}</option>
-            {% if f.multiple %}
-            {% set selected = (f.value or [])|list %}
-            {% for val, lbl in f.choices %}
-            <option value="{{ val }}" {{ 'selected' if val in selected else '' }}>{{ lbl }}</option>
-            {% endfor %}
+        {% if p < pg %} <button type="button" class="page-btn" data-page="{{ p + 1 }}"
+            hx-get="{{ _q(model, p + 1, per_page, sort, filters, fields_csv) }}" hx-target="#rows" hx-swap="innerHTML"
+            rel="next">Next</button>
+
+            <button type="button" class="page-btn" data-page="{{ pg }}"
+                hx-get="{{ _q(model, pg, per_page, sort, filters, fields_csv) }}" hx-target="#rows" hx-swap="innerHTML"
+                aria-label="Last page">Last</button>
             {% else %}
-            {% for val, lbl in f.choices %}
-            <option value="{{ val }}" {{ 'selected' if (f.value|string)==(val|string) else '' }}>{{ lbl }}</option>
-            {% endfor %}
+            <button type="button" class="page-btn" disabled>Next</button>
+            <button type="button" class="page-btn" disabled>Last</button>
             {% endif %}
-        </select>
-        {%- elif f.type == "checkbox" -%}
-        <input type="hidden" name="{{ f.name }}" value="0">
-        <input id="{{ fid }}" type="checkbox" name="{{ f.name }}" value="1" {{ "checked" if f.value else "" }}>
-        {%- else -%}
-        <input id="{{ fid }}" type="{{ f.type }}" name="{{ f.name }}"
-            value="{{ f.value if f.value is not none else '' }}" {%- if f.required %} required{% endif %} {%- if
-            f.maxlength %} maxlength="{{ f.maxlength }}" {% endif %}>
-        {%- endif -%}
-        {%- if f.help %}<div class="help">{{ f.help }}</div>{% endif -%}
-    </div>
-    {%- endfor -%}
+    </nav>
 
-    <div class="actions">
-        <button type="submit">Save</button>
-    </div>
-</form>
-{%- endmacro %}
+    {# one tiny listener to keep #pager-state in sync for every button #}
+    <script>
+        (function () {
+            const nav = document.currentScript.previousElementSibling;
+            nav.addEventListener('click', function (ev) {
+                const btn = ev.target.closest('.page-btn');
+                if (!btn || btn.disabled) return;
+                const page = btn.getAttribute('data-page');
+                if (!page) return;
+                const inp = document.querySelector('#pager-state input[name=page]');
+                if (inp) inp.value = page;
+            }, { capture: true });
+        })();
+    </script>
+    {%- endmacro %}
+
+    {% macro form(schema, action, method="POST", obj_id=None, hx=False, csrf_token=None) -%}
+    <form action="{{ action }}" method="post" {%- if hx %} hx-{{ "patch" if obj_id else "post" }}="{{ action }}"
+        hx-target="closest dialog, #modal-body, body" hx-swap="innerHTML" hx-disabled-elt="button[type=submit]" {%-
+        endif -%}>
+        {%- if csrf_token %}<input type="hidden" name="csrf_token" value="{{ csrf_token() }}">{% endif -%}
+        {%- if obj_id %}<input type="hidden" name="id" value="{{ obj_id }}">{% endif -%}
+        <input type="hidden" name="fields_csv" value="{{ request.args.get('fields_csv','id,name') }}">
+
+        {%- for f in schema -%}
+        <div class="field" data-name="{{ f.name }}">
+            {% set fid = 'f-' ~ f.name ~ '-' ~ (obj_id or 'new') %}
+            <label for="{{ fid }}">{{ f.label or f.name|replace('_',' ')|title }}</label>
+            {%- if f.type == "textarea" -%}
+            <textarea id="{{ fid }}" name="{{ f.name }}" {%- if f.required %} required{% endif %}{% if f.maxlength %}
+                maxlength="{{ f.maxlength }}" {% endif %}>{{ f.value or "" }}</textarea>
+            {%- elif f.type == "select" -%}
+            <select id="{{ fid }}" name="{{ f.name }}" {% if f.required %}required{% endif %}>
+                <option value="">{{ f.placeholder or ("Choose " ~ (f.label or f.name|replace('_',' ')|title)) }}
+                </option>
+                {% if f.multiple %}
+                {% set selected = (f.value or [])|list %}
+                {% for val, lbl in f.choices %}
+                <option value="{{ val }}" {{ 'selected' if val in selected else '' }}>{{ lbl }}</option>
+                {% endfor %}
+                {% else %}
+                {% for val, lbl in f.choices %}
+                <option value="{{ val }}" {{ 'selected' if (f.value|string)==(val|string) else '' }}>{{ lbl }}</option>
+                {% endfor %}
+                {% endif %}
+            </select>
+            {%- elif f.type == "checkbox" -%}
+            <input type="hidden" name="{{ f.name }}" value="0">
+            <input id="{{ fid }}" type="checkbox" name="{{ f.name }}" value="1" {{ "checked" if f.value else "" }}>
+            {%- else -%}
+            <input id="{{ fid }}" type="{{ f.type }}" name="{{ f.name }}"
+                value="{{ f.value if f.value is not none else '' }}" {%- if f.required %} required{% endif %} {%- if
+                f.maxlength %} maxlength="{{ f.maxlength }}" {% endif %}>
+            {%- endif -%}
+            {%- if f.help %}<div class="help">{{ f.help }}</div>{% endif -%}
+        </div>
+        {%- endfor -%}
+
+        <div class="actions">
+            <button type="submit">Save</button>
+        </div>
+    </form>
+    {%- endmacro %}

crudkit/html/ui_fragments.py

@@ -5,6 +5,7 @@ from flask import Blueprint, request, render_template, abort, make_response
 from sqlalchemy import select
 from sqlalchemy.orm import scoped_session
 from sqlalchemy.inspection import inspect
+from sqlalchemy.sql.elements import UnaryExpression
 from sqlalchemy.sql.sqltypes import Integer, Boolean, Date, DateTime, Float, Numeric
 
 from ..dsl import QuerySpec
@@ -115,12 +116,12 @@ def make_fragments_blueprint(db_session_factory, registry: Dict[str, Any], *, na
         page = request.args.get("page", type=int) or 1
         per_page = request.args.get("per_page", type=int) or 20
 
-        expand = _collect_expand_from_paths(fields)
+        expand = _collect_expand_from_paths(fields + ([sort.split(":")[0]] if sort else []))
         spec = QuerySpec(filters=filters, order_by=[sort] if sort else [], page=page, per_page=per_page, expand=expand)
         s = session(); svc = CrudService(s, default_eager_policy)
         rows, _ = svc.list(Model, spec)
 
-        html = render_template("crudkit/rows.html", items=rows, fields=fields, getp=_getp)
+        html = render_template("crudkit/rows.html", items=rows, fields=fields, getp=_getp, model=model)
 
         return html
 
@@ -134,7 +135,7 @@ def make_fragments_blueprint(db_session_factory, registry: Dict[str, Any], *, na
         sort = request.args.get("sort")
         fields_csv = request.args.get("fields_csv") or "id,name"
         fields = _paths_from_csv(fields_csv)
-        expand = _collect_expand_from_paths(fields)
+        expand = _collect_expand_from_paths(fields + ([sort.split(":")[0]] if sort else []))
 
         spec = QuerySpec(filters=filters, order_by=[sort] if sort else [], page=page, per_page=per_page, expand=expand)
         s = session(); svc = CrudService(s, default_eager_policy)

crudkit/service.py

@@ -1,10 +1,93 @@
-from sqlalchemy import func
+import sqlalchemy as sa
+from sqlalchemy import func, asc
 from sqlalchemy.exc import IntegrityError
-from sqlalchemy.orm import Session
+from sqlalchemy.orm import Session, aliased
+from sqlalchemy.inspection import inspect
+from sqlalchemy.sql.elements import UnaryExpression
 
-from .dsl import QuerySpec, build_query
+from .dsl import QuerySpec, build_query, split_sort_tokens
 from .eager import default_eager_policy
 
+def _dedup_order_by(ordering):
+    seen = set()
+    result = []
+    for ob in ordering:
+        col = ob.element if isinstance(ob, UnaryExpression) else ob
+        key = f"{col}-{getattr(ob, 'modifier', '')}-{getattr(ob, 'operator', '')}"
+        if key in seen:
+            continue
+        seen.add(key)
+        result.append(ob)
+    return result
+
+def _parse_sort_token(token: str):
+    token = token.strip()
+    direction = "asc"
+    if token.startswith('-'):
+        direction = "desc"
+        token = token[1:]
+    if ":" in token:
+        key, dirpart = token.rsplit(":", 1)
+        direction = "desc" if dirpart.lower().startswith("d") else "asc"
+        return key, direction
+    return token, direction
+
+def _apply_dotted_ordering(stmt, Model, sort_tokens):
+    """
+    stmt: a select(Model) statement
+    sort_tokens: list[str] like ["owner.identifier", "-brand.name"]
+    Returns: (stmt, alias_cache)
+    """
+    mapper = inspect(Model)
+    alias_cache = {}  # maps a path like "owner" or "brand" to its alias
+
+    for tok in sort_tokens:
+        path, direction = _parse_sort_token(tok)
+        parts = [p for p in path.split(".") if p]
+        if not parts:
+            continue
+
+        entity = Model
+        current_mapper = mapper
+        alias_path = []
+
+        # Walk relationships for all but the last part
+        for rel_name in parts[:-1]:
+            rel = current_mapper.relationships.get(rel_name)
+            if rel is None:
+                # invalid sort key; skip quietly or raise
+                # raise ValueError(f"Unknown relationship {current_mapper.class_.__name__}.{rel_name}")
+                entity = None
+                break
+
+            alias_path.append(rel_name)
+            key = ".".join(alias_path)
+
+            if key in alias_cache:
+                entity_alias = alias_cache[key]
+            else:
+                # build an alias and join
+                entity_alias = aliased(rel.mapper.class_)
+                stmt = stmt.outerjoin(entity_alias, getattr(entity, rel.key))
+                alias_cache[key] = entity_alias
+
+            entity = entity_alias
+            current_mapper = inspect(rel.mapper.class_)
+
+        if entity is None:
+            continue
+
+        col_name = parts[-1]
+        # Validate final column
+        if col_name not in current_mapper.columns:
+            # raise ValueError(f"Unknown column {current_mapper.class_.__name__}.{col_name}")
+            continue
+
+        col = getattr(entity, col_name) if entity is not Model else getattr(Model, col_name)
+        stmt = stmt.order_by(col.desc() if direction == "desc" else col.asc())
+
+    return stmt
+
 class CrudService:
     def __init__(self, session: Session, eager_policy=default_eager_policy):
         self.s = session
@@ -25,10 +108,44 @@ class CrudService:
 
     def list(self, Model, spec: QuerySpec):
         stmt = build_query(Model, spec, self.eager_policy)
-        count_stmt = stmt.with_only_columns(func.count()).order_by(None)
-        total = self.s.execute(count_stmt).scalar_one()
+
+        simple_sorts, dotted_sorts = split_sort_tokens(spec.order_by)
+        if dotted_sorts:
+            stmt = _apply_dotted_ordering(stmt, Model, dotted_sorts)
+
+        # count query
+        pk = getattr(Model, "id")  # adjust if not 'id'
+        count_base = stmt.with_only_columns(sa.distinct(pk)).order_by(None)
+        total = self.s.execute(
+            sa.select(sa.func.count()).select_from(count_base.subquery())
+        ).scalar_one()
+
         if spec.page and spec.per_page:
-            stmt = stmt.limit(spec.per_page).offset((spec.page - 1) * spec.per_page)
+            offset = (spec.page - 1) * spec.per_page
+            stmt = stmt.limit(spec.per_page).offset(offset)
+
+        # ---- ORDER BY handling ----
+        mapper = inspect(Model)
+        pk_cols = mapper.primary_key
+
+        # Gather all clauses added so far
+        ordering = list(stmt._order_by_clauses)
+
+        # Append pk tie-breakers if not already present
+        existing_cols = {
+            str(ob.element if isinstance(ob, UnaryExpression) else ob)
+            for ob in ordering
+        }
+        for c in pk_cols:
+            if str(c) not in existing_cols:
+                ordering.append(asc(c))
+
+        # Dedup *before* applying
+        ordering = _dedup_order_by(ordering)
+
+        # Now wipe old order_bys and set once
+        stmt = stmt.order_by(None).order_by(*ordering)
+
         rows = self.s.execute(stmt).scalars().all()
         return rows, total