More fixes and additions for forms. We are also haunted by detached sessions constantly.

crudkit/backend.py

@@ -74,18 +74,32 @@ def apply_pagination(sel: Select, backend: BackendInfo, *, page: int, per_page:
     per_page = max(1, int(per_page))
     offset = (page - 1) * per_page
 
-    if backend.requires_order_by_for_offset and not sel._order_by_clauses:
-        if default_order_by is None:
-            sel = sel.order_by(text("1"))
-        else:
-            sel = sel.order_by(default_order_by)
+    if backend.requires_order_by_for_offset:
+        # Avoid private attribute if possible:
+        has_order = bool(getattr(sel, "_order_by_clauses", ()))  # fallback for SA < 2.0.30
+        try:
+            has_order = has_order or bool(sel.get_order_by())
+        except Exception:
+            pass
+
+        if not has_order:
+            if default_order_by is not None:
+                sel = sel.order_by(default_order_by)
+            else:
+                # Try to find a primary key from the FROMs; fall back to a harmless literal.
+                try:
+                    first_from = sel.get_final_froms()[0]
+                    pk = next(iter(first_from.primary_key.columns))
+                    sel = sel.order_by(pk)
+                except Exception:
+                    sel = sel.order_by(text("1"))
 
     return sel.limit(per_page).offset(offset)
 
 @contextmanager
 def maybe_identify_insert(session: Session, table, backend: BackendInfo):
     """
-    For MSSQL tables with IDENTIFY PK when you need to insert explicit IDs.
+    For MSSQL tables with IDENTITY PK when you need to insert explicit IDs.
     No-op elsewhere.
     """
     if not backend.is_mssql:
@@ -93,7 +107,7 @@ def maybe_identify_insert(session: Session, table, backend: BackendInfo):
         return
 
     full_name = f"{table.schema}.{table.name}" if table.schema else table.name
-    session.execute(text(f"SET IDENTIFY_INSERT {full_name} ON"))
+    session.execute(text(f"SET IDENTITY_INSERT {full_name} ON"))
     try:
         yield
     finally:
@@ -101,7 +115,7 @@ def maybe_identify_insert(session: Session, table, backend: BackendInfo):
 
 def chunked_in(column, values: Iterable, backend: BackendInfo, chunk_size: Optional[int] = None) -> ClauseElement:
     """
-    Build a safe large IN() filter respecting bund param limits.
+    Build a safe large IN() filter respecting bind param limits.
     Returns a disjunction of chunked IN clauses if needed.
     """
     vals = list(values)
@@ -120,3 +134,12 @@ def chunked_in(column, values: Iterable, backend: BackendInfo, chunk_size: Optio
     for p in parts[1:]:
         expr = expr | p
     return expr
+
+def sql_trim(expr, backend: BackendInfo):
+    """
+    Portable TRIM. SQL Server before compat level 140 lacks TRIM().
+    Emit LTRIM(RTRIM(...)) there; use TRIM elsewhere
+    """
+    if backend.is_mssql:
+        return func.ltrim(func.rtrim(expr))
+    return func.trim(expr)

crudkit/core/service.py

@@ -1,7 +1,7 @@
 from typing import Any, Callable, Dict, Iterable, List, Tuple, Type, TypeVar, Generic, Optional, Protocol, runtime_checkable, cast
 from sqlalchemy import and_, func, inspect, or_, text
 from sqlalchemy.engine import Engine, Connection
-from sqlalchemy.orm import Load, Session, raiseload, selectinload, with_polymorphic, Mapper, RelationshipProperty, class_mapper
+from sqlalchemy.orm import Load, Session, raiseload, selectinload, with_polymorphic, Mapper, RelationshipProperty, class_mapper, ColumnProperty
 from sqlalchemy.orm.attributes import InstrumentedAttribute
 from sqlalchemy.orm.util import AliasedClass
 from sqlalchemy.sql import operators
@@ -12,6 +12,19 @@ from crudkit.core.spec import CRUDSpec
 from crudkit.core.types import OrderSpec, SeekWindow
 from crudkit.backend import BackendInfo, make_backend_info
 
+def _is_rel(model_cls, name: str) -> bool:
+    try:
+        prop = model_cls.__mapper__.relationships.get(name)
+        return isinstance(prop, RelationshipProperty)
+    except Exception:
+        return False
+
+def _is_instrumented_column(attr) -> bool:
+    try:
+        return hasattr(attr, "property") and isinstance(attr.property, ColumnProperty)
+    except Exception:
+        return False
+
 def _loader_options_for_fields(root_alias, model_cls, fields: list[str]) -> list[Load]:
     """
     For bare MANYTOONE names in fields (e.g. "location"), selectinload the relationship
@@ -103,43 +116,47 @@ class CRUDService(Generic[T]):
             => selectinload(root.location).selectinload(Room.room_function)
         """
         opts: List[Any] = []
-
         root_mapper: Mapper[Any] = cast(Mapper[Any], inspect(self.model))
 
         for path, names in (rel_field_names or {}).items():
             if not path:
                 continue
 
-            current_alias = root_alias
             current_mapper = root_mapper
             rel_props: List[RelationshipProperty] = []
 
             valid = True
             for step in path:
                 rel = current_mapper.relationships.get(step)
-                if rel is None:
+                if not isinstance(rel, RelationshipProperty):
                     valid = False
                     break
                 rel_props.append(rel)
                 current_mapper = cast(Mapper[Any], inspect(rel.entity.entity))
-            if not valid:
+            if not valid or not rel_props:
                 continue
 
-            target_cls = current_mapper.class_
+            first = rel_props[0]
+            base_loader = selectinload(getattr(root_alias, first.key))
+            for i in range(1, len(rel_props)):
+                prev_target_cls = rel_props[i - 1].mapper.class_
+                hop_attr = getattr(prev_target_cls, rel_props[i].key)
+                base_loader = base_loader.selectinload(hop_attr)
+
+            target_cls = rel_props[-1].mapper.class_
 
             requires = getattr(target_cls, "__crudkit_field_requires__", None)
             if not isinstance(requires, dict):
                 continue
 
             for field_name in names:
-                needed: Iterable[str] = requires.get(field_name, [])
+                needed: Iterable[str] = requires.get(field_name, []) or []
                 for rel_need in needed:
-                    loader = selectinload(getattr(root_alias, rel_props[0].key))
-                    for rp in rel_props[1:]:
-                        loader = loader.selectinload(getattr(getattr(root_alias, rp.parent.class_.__name__.lower(), None) or rp.parent.class_, rp.key))
-
-                    loader = loader.selectinload(getattr(target_cls, rel_need))
-                    opts.append(loader)
+                    rel_prop2 = target_cls.__mapper__.relationships.get(rel_need)
+                    if not isinstance(rel_prop2, RelationshipProperty):
+                        continue
+                    dep_attr = getattr(target_cls, rel_prop2.key)
+                    opts.append(base_loader.selectinload(dep_attr))
 
         return opts
 
@@ -225,12 +242,18 @@ class CRUDService(Generic[T]):
 
         root_fields, rel_field_names, root_field_names = spec.parse_fields()
 
+        seen_rel_roots = set()
         for path, names in (rel_field_names or {}).items():
-            if "label" in names:
-                rel_name = path[0]
+            if not path:
+                continue
+            rel_name = path[0]
+            if rel_name in seen_rel_roots:
+                continue
+            if _is_rel(self.model, rel_name):
                 rel_attr = getattr(root_alias, rel_name, None)
                 if rel_attr is not None:
                     query = query.options(selectinload(rel_attr))
+                    seen_rel_roots.add(rel_name)
 
         # Soft delete filter
         if self.supports_soft_delete and not _is_truthy(params.get("include_deleted")):
@@ -251,8 +274,8 @@ class CRUDService(Generic[T]):
         only_cols = [c for c in root_fields if isinstance(c, InstrumentedAttribute)]
         if only_cols:
             query = query.options(Load(root_alias).load_only(*only_cols))
-        for eager in spec.get_eager_loads(root_alias, fields_map=rel_field_names):
-            query = query.options(eager)
+        # for eager in spec.get_eager_loads(root_alias, fields_map=rel_field_names):
+        #     query = query.options(eager)
 
         for opt in self._resolve_required_includes(root_alias, rel_field_names):
             query = query.options(opt)
@@ -387,6 +410,20 @@ class CRUDService(Generic[T]):
         if params:
             root_fields, rel_field_names, root_field_names = spec.parse_fields()
 
+        if rel_field_names:
+            seen_rel_roots = set()
+            for path, names in rel_field_names.items():
+                if not path:
+                    continue
+                rel_name = path[0]
+                if rel_name in seen_rel_roots:
+                    continue
+                if _is_rel(self.model, rel_name):
+                    rel_attr = getattr(root_alias, rel_name, None)
+                    if rel_attr is not None:
+                        query = query.options(selectinload(rel_attr))
+                        seen_rel_roots.add(rel_name)
+
         fields = (params or {}).get("fields") if isinstance(params, dict) else None
         if fields:
             for opt in _loader_options_for_fields(root_alias, self.model, fields):
@@ -396,8 +433,8 @@ class CRUDService(Generic[T]):
         if only_cols:
             query = query.options(Load(root_alias).load_only(*only_cols))
 
-        for eager in spec.get_eager_loads(root_alias, fields_map=rel_field_names):
-            query = query.options(eager)
+        # for eager in spec.get_eager_loads(root_alias, fields_map=rel_field_names):
+        #     query = query.options(eager)
 
         if params:
             fields = params.get("fields") or []
@@ -454,12 +491,26 @@ class CRUDService(Generic[T]):
             if params:
                 root_fields, rel_field_names, root_field_names = spec.parse_fields()
 
+            if rel_field_names:
+                seen_rel_roots = set()
+                for path, names in rel_field_names.items():
+                    if not path:
+                        continue
+                    rel_name = path[0]
+                    if rel_name in seen_rel_roots:
+                        continue
+                    if _is_rel(self.model, rel_name):
+                        rel_attr = getattr(root_alias, rel_name, None)
+                        if rel_attr is not None:
+                            query = query.options(selectinload(rel_attr))
+                            seen_rel_roots.add(rel_name)
+
             only_cols = [c for c in root_fields if isinstance(c, InstrumentedAttribute)]
             if only_cols:
                 query = query.options(Load(root_alias).load_only(*only_cols))
 
-            for eager in spec.get_eager_loads(root_alias, fields_map=rel_field_names):
-                query = query.options(eager)
+            # for eager in spec.get_eager_loads(root_alias, fields_map=rel_field_names):
+            #     query = query.options(eager)
 
             if params:
                 fields = params.get("fields") or []

crudkit/ui/fragments.py

@@ -54,15 +54,30 @@ def _get_loaded_attr(obj: Any, name: str) -> Any:
     """
     try:
         st = inspect(obj)
+        # 1) Mapped attribute?
         attr = st.attrs.get(name)
         if attr is not None:
             val = attr.loaded_value
             return None if val is NO_VALUE else val
+        # 2) Already present value (e.g., eager-loaded or set on the dict)?
         if name in st.dict:
             return st.dict.get(name)
-        return getattr(obj, name, None)
+        # 3) If object is detached or attr is not mapped, DO NOT eval hybrids
+        #    or descriptors that could lazy-load. That would explode.
+        if st.session is None:
+            return None
+        # 4) As a last resort on attached instances only, try simple getattr,
+        #    but guard against DetachedInstanceError anyway.
+        try:
+            return getattr(obj, name, None)
+        except Exception:
+            return None
     except Exception:
-        return getattr(obj, name, None)
+        # If we can't even inspect it, be conservative
+        try:
+            return getattr(obj, name, None)
+        except Exception:
+            return None
 
 def _normalize_rows_layout(layout: Optional[List[dict]]) -> Dict[str, dict]:
     """
@@ -293,7 +308,21 @@ def _value_label_for_field(field: dict, mapper, values_map: dict, instance, sess
             rel_obj = q.get(rid)
 
     if rel_obj is not None:
-        return _label_from_obj(rel_obj, label_spec)
+        try:
+            s = _label_from_obj(rel_obj, label_spec)
+        except Exception:
+            s = None
+        # If we couldn't safely render and we have a session+id, do one lean retry.
+        if (s is None or s == "") and session is not None and rid is not None:
+            mdl = rel_prop.mapper.class_
+            try:
+                rel_obj2 = session.get(mdl, rid) # attached instance
+                s2 = _label_from_obj(rel_obj2, label_spec)
+                if s2:
+                    return s2
+            except Exception:
+                pass
+        return s
     return str(rid) if rid is not None else None
 
 class _SafeObj:
@@ -406,7 +435,7 @@ def _fk_options(session, related_model, label_spec):
     if simple_cols:
         first = simple_cols[0]
         if hasattr(related_model, first):
-            q = q.order_by(getattr(related_model, first))
+            q = q.order_by(None).order_by(getattr(related_model, first))
 
     rows = q.all()
     return [
@@ -559,7 +588,10 @@ def _label_from_obj(obj: Any, spec: Any) -> str:
         for f in fields:
             root = f.split(".", 1)[0]
             if root not in data:
-                data[root] = _SafeObj(_get_loaded_attr(obj, root))
+                try:
+                    data[root] = _SafeObj(_get_loaded_attr(obj, root))
+                except Exception:
+                    data[root] = _SafeObj(None)
         try:
             return spec.format(**data)
         except Exception:
@@ -1007,11 +1039,6 @@ def render_form(
                 base.update(f.get("template_ctx") or {})
                 f["template_ctx"] = base
 
-    for f in fields:
-        vl = _value_label_for_field(f, mapper, values_map, instance, session)
-        if vl is not None:
-            f["value_label"] = vl
-
     for f in fields:
         # existing FK label resolution
         vl = _value_label_for_field(f, mapper, values_map, instance, session)

inventory/models/user.py

@@ -1,10 +1,15 @@
 from typing import List, Optional, TYPE_CHECKING
 
+from flask import current_app
+
 from sqlalchemy import Boolean, Integer, ForeignKey, Unicode, case, func, literal
 from sqlalchemy.ext.hybrid import hybrid_property
 from sqlalchemy.orm import Mapped, mapped_column, relationship
 from sqlalchemy.sql import expression as sql
 
+import crudkit
+
+from crudkit.backend import make_backend_info, sql_trim
 from crudkit.core.base import Base, CRUDMixin
 
 if TYPE_CHECKING:
@@ -12,6 +17,10 @@ if TYPE_CHECKING:
 
 class User(Base, CRUDMixin):
     __tablename__ = 'users'
+    __crud_label__ = "{label}"
+    __crudkit_field_requires__ = {
+        "label": ["first_name", "last_name", "title"]  # whatever the hybrid touches
+    }
 
     first_name: Mapped[Optional[str]] = mapped_column(Unicode(255), nullable=True, index=True)
     last_name: Mapped[Optional[str]] = mapped_column(Unicode(255), nullable=True, index=True)
@@ -48,17 +57,19 @@ class User(Base, CRUDMixin):
 
     @label.expression
     def label(cls):
+        engine = current_app.extensions["crudkit"]["runtime"].engine
+        backend = make_backend_info(engine)
         first = func.coalesce(cls.first_name, "")
         last  = func.coalesce(cls.last_name, "")
         title = func.coalesce(cls.title, "")
 
-        have_first = func.length(func.trim(first)) > 0
-        have_last  = func.length(func.trim(last)) > 0
+        have_first = func.length(sql_trim(first, backend)) > 0
+        have_last  = func.length(sql_trim(last, backend)) > 0
 
         space = case((have_first & have_last, literal(" ")), else_=literal(""))
 
         title_part = case(
-            (func.length(func.trim(title)) > 0, func.concat(" (", title, ")")),
+            (func.length(sql_trim(title, backend)) > 0, func.concat(" (", title, ")")),
             else_=literal("")
         )
 

inventory/routes/entry.py

@@ -42,7 +42,7 @@ def init_entry_routes(app):
 
                 {"name": "owner", "row": "status", "label": "Contact", "wrap": {"class": "col"},
                  "attrs": {"class": "form-control"}, "label_attrs": {"class": "form-label"},
-                 "label_spec": "{first_name} {last_name}"},
+                 "label_spec": "{label}"},
                 {"name": "location", "row": "status", "label": "Location", "wrap": {"class": "col"},
                  "attrs": {"class": "form-control"}, "label_attrs": {"class": "form-label"},
                  "label_spec": "{name} - {room_function.description}"},
@@ -50,10 +50,11 @@ def init_entry_routes(app):
                  "attrs": {"class": "form-control"}, "label_attrs": {"class": "form-label"}},
 
                 {"name": "image", "label": "", "row": "image", "type": "template", "label_spec": "{filename}",
-                 "template": "image_display.html", "attrs": {"class": "img-fluid img-thumbnail"}},
+                 "template": "image_display.html", "attrs": {"class": "img-fluid img-thumbnail h-auto"},
+                 "wrap": {"class": "h-100 w-100"}},
 
                 {"name": "notes", "type": "textarea", "label": "Notes", "row": "notes", "wrap": {"class": "col"},
-                 "attrs": {"class": "form-control", "rows": 10}},
+                 "attrs": {"class": "form-control", "rows": 10}, "label_attrs": {"class": "form-label"}},
             ]
             layout = [
                 {"name": "label", "order": 5},
@@ -84,7 +85,7 @@ def init_entry_routes(app):
                  "row": "name", "wrap": {"class": "col-3"}},
 
                 {"name": "supervisor", "label": "Supervisor", "label_attrs": {"class": "form-label"},
-                 "label_spec": "{first_name} {last_name}", "row": "details", "wrap": {"class": "col-3"},
+                 "label_spec": "{label}", "row": "details", "wrap": {"class": "col-3"},
                  "attrs": {"class": "form-control"}},
 
                 {"name": "location", "label": "Room", "label_attrs": {"class": "form-label"},
@@ -110,7 +111,7 @@ def init_entry_routes(app):
                  "attrs": {"class": "display-6 mb-3"}, "row": "label"},
 
                 {"name": "contact", "row": "ownership", "wrap": {"class": "col"}, "label": "Contact",
-                 "label_spec": "{first_name} {last_name}", "attrs": {"class": "form-control"},
+                 "label_spec": "{label}", "attrs": {"class": "form-control"},
                  "label_attrs": {"class": "form-label"}},
                 {"name": "work_item", "row": "ownership", "wrap": {"class": "col"}, "label": "Work Item",
                  "label_spec": "{label}", "attrs": {"class": "form-control"}, "label_attrs": {"class": "form-label"}},

inventory/static/images/noimage.svg

@@ -0,0 +1 @@
+<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24"><path fill="currentColor" d="M21 5v11q0 .5-.312.75T20 17t-.687-.262t-.313-.763V5H8q-.5 0-.75-.312T7 4t.25-.687T8 3h11q.825 0 1.413.588T21 5M5 21q-.825 0-1.412-.587T3 19V5.8l-.9-.9q-.275-.275-.275-.7t.275-.7t.7-.275t.7.275l17 17q.275.275.275.7t-.275.7t-.7.275t-.7-.275l-.9-.9zm9.175-4H7q-.3 0-.45-.275t.05-.525l2-2.675q.15-.2.4-.2t.4.2L11.25 16l.825-1.1L5 7.825V19h11.175zM10.6 13.4"/></svg>

inventory/templates/image_display.html

@@ -1,4 +1,6 @@
 {% if value %}
 <img src="{{ url_for('static', filename=field['value_label']) }}" alt="{{ value }}" {% if field['attrs'] %}{% for k,v in
     field['attrs'].items() %} {{k}}{% if v is not sameas true %}="{{ v }}" {% endif %} {% endfor %}{% endif %}>
+{% else %}
+<img src="{{ url_for('static', filename='images/noimage.svg') }}" class="img-fluid img-thumbnail h-100">
 {% endif %}