diff --git a/crudkit/core/service.py b/crudkit/core/service.py index 60e3aef..07514ce 100644 --- a/crudkit/core/service.py +++ b/crudkit/core/service.py @@ -247,30 +247,32 @@ class CRUDService(Generic[T]): # Detect first hops that have deeper, nested tails requested (e.g. "contact.supervisor") nested_first_hops = { path[0] for path in (rel_field_names or {}).keys() if len(path) > 1 } - # IMPORTANT: - # - Only attach loader options for first-hop relations from the root. - # - Always use selectinload here (avoid contains_eager joins). - # - Let compile_projections() supply deep chained options. + used_contains_eager = False for base_alias, rel_attr, target_alias in join_paths: - is_firsthop_from_root = (base_alias is root_alias) - if not is_firsthop_from_root: - # Deeper hops are handled by proj_opts below - continue - prop = getattr(rel_attr, "property", None) - is_collection = bool(getattr(prop, "uselist", False)) + is_collection = bool(getattr(getattr(rel_attr, "property", None), "uselist", False)) is_nested_firsthop = rel_attr.key in nested_first_hops - opt = selectinload(rel_attr) - # Optional narrowng for collections - if is_collection: - child_names = (collection_field_names or {}).get(rel_attr.key, []) - if child_names: - target_cls = prop.mapper.class_ - cols = [getattr(target_cls, n, None) for n in child_names] - cols = [c for c in cols if isinstance(c, InstrumentedAttribute)] - if cols: - opt = opt.load_only(*cols) - query = query.options(opt) + if is_collection or is_nested_firsthop: + # Use selectinload so deeper hops can chain cleanly (and to avoid + # contains_eager/loader conflicts on nested paths). + opt = selectinload(rel_attr) + + # Narrow columns for collections if we know child scalar names + if is_collection: + child_names = (collection_field_names or {}).get(rel_attr.key, []) + if child_names: + target_cls = rel_attr.property.mapper.class_ + cols = [getattr(target_cls, n, None) for n in child_names] + cols = [c for c in cols if isinstance(c, InstrumentedAttribute)] + if cols: + opt = opt.load_only(*cols) + + query = query.options(opt) + else: + # Simple first-hop scalar rel with no deeper tails: safe to join + contains_eager + query = query.join(target_alias, rel_attr.of_type(target_alias), isouter=True) + query = query.options(contains_eager(rel_attr, alias=target_alias)) + used_contains_eager = True # Filters AFTER joins → no cartesian products if filters: @@ -363,10 +365,6 @@ class CRUDService(Generic[T]): last_key = None # Count DISTINCT ids with mirrored joins - - # Apply deep projection loader options (safe: we avoided contains_eager) - if proj_opts: - query = query.options(*proj_opts) total = None if include_total: base = session.query(getattr(root_alias, "id")) @@ -468,25 +466,26 @@ class CRUDService(Generic[T]): nested_first_hops = { path[0] for path in (rel_field_names or {}).keys() if len(path) > 1 } - # First-hop only; use selectinload (no contains_eager) + used_contains_eager = False for base_alias, rel_attr, target_alias in join_paths: - is_firsthop_from_root = (base_alias is root_alias) - if not is_firsthop_from_root: - continue - prop = getattr(rel_attr, "property", None) - is_collection = bool(getattr(prop, "uselist", False)) - _is_nested_firsthop = rel_attr.key in nested_first_hops + is_collection = bool(getattr(getattr(rel_attr, "property", None), "uselist", False)) + is_nested_firsthop = rel_attr.key in nested_first_hops - opt = selectinload(rel_attr) - if is_collection: - child_names = (collection_field_names or {}).get(rel_attr.key, []) - if child_names: - target_cls = prop.mapper.class_ - cols = [getattr(target_cls, n, None) for n in child_names] - cols = [c for c in cols if isinstance(c, InstrumentedAttribute)] - if cols: - opt = opt.load_only(*cols) - query = query.options(opt) + if is_collection or is_nested_firsthop: + opt = selectinload(rel_attr) + if is_collection: + child_names = (collection_field_names or {}).get(rel_attr.key, []) + if child_names: + target_cls = rel_attr.property.mapper.class_ + cols = [getattr(target_cls, n, None) for n in child_names] + cols = [c for c in cols if isinstance(c, InstrumentedAttribute)] + if cols: + opt = opt.load_only(*cols) + query = query.options(opt) + else: + query = query.join(target_alias, rel_attr.of_type(target_alias), isouter=True) + query = query.options(contains_eager(rel_attr, alias=target_alias)) + used_contains_eager = True # Apply filters (joins are in place → no cartesian products) if filters: @@ -498,7 +497,7 @@ class CRUDService(Generic[T]): # Projection loader options compiled from requested fields. # Skip if we used contains_eager to avoid loader-strategy conflicts. expanded_fields, proj_opts = compile_projection(self.model, req_fields) if req_fields else ([], []) - if proj_opts: + if proj_opts and not used_contains_eager: query = query.options(*proj_opts) obj = query.first() @@ -566,25 +565,26 @@ class CRUDService(Generic[T]): nested_first_hops = { path[0] for path in (rel_field_names or {}).keys() if len(path) > 1 } - # First-hop only; use selectinload - for base_alias, rel_attr, target_alias in join_paths: - is_firsthop_from_root = (base_alias is root_alias) - if not is_firsthop_from_root: - continue - prop = getattr(rel_attr, "property", None) - is_collection = bool(getattr(prop, "uselist", False)) - _is_nested_firsthop = rel_attr.key in nested_first_hops + used_contains_eager = False + for _base_alias, rel_attr, target_alias in join_paths: + is_collection = bool(getattr(getattr(rel_attr, "property", None), "uselist", False)) + is_nested_firsthop = rel_attr.key in nested_first_hops - opt = selectinload(rel_attr) - if is_collection: - child_names = (collection_field_names or {}).get(rel_attr.key, []) - if child_names: - target_cls = prop.mapper.class_ - cols = [getattr(target_cls, n, None) for n in child_names] - cols = [c for c in cols if isinstance(c, InstrumentedAttribute)] - if cols: - opt = opt.load_only(*cols) - query = query.options(opt) + if is_collection or is_nested_firsthop: + opt = selectinload(rel_attr) + if is_collection: + child_names = (collection_field_names or {}).get(rel_attr.key, []) + if child_names: + target_cls = rel_attr.property.mapper.class_ + cols = [getattr(target_cls, n, None) for n in child_names] + cols = [c for c in cols if isinstance(c, InstrumentedAttribute)] + if cols: + opt = opt.load_only(*cols) + query = query.options(opt) + else: + query = query.join(target_alias, rel_attr.of_type(target_alias), isouter=True) + query = query.options(contains_eager(rel_attr, alias=target_alias)) + used_contains_eager = True # Filters AFTER joins → no cartesian products if filters: @@ -608,7 +608,7 @@ class CRUDService(Generic[T]): # Projection loaders only if we didn’t use contains_eager expanded_fields, proj_opts = compile_projection(self.model, req_fields) if req_fields else ([], []) - if proj_opts: + if proj_opts and not used_contains_eager: query = query.options(*proj_opts) else: diff --git a/crudkit/ui/fragments.py b/crudkit/ui/fragments.py index b4db83f..c443d8b 100644 --- a/crudkit/ui/fragments.py +++ b/crudkit/ui/fragments.py @@ -66,46 +66,13 @@ _ENUMS = { } def get_env(): - """ - Return an overlay Jinja Environment that knows how to load crudkit templates - and has our helper functions available as globals. - """ app = current_app default_path = os.path.join(os.path.dirname(__file__), 'templates') fallback_loader = FileSystemLoader(default_path) - env = app.jinja_env.overlay(loader=ChoiceLoader([app.jinja_loader, fallback_loader])) - # Ensure helpers are available even when we render via this overlay env. - # These names are resolved at *call time* (not at def time), so it's safe. - try: - env.globals.setdefault("render_table", render_table) - env.globals.setdefault("render_form", render_form) - env.globals.setdefault("render_field", render_field) - except NameError: - # Functions may not be defined yet at import time; later calls will set them. - pass - - return env - -def register_template_globals(app=None): - """ - Register crudkit helpers as app-wide Jinja globals so they can be used - directly in any template via {{ render_table(...) }}, {{ render_form(...) }}, - and {{ render_field(...) }}. - """ - if app is None: - app = current_app - # Idempotent install using an extension flag - installed = app.extensions.setdefault("crudkit_ui_helpers", set()) - to_register = { - "render_table": render_table, - "render_form": render_form, - "render_field": render_field, - } - for name, fn in to_register.items(): - if name not in installed: - app.add_template_global(fn, name) - installed.add(name) + return app.jinja_env.overlay( + loader=ChoiceLoader([app.jinja_loader, fallback_loader]) + ) def expand_projection(model_cls, fields): req = getattr(model_cls, "__crudkit_field_requires__", {}) or {} diff --git a/inventory/routes/entry.py b/inventory/routes/entry.py index 7800fdd..5ed0007 100644 --- a/inventory/routes/entry.py +++ b/inventory/routes/entry.py @@ -4,7 +4,7 @@ from sqlalchemy.inspection import inspect from typing import Any, Dict, List, Tuple, Callable, Optional import crudkit -from crudkit.ui.fragments import render_form, register_template_globals +from crudkit.ui.fragments import render_form from crudkit.core import normalize_payload bp_entry = Blueprint("entry", __name__) @@ -16,9 +16,7 @@ def _fields_for_model(model: str): if model == "inventory": fields = ["label", "name", "serial", "barcode", "brand", "model", - "device_type", "owner", "location", "condition", "image", - "notes", "work_logs.start_time", "work_logs.work_item.label", - "work_logs.contact.label", "work_logs.complete"] + "device_type", "owner", "location", "condition", "image", "notes"] fields_spec = [ {"name": "label", "type": "display", "label": "", "row": "label", "attrs": {"class": "display-6 mb-3"}, "wrap": {"class": "col"}}, @@ -58,7 +56,6 @@ def _fields_for_model(model: str): "wrap": {"class": "h-100 w-100"}}, {"name": "notes", "type": "template", "label": "Notes", "row": "notes", "wrap": {"class": "col"}, "template": "inventory_note.html"}, - {"name": "work_logs", "type": "template", "row": "notes", "wrap": {"class": "col"}, "template": "inventory_logs.html"}, ] layout = [ {"name": "label", "order": 5, "attrs": {"class": "row align-items-center"}}, @@ -168,8 +165,6 @@ def _apply_worklog_updates(worklog, updates, delete_ids): note_svc.delete(uid, actor="bulk_child_delete", commit=False) def init_entry_routes(app): - # Make helpers available in all templates - register_template_globals(app) @bp_entry.get("/entry//") def entry(model: str, id: int): diff --git a/inventory/routes/listing.py b/inventory/routes/listing.py index 241e3b8..b7837f3 100644 --- a/inventory/routes/listing.py +++ b/inventory/routes/listing.py @@ -3,14 +3,11 @@ from flask import Blueprint, render_template, abort, request import crudkit from crudkit.api._cursor import decode_cursor, encode_cursor -from crudkit.ui.fragments import render_table, register_template_globals +from crudkit.ui.fragments import render_table bp_listing = Blueprint("listing", __name__) def init_listing_routes(app): - # Make helpers available in all templates - register_template_globals(app) - @bp_listing.get("/listing/") def show_list(model): if model.lower() not in {"inventory", "user", "worklog"}: diff --git a/inventory/templates/entry.html b/inventory/templates/entry.html index 3c8c6bf..d107960 100644 --- a/inventory/templates/entry.html +++ b/inventory/templates/entry.html @@ -1,7 +1,6 @@ {% extends 'base.html' %} {% block main %} -
{{ form | safe }}
diff --git a/inventory/templates/inventory_logs.html b/inventory/templates/inventory_logs.html deleted file mode 100644 index 00f54f1..0000000 --- a/inventory/templates/inventory_logs.html +++ /dev/null @@ -1,8 +0,0 @@ - -

- {{ field }} -

-

- {{ field['template_ctx']['values']['work_logs'] }} -

-{% include "table.html" %} \ No newline at end of file