From dd863dba991c90a3df20bd12a613996086960889 Mon Sep 17 00:00:00 2001 From: Yaro Kasear Date: Tue, 21 Oct 2025 08:54:15 -0500 Subject: [PATCH] More attempts to fix a bug. --- crudkit/core/base.py | 46 ++++++++++++++++--------- crudkit/core/meta.py | 2 +- crudkit/core/service.py | 75 ++++++++++++++++++++++++++++++++++++++--- 3 files changed, 102 insertions(+), 21 deletions(-) diff --git a/crudkit/core/base.py b/crudkit/core/base.py index 43a605d..759a42c 100644 --- a/crudkit/core/base.py +++ b/crudkit/core/base.py @@ -145,16 +145,18 @@ def _split_field_tokens(fields: Iterable[str]) -> Tuple[List[str], Dict[str, Lis def _deep_get_loaded(obj: Any, dotted: str) -> Any: """ Deep get with no lazy loads: - - Walk intermediate hops via _safe_get_loaded_attr only. - - Final hop: prefer _safe_get_loaded_attr; if that returns an ORM object or a collection of ORM objects, - serialize to simple dicts; else return the plain value. + - Intermediate hops via _safe_get_loaded_attr only. + - Final hop: + * If relationship and not loaded: return None + * else allow getattr for non-relationship attrs (hybrids/properties) that compute from already-loaded data. + * Serialize ORM objects at the lead into dicts """ parts = dotted.split(".") if not parts: return None cur = obj - # Traverse up to parent of last token without lazy-loads + # Traverse up to the parent of the last token safely for part in parts[:-1]: if cur is None: return None @@ -163,24 +165,36 @@ def _deep_get_loaded(obj: Any, dotted: str) -> Any: return None last = parts[-1] + + # If last is an ORM relationship, only return it if already loaded. + m = _sa_mapper(cur) + if m is not None and last in m.relationships: + val = _safe_get_loaded_attr(cur, last) + if val is None: + return None + # serialize relationship value + if _sa_mapper(val) is not None: + return _serialize_simple_obj(val) + if isinstance(val, (list, tuple)): + out = [] + for v in val: + out.append(_serialize_simple_obj(v) if _sa_mapper(v) is not None else v) + return out + return val + + # Not a relationship: try loaded value, else safe getattr val = _safe_get_loaded_attr(cur, last) if val is None: - # Do NOT lazy load. If it isn't loaded, treat as absent. - return None + try: + # getattr here will not lazy-load relationships because we already gated those + val = getattr(cur, last, None) + except Exception: + val = None - # If the final hop is an ORM object or collection, serialize it if _sa_mapper(val) is not None: return _serialize_simple_obj(val) if isinstance(val, (list, tuple)): - out = [] - for v in val: - if _sa_mapper(v) is not None: - out.append(_serialize_simple_obj(v)) - else: - out.append(v) - return out - - # Plain scalar/computed value + return [_serialize_simple_obj(v) if _sa_mapper(v) is not None else v for v in val] return val def _serialize_leaf(obj: Any) -> Any: diff --git a/crudkit/core/meta.py b/crudkit/core/meta.py index f6dabbc..050d175 100644 --- a/crudkit/core/meta.py +++ b/crudkit/core/meta.py @@ -23,4 +23,4 @@ def rel_map(model: type) -> Dict[str, RelInfo]: return mapper_info(model)["rels"] def column_names_for_model(model: type) -> Tuple[str, ...]: - return mapper_info(model)["col"] + return mapper_info(model)["cols"] diff --git a/crudkit/core/service.py b/crudkit/core/service.py index 6fce1a4..cc33959 100644 --- a/crudkit/core/service.py +++ b/crudkit/core/service.py @@ -12,7 +12,7 @@ from sqlalchemy.sql.elements import UnaryExpression, ColumnElement from crudkit.core import to_jsonable, deep_diff, diff_to_patch, filter_to_columns, normalize_payload from crudkit.core.base import Version -from crudkit.core.meta import rel_map +from crudkit.core.meta import rel_map, column_names_for_model from crudkit.core.params import is_truthy, normalize_fields_param from crudkit.core.spec import CRUDSpec, CollPred from crudkit.core.types import OrderSpec, SeekWindow @@ -281,7 +281,10 @@ class CRUDService(Generic[T]): def _stable_order_by(self, root_alias, given_order_by): order_by = list(given_order_by or []) if not order_by: + # Safe default: primary key(s) only. No unlabeled expressions. return _dedupe_order_by(self._default_order_by(root_alias)) + + # Dedupe what the user gave us, then ensure PK tie-breakers exist order_by = _dedupe_order_by(order_by) mapper: Mapper[Any] = cast(Mapper[Any], inspect(self.model)) present = {_order_identity(_unwrap_ob(ob)[0]) for ob in order_by} @@ -355,7 +358,16 @@ class CRUDService(Generic[T]): join_paths = tuple(spec.get_join_paths()) filter_tables = _collect_tables_from_filters(filters) fkeys = set() - _, proj_opts = compile_projection(self.model, req_fields) if req_fields else ([], []) + # Build projection opts only if there are true scalar columns requested. + # Bare relationship fields like "owner" should not force root column pruning. + column_names = set(column_names_for_model(self.model)) + has_scalar_column_tokens = any( + (("." not in f) and (f in column_names)) + for f in (req_fields or []) + ) + _, proj_opts = (compile_projection(self.model, req_fields) + if (req_fields and has_scalar_column_tokens) + else ([], [])) # filter_tables = () # fkeys = set() @@ -374,8 +386,25 @@ class CRUDService(Generic[T]): def _apply_firsthop_strategies(self, query, root_alias, plan: _Plan): joined_rel_keys: set[str] = set() - rels = rel_map(self.model) + rels = rel_map(self.model) # {name: RelInfo} + # Eager join to-one relationships requested as bare fields (e.g., fields=owner) + requested_scalars = set(plan.root_field_names or []) + for key in requested_scalars: + info = rels.get(key) + if info and not info.uselist and key not in joined_rel_keys: + query = query.join(getattr(root_alias, key), isouter=True) + joined_rel_keys.add(key) + + # 1) Join to-one relationships explicitly requested as bare fields + requested_scalars = set(plan.root_field_names or []) # names like "owner", "supervisor" + for key in requested_scalars: + info = rels.get(key) + if info and not info.uselist and key not in joined_rel_keys: + query = query.join(getattr(root_alias, key), isouter=True) + joined_rel_keys.add(key) + + # 2) Join to-one relationships from parsed join_paths for base_alias, rel_attr, target_alias in plan.join_paths: if base_alias is not root_alias: continue @@ -384,6 +413,7 @@ class CRUDService(Generic[T]): query = query.join(target_alias, rel_attr.of_type(target_alias), isouter=True) joined_rel_keys.add(prop.key if prop is not None else rel_attr.key) + # 3) Ensure to-one touched by filters is joined if plan.filter_tables: for key, info in rels.items(): if info.uselist or not info.target_cls: @@ -393,6 +423,7 @@ class CRUDService(Generic[T]): query = query.join(getattr(root_alias, key), isouter=True) joined_rel_keys.add(key) + # 4) Collections via selectinload, optionally load_only for requested child columns for base_alias, rel_attr, _target_alias in plan.join_paths: if base_alias is not root_alias: continue @@ -408,10 +439,46 @@ class CRUDService(Generic[T]): if cols: opt = opt.load_only(*cols) query = query.options(opt) + + for path, _names in (plan.rel_field_names or {}).items(): + if not path: + continue + # Build a chained selectinload for each relationship segment in the path + first = path[0] + info = rels.get(first) + if not info or info.target_cls is None: + continue + + # Start with selectinload on the first hop + opt = selectinload(getattr(root_alias, first)) + + # Walk deeper segments + cur_cls = info.target_cls + for seg in path[1:]: + sub = rel_map(cur_cls).get(seg) + if not sub or sub.target_cls is None: + # if segment isn't a relationship, we stop the chain + break + opt = opt.selectinload(getattr(cur_cls, seg)) + cur_cls = sub.target_cls + + query = query.options(opt) + return query + def _apply_proj_opts(self, query, plan: _Plan): - return query.options(*plan.proj_opts) if plan.proj_opts else query + if not plan.proj_opts: + return query + try: + return query.options(*plan.proj_opts) + except KeyError as e: + # Seen "KeyError: 'col'" when alias-column remapping meets unlabeled exprs. + log.debug("Projection options disabled due to %r; proceeding without them.", e) + return query + except Exception as e: + log.debug("Projection options failed (%r); proceeding without them.", e) + return query def _projection_meta(self, plan: _Plan): if plan.req_fields: