Skip to content

fix: harden schema property error handling across AST classes #23

@dtsong

Description

@dtsong

Context

Follow-up from PR #21 code review. Three hardening opportunities that affect Cte.schema and related schema properties (Join.schema, Select.schema).

1. type(s)(...) assumes single-arg constructor for all Schema subtypes

Cte.schema, Join.schema (line 487), and Select.schema (line 585) all use type(s)(dict(...)) to reconstruct schemas. This breaks if a Schema subclass requires additional constructor args (e.g., db_name). The existing CaseAwareMapping.new() method was designed for this but is unused.

Consider using s.new(...) consistently, or documenting the single-arg constructor contract.

Files: data_diff/queries/ast_classes.py lines 487, 585, 647

2. Inner schema errors lose CTE context

When self.table.schema raises (e.g., from a nested Join without columns), the exception propagates without any CTE context. Wrapping with a chained exception would aid debugging:

try:
    s = self.table.schema
except QueryBuilderError as exc:
    raise QueryBuilderError(f"Failed to resolve schema for CTE: {exc}") from exc

File: data_diff/queries/ast_classes.py, line 638

3. No runtime validation on params element types

params is annotated Sequence[str] | None but attrs doesn't enforce this at runtime. Passing params=[1, 2] produces confusing errors deep in CaseInsensitiveDict.__init__ (AttributeError: 'int' object has no attribute 'lower').

File: data_diff/queries/ast_classes.py, line 630

Metadata

Metadata

Assignees

No one assigned

    Labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions