diff --git a/crudkit/backend.py b/crudkit/backend.py index 3232b71..2da68f8 100644 --- a/crudkit/backend.py +++ b/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) diff --git a/crudkit/core/service.py b/crudkit/core/service.py index 1220e04..c34bc2d 100644 --- a/crudkit/core/service.py +++ b/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 [] diff --git a/crudkit/ui/fragments.py b/crudkit/ui/fragments.py index 6da9be8..a26bf0b 100644 --- a/crudkit/ui/fragments.py +++ b/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) diff --git a/inventory/models/user.py b/inventory/models/user.py index 1447ef1..241cb3d 100644 --- a/inventory/models/user.py +++ b/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("") ) diff --git a/inventory/routes/entry.py b/inventory/routes/entry.py index 5f57179..dba0426 100644 --- a/inventory/routes/entry.py +++ b/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,8 +50,8 @@ 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 h-100"}, - "wrap": {"class": "h-100"}}, + "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}, "label_attrs": {"class": "form-label"}}, @@ -85,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"}, @@ -111,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"}}, diff --git a/inventory/templates/image_display.html b/inventory/templates/image_display.html index fbc167a..ca32078 100644 --- a/inventory/templates/image_display.html +++ b/inventory/templates/image_display.html @@ -2,6 +2,5 @@ {{ value }} {% else %} - + {% endif %} \ No newline at end of file