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)