Skip to content

context: _load_views_v1 returns non-mapping entries, crashing validate/build/migrate on legacy v1 projects #2597

Description

@goldmedal

Summary

_load_views_v1 (core/wren/src/wren/context.py) is the only project loader that does not filter non-mapping entries out of the list it returns. Every one of its four consumers then calls .get() / .pop() on the entries, so a legacy (schema_version: 1) project whose hand-written views.yml contains a non-mapping list item crashes with a bare AttributeError — including in wren context validate, whose job is to report problems in hand-edited files, and in the v1→v2 migration, which can be left half-applied.

Root cause

def _load_views_v1(project_path: Path) -> list[dict]:
    """Legacy: load views from project_path/views.yml."""
    views_file = project_path / "views.yml"
    if not views_file.exists():
        return []
    data = yaml.safe_load(views_file.read_text(encoding="utf-8")) or {}
    return data.get("views", []) if isinstance(data, dict) else []   # <- outer dict checked, items not

The sibling loaders all guard the items themselves and are unaffected:

loader filters non-dict items?
_load_models_v1 yes — if isinstance(data, dict)
_load_models_v2 yes — if not isinstance(model, dict): continue
_load_views_v2 yes — if not isinstance(view, dict): continue
_load_views_v1 no

The return annotation says list[dict], so consumers are reasonable in assuming it.

Reproduction

project/
  wren_project.yml    # schema_version: 1
  views.yml
# views.yml
views:
  - null
  - "junk"
>>> load_views(p)
[None, 'junk']
>>> validate_project(p)
AttributeError: 'NoneType' object has no attribute 'get'
>>> build_manifest(p)
AttributeError: 'NoneType' object has no attribute 'pop'
>>> build_json(p)
AttributeError: 'NoneType' object has no attribute 'pop'

Impact

Four consumers, all of which assume mappings:

  1. validate_project — crashes instead of emitting a ValidationError. This is the worst fit: the whole point of the validator is to tell the user what's wrong with a hand-edited project, and a malformed views.yml is exactly the input it should diagnose.
  2. build_manifest / build_json — crash in the for v in views: v.pop("_source_dir", None) strip step, so the project can't be built or queried either.
  3. _plan_v1_to_v2 — the migration plan/dry-run crashes at view.get("name").
  4. _apply_v1_to_v2 — the migration apply crashes at the same line, but only after the models loop has already written the new models/<name>/ directories and unlink()ed the old flat models/*.yml files. The project is left half-migrated: models restructured, views.yml untouched, no view directories, and the failure surfaces as an unhandled AttributeError.

Suggested fix

One place, at the loader, matching what the other three loaders already do:

    views = data.get("views", []) if isinstance(data, dict) else []
    return [v for v in views if isinstance(v, dict)]

That restores the list[dict] contract for all four consumers at once. Whether silently dropping the bad entry or surfacing it is preferable is worth a moment's thought — dropping matches the existing loaders' behaviour, and validate_project could grow a separate check that reads views.yml directly if we want the user to be told rather than have it silently ignored.

Credit

Surfaced while reviewing #2567, which patches the validate_project consumer rather than the loader (and so leaves build_manifest, build_json and the two migration paths still crashing).

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions